From 532d4ed9b4e31e8ac6e1590268ff57279248cb6e Mon Sep 17 00:00:00 2001 From: ijemmy Date: Mon, 3 Jan 2022 16:55:53 +0100 Subject: [PATCH 1/6] fix(metrics): Support multiple addMetric() call with the same metric name --- packages/metrics/package-lock.json | 2 +- packages/metrics/package.json | 2 +- packages/metrics/src/Metrics.ts | 35 ++++++++++--- packages/metrics/src/types/Metrics.ts | 2 +- packages/metrics/tests/unit/Metrics.test.ts | 49 +++++++++++++++++++ .../tests/unit/middleware/middy.test.ts | 37 ++++++++++++-- 6 files changed, 115 insertions(+), 12 deletions(-) diff --git a/packages/metrics/package-lock.json b/packages/metrics/package-lock.json index 8ef46633ad..604e2a6ce0 100644 --- a/packages/metrics/package-lock.json +++ b/packages/metrics/package-lock.json @@ -6,7 +6,7 @@ "packages": { "": { "name": "@aws-lambda-powertools/metrics", - "version": "0.2.0-beta.13", + "version": "0.2.0-beta.14", "license": "MIT-0", "dependencies": { "@aws-lambda-powertools/commons": "^0.0.2", diff --git a/packages/metrics/package.json b/packages/metrics/package.json index e9beadbdfe..ef9e6f8291 100644 --- a/packages/metrics/package.json +++ b/packages/metrics/package.json @@ -10,7 +10,7 @@ "commit": "commit", "test": "jest --group=unit --detectOpenHandles --coverage --verbose", "test:e2e": "jest --group=e2e", - "watch": "jest --watch", + "watch": "jest --group=unit --watch ", "build": "tsc", "lint": "eslint --ext .ts --fix --no-error-on-unmatched-pattern src tests", "format": "eslint --fix --ext .ts --fix --no-error-on-unmatched-pattern src tests", diff --git a/packages/metrics/src/Metrics.ts b/packages/metrics/src/Metrics.ts index 6a51589d37..ad71ce3b45 100644 --- a/packages/metrics/src/Metrics.ts +++ b/packages/metrics/src/Metrics.ts @@ -311,7 +311,7 @@ class Metrics implements MetricsInterface { if (!this.namespace) console.warn('Namespace should be defined, default used'); const metricValues = Object.values(this.storedMetrics).reduce( - (result: { [key: string]: number }, { name, value }: { name: string; value: number }) => { + (result: { [key: string]: number | number[] }, { name, value }: { name: string; value: number | number[] }) => { result[name] = value; return result; @@ -390,6 +390,19 @@ class Metrics implements MetricsInterface { return this.envVarsService; } + private isNewMetric(name: string, unit: MetricUnit): boolean { + if (this.storedMetrics[name]){ + // Inconsistent units indicates a bug or typos. We want to flag this to users early early + if (this.storedMetrics[name].unit !== unit) { + throw new Error('The same metric name has been added before with a different unit.'); + } + + return false; + } else { + return true; + } + } + private setCustomConfigService(customConfigService?: ConfigServiceInterface): void { this.customConfigService = customConfigService ? customConfigService : undefined; } @@ -430,12 +443,22 @@ class Metrics implements MetricsInterface { if (Object.keys(this.storedMetrics).length >= MAX_METRICS_SIZE) { this.purgeStoredMetrics(); } - this.storedMetrics[name] = { - unit, - value, - name, - }; + + if (this.isNewMetric(name, unit)) { + this.storedMetrics[name] = { + unit, + value, + name, + }; + } else { + const storedMetric = this.storedMetrics[name]; + if (!Array.isArray(storedMetric.value)) { + storedMetric.value = [storedMetric.value]; + } + storedMetric.value.push(value); + } } + } export { Metrics, MetricUnits }; diff --git a/packages/metrics/src/types/Metrics.ts b/packages/metrics/src/types/Metrics.ts index 0ceb2bc551..d33ecf119e 100644 --- a/packages/metrics/src/types/Metrics.ts +++ b/packages/metrics/src/types/Metrics.ts @@ -59,7 +59,7 @@ type ExtraOptions = { type StoredMetric = { name: string unit: MetricUnit - value: number + value: number | number[] }; type StoredMetrics = { diff --git a/packages/metrics/tests/unit/Metrics.test.ts b/packages/metrics/tests/unit/Metrics.test.ts index 2e348f39f5..8a8595a374 100644 --- a/packages/metrics/tests/unit/Metrics.test.ts +++ b/packages/metrics/tests/unit/Metrics.test.ts @@ -435,6 +435,55 @@ describe('Class: Metrics', () => { expect(serializedMetrics._aws.CloudWatchMetrics[0].Namespace).toBe(DEFAULT_NAMESPACE); expect(console.warn).toHaveBeenNthCalledWith(1, 'Namespace should be defined, default used'); }); + + test('Should contain a metric value if added once', ()=> { + const metrics = new Metrics(); + + metrics.addMetric('test_name', MetricUnits.Count, 1); + const serializedMetrics = metrics.serializeMetrics(); + + expect(serializedMetrics._aws.CloudWatchMetrics[0].Metrics.length).toBe(1); + + expect(serializedMetrics['test_name']).toBe(1); + }); + + test('Should convert multiple metrics with the same name and unit into an array', ()=> { + const metrics = new Metrics(); + + metrics.addMetric('test_name', MetricUnits.Count, 2); + metrics.addMetric('test_name', MetricUnits.Count, 1); + const serializedMetrics = metrics.serializeMetrics(); + + expect(serializedMetrics._aws.CloudWatchMetrics[0].Metrics.length).toBe(1); + expect(serializedMetrics['test_name']).toStrictEqual([ 2, 1 ]); + }); + + test('Should throw an error if the same metric name is added again with a different unit', ()=> { + const metrics = new Metrics(); + + metrics.addMetric('test_name', MetricUnits.Count, 2); + try { + metrics.addMetric('test_name', MetricUnits.Seconds, 10); + } catch (e) { + expect((e).message).toBe('The same metric name has been added before with a different unit.'); + } + }); + + test('Should contain multiple metric values if added with multiple names', ()=> { + const metrics = new Metrics(); + + metrics.addMetric('test_name', MetricUnits.Count, 1); + metrics.addMetric('test_name2', MetricUnits.Count, 2); + const serializedMetrics = metrics.serializeMetrics(); + + expect(serializedMetrics._aws.CloudWatchMetrics[0].Metrics).toStrictEqual([ + { Name: 'test_name', Unit: 'Count' }, + { Name: 'test_name2', Unit: 'Count' }, + ]); + + expect(serializedMetrics['test_name']).toBe(1); + expect(serializedMetrics['test_name2']).toBe(2); + }); }); describe('Feature: Clearing Metrics ', () => { diff --git a/packages/metrics/tests/unit/middleware/middy.test.ts b/packages/metrics/tests/unit/middleware/middy.test.ts index 8a48c3b0e9..218bfa2f15 100644 --- a/packages/metrics/tests/unit/middleware/middy.test.ts +++ b/packages/metrics/tests/unit/middleware/middy.test.ts @@ -44,6 +44,40 @@ describe('Middy middleware', () => { succeed: () => console.log('Succeeded!'), }; + test('when a metrics instance receive multiple metrics with the same name, it prints multiple values in an array format', async () => { + // Prepare + const metrics = new Metrics({ namespace:'serverlessAirline', service:'orders' }); + + const lambdaHandler = (): void => { + metrics.addMetric('successfulBooking', MetricUnits.Count, 2); + metrics.addMetric('successfulBooking', MetricUnits.Count, 1); + }; + + const handler = middy(lambdaHandler).use(logMetrics(metrics)); + + // Act + await handler(event, context, () => console.log('Lambda invoked!')); + + // Assess + expect(console.log).toHaveBeenNthCalledWith(1, JSON.stringify({ + '_aws': { + 'Timestamp': 1466424490000, + 'CloudWatchMetrics': [{ + 'Namespace': 'serverlessAirline', + 'Dimensions': [ + ['service'] + ], + 'Metrics': [{ 'Name': 'successfulBooking', 'Unit': 'Count' }], + }], + }, + 'service': 'orders', + 'successfulBooking': [ + 2, + 1, + ], + })); + }); + test('when a metrics instance is passed WITH custom options, it prints the metrics in the stdout', async () => { // Prepare @@ -51,7 +85,6 @@ describe('Middy middleware', () => { const lambdaHandler = (): void => { metrics.addMetric('successfulBooking', MetricUnits.Count, 1); - metrics.addMetric('successfulBooking', MetricUnits.Count, 1); }; const metricsOptions: ExtraOptions = { raiseOnEmptyMetrics: true, @@ -107,7 +140,6 @@ describe('Middy middleware', () => { const lambdaHandler = (): void => { metrics.addMetric('successfulBooking', MetricUnits.Count, 1); - metrics.addMetric('successfulBooking', MetricUnits.Count, 1); }; const handler = middy(lambdaHandler).use(logMetrics(metrics)); @@ -140,7 +172,6 @@ describe('Middy middleware', () => { const lambdaHandler = (): void => { metrics.addMetric('successfulBooking', MetricUnits.Count, 1); - metrics.addMetric('successfulBooking', MetricUnits.Count, 1); }; const metricsOptions: ExtraOptions = { raiseOnEmptyMetrics: true From 2752ed7b876532d20f1922782e98f011f5b0db2b Mon Sep 17 00:00:00 2001 From: ijemmy Date: Mon, 3 Jan 2022 17:13:51 +0100 Subject: [PATCH 2/6] docs(metrics): Add section about adding the same metric name multiple time --- docs/core/metrics.md | 133 +++++++++++++++++++++++++++++-------------- 1 file changed, 91 insertions(+), 42 deletions(-) diff --git a/docs/core/metrics.md b/docs/core/metrics.md index 9a9f906311..6250674d22 100644 --- a/docs/core/metrics.md +++ b/docs/core/metrics.md @@ -132,6 +132,55 @@ You can create metrics using `addMetric`, and you can create dimensions for all !!! warning "Do not create metrics or dimensions outside the handler" Metrics or dimensions added in the global scope will only be added during cold start. Disregard if that's the intended behaviour. +### Adding the same metric multiple times +You can call `addMetric()` with the same name multiple times. The values will be grouped together in an array. + +=== "addMetric() with the same name" + + ```typescript hl_lines="8 10" + import { Metrics, MetricUnits } from '@aws-lambda-powertools/metrics'; + import { Context } from 'aws-lambda'; + + + const metrics = new Metrics({namespace:"serverlessAirline", service:"orders"}); + + export const handler = async (event: any, context: Context) => { + metrics.addMetric('performedActionA', MetricUnits.Count, 2); + // do something else... + metrics.addMetric('performedActionA', MetricUnits.Count, 1); + } + ``` +=== "Example CloudWatch Logs excerpt" + + ```json hl_lines="15-16 23-26" + { + "successfulBooking": 1.0, + "_aws": { + "Timestamp": 1592234975665, + "CloudWatchMetrics": [ + { + "Namespace": "serverlessAirline", + "Dimensions": [ + [ + "service" + ] + ], + "Metrics": [ + { + "Name": "performedActionA", + "Unit": "Count" + } + ] + } + ] + }, + "service": "orders", + "performedActionA": [ + 2, + 1 + ] + } + ``` ### Adding default dimensions You can use add default dimensions to your metrics by passing them as parameters in 4 ways: @@ -264,23 +313,23 @@ See below an example of how to automatically flush metrics with the Middy-compat { "bookingConfirmation": 1.0, "_aws": { - "Timestamp": 1592234975665, - "CloudWatchMetrics": [ - { - "Namespace": "exampleApplication", - "Dimensions": [ - [ - "service" - ] - ], - "Metrics": [ + "Timestamp": 1592234975665, + "CloudWatchMetrics": [ { - "Name": "bookingConfirmation", - "Unit": "Count" + "Namespace": "exampleApplication", + "Dimensions": [ + [ + "service" + ] + ], + "Metrics": [ + { + "Name": "bookingConfirmation", + "Unit": "Count" + } + ] } ] - } - ] }, "service": "exampleService" } @@ -316,23 +365,23 @@ export class MyFunction { { "bookingConfirmation": 1.0, "_aws": { - "Timestamp": 1592234975665, - "CloudWatchMetrics": [ - { - "Namespace": "exampleApplication", - "Dimensions": [ - [ - "service" - ] - ], - "Metrics": [ + "Timestamp": 1592234975665, + "CloudWatchMetrics": [ { - "Name": "bookingConfirmation", - "Unit": "Count" + "Namespace": "exampleApplication", + "Dimensions": [ + [ + "service" + ] + ], + "Metrics": [ + { + "Name": "bookingConfirmation", + "Unit": "Count" + } + ] } ] - } - ] }, "service": "exampleService" } @@ -456,23 +505,23 @@ You can add high-cardinality data as part of your Metrics log with `addMetadata` { "successfulBooking": 1.0, "_aws": { - "Timestamp": 1592234975665, - "CloudWatchMetrics": [ - { - "Namespace": "exampleApplication", - "Dimensions": [ - [ - "service" - ] - ], - "Metrics": [ + "Timestamp": 1592234975665, + "CloudWatchMetrics": [ { - "Name": "successfulBooking", - "Unit": "Count" + "Namespace": "exampleApplication", + "Dimensions": [ + [ + "service" + ] + ], + "Metrics": [ + { + "Name": "successfulBooking", + "Unit": "Count" + } + ] } ] - } - ] }, "service": "booking", "bookingId": "7051cd10-6283-11ec-90d6-0242ac120003" From d06020f621b57a47c13b3cecb2bb8d9481a70a98 Mon Sep 17 00:00:00 2001 From: ijemmy Date: Mon, 3 Jan 2022 19:56:27 +0100 Subject: [PATCH 3/6] docs(metrics): Rearrange excerpt example to be the same as others --- docs/core/metrics.md | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/docs/core/metrics.md b/docs/core/metrics.md index 6250674d22..ac75a1fc08 100644 --- a/docs/core/metrics.md +++ b/docs/core/metrics.md @@ -152,9 +152,12 @@ You can call `addMetric()` with the same name multiple times. The values will be ``` === "Example CloudWatch Logs excerpt" - ```json hl_lines="15-16 23-26" + ```json hl_lines="2-5 18-19" { - "successfulBooking": 1.0, + "performedActionA": [ + 2, + 1 + ], "_aws": { "Timestamp": 1592234975665, "CloudWatchMetrics": [ @@ -174,11 +177,7 @@ You can call `addMetric()` with the same name multiple times. The values will be } ] }, - "service": "orders", - "performedActionA": [ - 2, - 1 - ] + "service": "orders" } ``` ### Adding default dimensions From 55b20ea5c066ef66885ca68988ee4a952c7522b9 Mon Sep 17 00:00:00 2001 From: ijemmy Date: Tue, 4 Jan 2022 09:19:45 +0100 Subject: [PATCH 4/6] docs(metrics) Change to more appropriate term Co-authored-by: Heitor Lessa --- docs/core/metrics.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/core/metrics.md b/docs/core/metrics.md index ac75a1fc08..1148d0e172 100644 --- a/docs/core/metrics.md +++ b/docs/core/metrics.md @@ -132,7 +132,7 @@ You can create metrics using `addMetric`, and you can create dimensions for all !!! warning "Do not create metrics or dimensions outside the handler" Metrics or dimensions added in the global scope will only be added during cold start. Disregard if that's the intended behaviour. -### Adding the same metric multiple times +### Adding multi-value metrics You can call `addMetric()` with the same name multiple times. The values will be grouped together in an array. === "addMetric() with the same name" From 7580f260c2702364d82eb7febc8a2e29154e955a Mon Sep 17 00:00:00 2001 From: ijemmy Date: Tue, 4 Jan 2022 13:23:38 +0100 Subject: [PATCH 5/6] fix(metrics): Fix typos + grammar in a comment Co-authored-by: Sara Gerion <47529391+saragerion@users.noreply.github.com> --- packages/metrics/src/Metrics.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/metrics/src/Metrics.ts b/packages/metrics/src/Metrics.ts index ad71ce3b45..1d9cf833b7 100644 --- a/packages/metrics/src/Metrics.ts +++ b/packages/metrics/src/Metrics.ts @@ -392,7 +392,7 @@ class Metrics implements MetricsInterface { private isNewMetric(name: string, unit: MetricUnit): boolean { if (this.storedMetrics[name]){ - // Inconsistent units indicates a bug or typos. We want to flag this to users early early + // Inconsistent units indicates a bug or typos and we want to flag this to users early if (this.storedMetrics[name].unit !== unit) { throw new Error('The same metric name has been added before with a different unit.'); } From 05323b46d2af1b50de4a7ac7b713eae98168afe3 Mon Sep 17 00:00:00 2001 From: ijemmy Date: Tue, 4 Jan 2022 14:26:28 +0100 Subject: [PATCH 6/6] fix(metrics): change an error message to suggest a potential solution --- packages/metrics/src/Metrics.ts | 3 ++- packages/metrics/tests/unit/Metrics.test.ts | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/metrics/src/Metrics.ts b/packages/metrics/src/Metrics.ts index 1d9cf833b7..219cf7de20 100644 --- a/packages/metrics/src/Metrics.ts +++ b/packages/metrics/src/Metrics.ts @@ -394,7 +394,8 @@ class Metrics implements MetricsInterface { if (this.storedMetrics[name]){ // Inconsistent units indicates a bug or typos and we want to flag this to users early if (this.storedMetrics[name].unit !== unit) { - throw new Error('The same metric name has been added before with a different unit.'); + const currentUnit = this.storedMetrics[name].unit; + throw new Error(`Metric "${name}" has already been added with unit "${currentUnit}", but we received unit "${unit}". Did you mean to use metric unit "${currentUnit}"?`); } return false; diff --git a/packages/metrics/tests/unit/Metrics.test.ts b/packages/metrics/tests/unit/Metrics.test.ts index 8a8595a374..2216026f40 100644 --- a/packages/metrics/tests/unit/Metrics.test.ts +++ b/packages/metrics/tests/unit/Metrics.test.ts @@ -465,7 +465,7 @@ describe('Class: Metrics', () => { try { metrics.addMetric('test_name', MetricUnits.Seconds, 10); } catch (e) { - expect((e).message).toBe('The same metric name has been added before with a different unit.'); + expect((e).message).toBe('Metric "test_name" has already been added with unit "Count", but we received unit "Seconds". Did you mean to use metric unit "Count"?'); } });