From f532619558aade9c1293e068b6bc34da8b57fe88 Mon Sep 17 00:00:00 2001 From: zirkelc Date: Sun, 24 Nov 2024 14:32:24 +0100 Subject: [PATCH 1/3] feat(metrics): disable metrics with env var --- packages/metrics/src/Metrics.ts | 34 ++++++++++++- .../src/config/EnvironmentVariablesService.ts | 11 +++++ packages/metrics/tests/unit/Metrics.test.ts | 49 +++++++++++++++++++ 3 files changed, 92 insertions(+), 2 deletions(-) diff --git a/packages/metrics/src/Metrics.ts b/packages/metrics/src/Metrics.ts index ea4fba2270..c594e7bac4 100644 --- a/packages/metrics/src/Metrics.ts +++ b/packages/metrics/src/Metrics.ts @@ -201,6 +201,11 @@ class Metrics extends Utility implements MetricsInterface { */ private storedMetrics: StoredMetrics = {}; + /** + * Whether to disable metrics + */ + private disabled = false; + /** * Custom timestamp for the metrics */ @@ -480,6 +485,13 @@ class Metrics extends Utility implements MetricsInterface { return Object.keys(this.storedMetrics).length > 0; } + /** + * Whether metrics are disabled. + */ + public isDisabled(): boolean { + return this.disabled; + } + /** * A class method decorator to automatically log metrics after the method returns or throws an error. * @@ -587,8 +599,14 @@ class Metrics extends Utility implements MetricsInterface { 'If application metrics should never be empty, consider using `throwOnEmptyMetrics`' ); } - const emfOutput = this.serializeMetrics(); - hasMetrics && this.console.log(JSON.stringify(emfOutput)); + + if (!this.disabled) { + const emfOutput = this.serializeMetrics(); + hasMetrics && this.console.log(JSON.stringify(emfOutput)); + } else { + // TODO log warning that metrics are disabled? + } + this.clearMetrics(); this.clearDimensions(); this.clearMetadata(); @@ -921,6 +939,17 @@ class Metrics extends Utility implements MetricsInterface { this.getEnvVarsService().getNamespace()) as string; } + /** + * Set the disbaled flag based on the environment variables `POWERTOOLS_METRICS_DISABLED` and `POWERTOOLS_DEV`. + */ + private setDisabled(): void { + this.disabled = + this.getEnvVarsService().isDevMode() || + this.getEnvVarsService().getMetricsDisabled(); + + // TODO if POWERTOOLS_METRICS_DISABLED is set to false, and dev mode is enabled, then POWERTOOLS_METRICS_DISABLED takes precedence and the utility stays enabled. + } + /** * Set the options to be used by the Metrics instance. * @@ -943,6 +972,7 @@ class Metrics extends Utility implements MetricsInterface { this.setNamespace(namespace); this.setService(serviceName); this.setDefaultDimensions(defaultDimensions); + this.setDisabled(); this.isSingleMetric = singleMetric || false; return this; diff --git a/packages/metrics/src/config/EnvironmentVariablesService.ts b/packages/metrics/src/config/EnvironmentVariablesService.ts index 5f277e3010..54142a8b70 100644 --- a/packages/metrics/src/config/EnvironmentVariablesService.ts +++ b/packages/metrics/src/config/EnvironmentVariablesService.ts @@ -13,12 +13,23 @@ class EnvironmentVariablesService { private namespaceVariable = 'POWERTOOLS_METRICS_NAMESPACE'; + private disabledVariable = 'POWERTOOLS_METRICS_DISABLED'; + /** * Get the value of the `POWERTOOLS_METRICS_NAMESPACE` environment variable. */ public getNamespace(): string { return this.get(this.namespaceVariable); } + + /** + * Get the value of the `POWERTOOLS_METRICS_DISABLED` environment variable. + */ + public getMetricsDisabled(): boolean { + const value = this.get(this.disabledVariable); + + return this.isValueTrue(value); + } } export { EnvironmentVariablesService }; diff --git a/packages/metrics/tests/unit/Metrics.test.ts b/packages/metrics/tests/unit/Metrics.test.ts index f555e91515..907a29a267 100644 --- a/packages/metrics/tests/unit/Metrics.test.ts +++ b/packages/metrics/tests/unit/Metrics.test.ts @@ -1418,6 +1418,29 @@ describe('Class: Metrics', () => { expect(consoleLogSpy).toBeCalledWith(JSON.stringify(mockData)); }); + test('it should not log anything if metrics are disabled', () => { + // Prepare + process.env.POWERTOOLS_METRICS_DISABLED = 'true'; + const customLogger = { + log: jest.fn(), + warn: jest.fn(), + debug: jest.fn(), + error: jest.fn(), + info: jest.fn(), + }; + const metrics: Metrics = new Metrics({ + namespace: TEST_NAMESPACE, + logger: customLogger, + }); + const consoleLogSpy = jest.spyOn(customLogger, 'log'); + + // Act + metrics.publishStoredMetrics(); + + // Assess + expect(consoleLogSpy).toHaveBeenCalledTimes(0); + }); + test('it should call clearMetrics function', () => { // Prepare const metrics: Metrics = new Metrics({ namespace: TEST_NAMESPACE }); @@ -2343,6 +2366,32 @@ describe('Class: Metrics', () => { }); }); + describe('Method: isDisabled', () => { + it('should be enabled by default', () => { + const metrics: Metrics = new Metrics({ namespace: TEST_NAMESPACE }); + + // Act & Assess + expect(metrics.isDisabled()).toBe(false); + }); + + it('should be disabled if POWERTOOLS_DEV is set to true', () => { + process.env.POWERTOOLS_DEV = 'true'; + const metrics: Metrics = new Metrics({ namespace: TEST_NAMESPACE }); + + // Act & Assess + expect(metrics.isDisabled()).toBe(true); + }); + + it('should be disabled if POWERTOOLS_METRICS_DISABLED is set to true', () => { + // Prepare + process.env.POWERTOOLS_METRICS_DISABLED = 'true'; + const metrics: Metrics = new Metrics({ namespace: TEST_NAMESPACE }); + + // Act & Assess + expect(metrics.isDisabled()).toBe(true); + }); + }); + describe('Method: setTimestamp', () => { const testCases = [ { From 423268c4eb63a8c83414a93b700326fe28130a4b Mon Sep 17 00:00:00 2001 From: zirkelc Date: Mon, 25 Nov 2024 09:56:50 +0100 Subject: [PATCH 2/3] feat(metrics): resolve comments --- .../src/config/EnvironmentVariablesService.ts | 11 +++++++ .../unit/EnvironmentVariablesService.test.ts | 31 +++++++++++++++++++ packages/metrics/src/Metrics.ts | 18 ++++++----- .../src/config/EnvironmentVariablesService.ts | 10 ++++-- packages/metrics/tests/unit/Metrics.test.ts | 28 +++++++++++++++-- 5 files changed, 84 insertions(+), 14 deletions(-) diff --git a/packages/commons/src/config/EnvironmentVariablesService.ts b/packages/commons/src/config/EnvironmentVariablesService.ts index f75ac127d0..d9d45d8f88 100644 --- a/packages/commons/src/config/EnvironmentVariablesService.ts +++ b/packages/commons/src/config/EnvironmentVariablesService.ts @@ -93,6 +93,17 @@ class EnvironmentVariablesService implements ConfigServiceInterface { return truthyValues.includes(value.toLowerCase()); } + /** + * Helper function to determine if a value is considered falsy. + * + * @param value The value to check for falsiness. + */ + public isValueFalse(value: string): boolean { + const falsyValues: string[] = ['0', 'n', 'no', 'f', 'false', 'off']; + + return falsyValues.includes(value.toLowerCase()); + } + /** * Get the AWS X-Ray Trace data from the environment variable. * diff --git a/packages/commons/tests/unit/EnvironmentVariablesService.test.ts b/packages/commons/tests/unit/EnvironmentVariablesService.test.ts index f941dbf6cb..fcf9974527 100644 --- a/packages/commons/tests/unit/EnvironmentVariablesService.test.ts +++ b/packages/commons/tests/unit/EnvironmentVariablesService.test.ts @@ -159,6 +159,37 @@ describe('Class: EnvironmentVariablesService', () => { ); }); + describe('Method: isValueFalse', () => { + const valuesToTest: Array> = [ + ['0', true], + ['n', true], + ['no', true], + ['f', true], + ['FALSE', true], + ['off', true], + ['1', false], + ['y', false], + ['yes', false], + ['t', false], + ['TRUE', false], + ['on', false], + ['', false], + ['somethingsilly', false], + ]; + + it.each(valuesToTest)( + 'takes string "%s" and returns %s', + (input, output) => { + // Prepare + const service = new EnvironmentVariablesService(); + // Act + const value = service.isValueFalse(input as string); + // Assess + expect(value).toBe(output); + } + ); + }); + describe('Method: isDevMode', () => { it('returns true if the environment variable POWERTOOLS_DEV is "true"', () => { // Prepare diff --git a/packages/metrics/src/Metrics.ts b/packages/metrics/src/Metrics.ts index c594e7bac4..4860a96e98 100644 --- a/packages/metrics/src/Metrics.ts +++ b/packages/metrics/src/Metrics.ts @@ -488,7 +488,7 @@ class Metrics extends Utility implements MetricsInterface { /** * Whether metrics are disabled. */ - public isDisabled(): boolean { + protected isDisabled(): boolean { return this.disabled; } @@ -603,8 +603,6 @@ class Metrics extends Utility implements MetricsInterface { if (!this.disabled) { const emfOutput = this.serializeMetrics(); hasMetrics && this.console.log(JSON.stringify(emfOutput)); - } else { - // TODO log warning that metrics are disabled? } this.clearMetrics(); @@ -941,13 +939,17 @@ class Metrics extends Utility implements MetricsInterface { /** * Set the disbaled flag based on the environment variables `POWERTOOLS_METRICS_DISABLED` and `POWERTOOLS_DEV`. + * + * The `POWERTOOLS_METRICS_DISABLED` environment variable takes precedence over `POWERTOOLS_DEV`. */ private setDisabled(): void { - this.disabled = - this.getEnvVarsService().isDevMode() || - this.getEnvVarsService().getMetricsDisabled(); + const devMode = this.getEnvVarsService().isDevMode(); + if (devMode) this.disabled = true; - // TODO if POWERTOOLS_METRICS_DISABLED is set to false, and dev mode is enabled, then POWERTOOLS_METRICS_DISABLED takes precedence and the utility stays enabled. + // This returns boolean or undefined, so we have to use explicit checks on true and false + const metricsDisabled = this.getEnvVarsService().getMetricsDisabled(); + if (metricsDisabled === true) this.disabled = true; + if (metricsDisabled === false) this.disabled = false; } /** @@ -969,10 +971,10 @@ class Metrics extends Utility implements MetricsInterface { this.setEnvVarsService(); this.setConsole(); this.setCustomConfigService(customConfigService); + this.setDisabled(); this.setNamespace(namespace); this.setService(serviceName); this.setDefaultDimensions(defaultDimensions); - this.setDisabled(); this.isSingleMetric = singleMetric || false; return this; diff --git a/packages/metrics/src/config/EnvironmentVariablesService.ts b/packages/metrics/src/config/EnvironmentVariablesService.ts index 54142a8b70..81aa3537ff 100644 --- a/packages/metrics/src/config/EnvironmentVariablesService.ts +++ b/packages/metrics/src/config/EnvironmentVariablesService.ts @@ -13,7 +13,7 @@ class EnvironmentVariablesService { private namespaceVariable = 'POWERTOOLS_METRICS_NAMESPACE'; - private disabledVariable = 'POWERTOOLS_METRICS_DISABLED'; + private readonly disabledVariable = 'POWERTOOLS_METRICS_DISABLED'; /** * Get the value of the `POWERTOOLS_METRICS_NAMESPACE` environment variable. @@ -25,10 +25,14 @@ class EnvironmentVariablesService /** * Get the value of the `POWERTOOLS_METRICS_DISABLED` environment variable. */ - public getMetricsDisabled(): boolean { + public getMetricsDisabled(): boolean | undefined { const value = this.get(this.disabledVariable); - return this.isValueTrue(value); + if (this.isValueTrue(value)) return true; + + if (this.isValueFalse(value)) return false; + + return undefined; } } diff --git a/packages/metrics/tests/unit/Metrics.test.ts b/packages/metrics/tests/unit/Metrics.test.ts index 907a29a267..9bb1c1230c 100644 --- a/packages/metrics/tests/unit/Metrics.test.ts +++ b/packages/metrics/tests/unit/Metrics.test.ts @@ -2371,7 +2371,8 @@ describe('Class: Metrics', () => { const metrics: Metrics = new Metrics({ namespace: TEST_NAMESPACE }); // Act & Assess - expect(metrics.isDisabled()).toBe(false); + // biome-ignore lint/complexity/useLiteralKeys: accessing protected method + expect(metrics['isDisabled']()).toBe(false); }); it('should be disabled if POWERTOOLS_DEV is set to true', () => { @@ -2379,7 +2380,8 @@ describe('Class: Metrics', () => { const metrics: Metrics = new Metrics({ namespace: TEST_NAMESPACE }); // Act & Assess - expect(metrics.isDisabled()).toBe(true); + // biome-ignore lint/complexity/useLiteralKeys: accessing protected method + expect(metrics['isDisabled']()).toBe(true); }); it('should be disabled if POWERTOOLS_METRICS_DISABLED is set to true', () => { @@ -2388,7 +2390,27 @@ describe('Class: Metrics', () => { const metrics: Metrics = new Metrics({ namespace: TEST_NAMESPACE }); // Act & Assess - expect(metrics.isDisabled()).toBe(true); + // biome-ignore lint/complexity/useLiteralKeys: accessing protected method + expect(metrics['isDisabled']()).toBe(true); + }); + + it('should be enabled if POWERTOOLS_METRICS_DISABLED is set to false', () => { + process.env.POWERTOOLS_METRICS_DISABLED = 'false'; + const metrics: Metrics = new Metrics({ namespace: TEST_NAMESPACE }); + + // Act & Assess + // biome-ignore lint/complexity/useLiteralKeys: accessing protected method + expect(metrics['isDisabled']()).toBe(false); + }); + + it('should be enabled if POWERTOOLS_DEV is set to true and POWERTOOLS_METRICS_DISABLED is set to false', () => { + process.env.POWERTOOLS_DEV = 'true'; + process.env.POWERTOOLS_METRICS_DISABLED = 'false'; + const metrics: Metrics = new Metrics({ namespace: TEST_NAMESPACE }); + + // Act & Assess + // biome-ignore lint/complexity/useLiteralKeys: accessing protected method + expect(metrics['isDisabled']()).toBe(false); }); }); From 9e93028b5d12d24240b4dee92cb3347993896e94 Mon Sep 17 00:00:00 2001 From: zirkelc Date: Mon, 25 Nov 2024 10:32:36 +0100 Subject: [PATCH 3/3] feat(metrics): move env var logic --- packages/metrics/src/Metrics.ts | 8 +------- .../src/config/EnvironmentVariablesService.ts | 12 +++++++----- 2 files changed, 8 insertions(+), 12 deletions(-) diff --git a/packages/metrics/src/Metrics.ts b/packages/metrics/src/Metrics.ts index 4860a96e98..5ff04ef8fb 100644 --- a/packages/metrics/src/Metrics.ts +++ b/packages/metrics/src/Metrics.ts @@ -943,13 +943,7 @@ class Metrics extends Utility implements MetricsInterface { * The `POWERTOOLS_METRICS_DISABLED` environment variable takes precedence over `POWERTOOLS_DEV`. */ private setDisabled(): void { - const devMode = this.getEnvVarsService().isDevMode(); - if (devMode) this.disabled = true; - - // This returns boolean or undefined, so we have to use explicit checks on true and false - const metricsDisabled = this.getEnvVarsService().getMetricsDisabled(); - if (metricsDisabled === true) this.disabled = true; - if (metricsDisabled === false) this.disabled = false; + this.disabled = this.getEnvVarsService().getMetricsDisabled(); } /** diff --git a/packages/metrics/src/config/EnvironmentVariablesService.ts b/packages/metrics/src/config/EnvironmentVariablesService.ts index 81aa3537ff..a611308649 100644 --- a/packages/metrics/src/config/EnvironmentVariablesService.ts +++ b/packages/metrics/src/config/EnvironmentVariablesService.ts @@ -23,16 +23,18 @@ class EnvironmentVariablesService } /** - * Get the value of the `POWERTOOLS_METRICS_DISABLED` environment variable. + * Get the value of the `POWERTOOLS_METRICS_DISABLED` or `POWERTOOLS_DEV` environment variables. + * + * The `POWERTOOLS_METRICS_DISABLED` environment variable takes precedence over `POWERTOOLS_DEV`. */ - public getMetricsDisabled(): boolean | undefined { + public getMetricsDisabled(): boolean { const value = this.get(this.disabledVariable); - if (this.isValueTrue(value)) return true; - if (this.isValueFalse(value)) return false; + if (this.isValueTrue(value)) return true; + if (this.isDevMode()) return true; - return undefined; + return false; } }