From 9398bd969ef30600fd826c31a5139069b90ec1ad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gast=C3=B3n=20Fournier?= Date: Tue, 25 Jul 2023 13:33:21 +0000 Subject: [PATCH] fix: Client metrics name validation (#4339) (#4342) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1. Add a test for the failing use case (we can see it [here](https://github.com/Unleash/unleash/actions/runs/5656229196/job/15322845002?pr=4339#step:5:783)): ``` FAIL src/lib/services/client-metrics/metrics-service-v2.test.ts ● process metrics properly even when some names are not url friendly ValidationError: "name" must be URL friendly ``` 2. Fix and handle this gracefully 3. Added a new toggle to silently ignore bad names: filterInvalidClientMetrics Fixes: https://github.com/Unleash/unleash/pull/4193 --- .../__snapshots__/create-config.test.ts.snap | 2 + .../client-metrics/metrics-service-v2.test.ts | 116 ++++++++++++++++++ .../client-metrics/metrics-service-v2.ts | 76 ++++++++---- src/lib/types/experimental.ts | 7 +- 4 files changed, 177 insertions(+), 24 deletions(-) create mode 100644 src/lib/services/client-metrics/metrics-service-v2.test.ts diff --git a/src/lib/__snapshots__/create-config.test.ts.snap b/src/lib/__snapshots__/create-config.test.ts.snap index 89a6733d66..5f4f91b842 100644 --- a/src/lib/__snapshots__/create-config.test.ts.snap +++ b/src/lib/__snapshots__/create-config.test.ts.snap @@ -79,6 +79,7 @@ exports[`should create default config 1`] = ` "embedProxyFrontend": true, "emitPotentiallyStaleEvents": false, "featuresExportImport": true, + "filterInvalidClientMetrics": false, "googleAuthEnabled": false, "maintenanceMode": false, "messageBanner": { @@ -113,6 +114,7 @@ exports[`should create default config 1`] = ` "embedProxyFrontend": true, "emitPotentiallyStaleEvents": false, "featuresExportImport": true, + "filterInvalidClientMetrics": false, "googleAuthEnabled": false, "maintenanceMode": false, "messageBanner": { diff --git a/src/lib/services/client-metrics/metrics-service-v2.test.ts b/src/lib/services/client-metrics/metrics-service-v2.test.ts new file mode 100644 index 0000000000..db9336cfc0 --- /dev/null +++ b/src/lib/services/client-metrics/metrics-service-v2.test.ts @@ -0,0 +1,116 @@ +import ClientMetricsServiceV2 from './metrics-service-v2'; + +import getLogger from '../../../test/fixtures/no-logger'; + +import createStores from '../../../test/fixtures/store'; +import EventEmitter from 'events'; +import { LastSeenService } from './last-seen-service'; +import { IUnleashConfig } from 'lib/types'; + +function initClientMetrics(flagEnabled = true) { + const stores = createStores(); + + const eventBus = new EventEmitter(); + eventBus.emit = jest.fn(); + + const config = { + eventBus, + getLogger, + flagResolver: { + isEnabled: () => { + return flagEnabled; + }, + }, + } as unknown as IUnleashConfig; + + const lastSeenService = new LastSeenService(stores, config); + lastSeenService.updateLastSeen = jest.fn(); + + const service = new ClientMetricsServiceV2(stores, config, lastSeenService); + return { clientMetricsService: service, eventBus, lastSeenService }; +} + +test('process metrics properly', async () => { + const { clientMetricsService, eventBus, lastSeenService } = + initClientMetrics(); + await clientMetricsService.registerClientMetrics( + { + appName: 'test', + bucket: { + start: '1982-07-25T12:00:00.000Z', + stop: '2023-07-25T12:00:00.000Z', + toggles: { + myCoolToggle: { + yes: 25, + no: 42, + variants: { + blue: 6, + green: 15, + red: 46, + }, + }, + myOtherToggle: { + yes: 0, + no: 100, + }, + }, + }, + environment: 'test', + }, + '127.0.0.1', + ); + + expect(eventBus.emit).toHaveBeenCalledTimes(1); + expect(lastSeenService.updateLastSeen).toHaveBeenCalledTimes(1); +}); + +test('process metrics properly even when some names are not url friendly, filtering out invalid names when flag is on', async () => { + const { clientMetricsService, eventBus, lastSeenService } = + initClientMetrics(); + await clientMetricsService.registerClientMetrics( + { + appName: 'test', + bucket: { + start: '1982-07-25T12:00:00.000Z', + stop: '2023-07-25T12:00:00.000Z', + toggles: { + 'not url friendly ☹': { + yes: 0, + no: 100, + }, + }, + }, + environment: 'test', + }, + '127.0.0.1', + ); + + // only toggle with a bad name gets filtered out + expect(eventBus.emit).not.toHaveBeenCalled(); + expect(lastSeenService.updateLastSeen).not.toHaveBeenCalled(); +}); + +test('process metrics properly even when some names are not url friendly, with default behavior when flag is off', async () => { + const { clientMetricsService, eventBus, lastSeenService } = + initClientMetrics(false); + await clientMetricsService.registerClientMetrics( + { + appName: 'test', + bucket: { + start: '1982-07-25T12:00:00.000Z', + stop: '2023-07-25T12:00:00.000Z', + toggles: { + 'not url friendly ☹': { + yes: 0, + no: 100, + }, + }, + }, + environment: 'test', + }, + '127.0.0.1', + ); + + expect(eventBus.emit).toHaveBeenCalledTimes(1); + expect(lastSeenService.updateLastSeen).toHaveBeenCalledTimes(1); +}); diff --git a/src/lib/services/client-metrics/metrics-service-v2.ts b/src/lib/services/client-metrics/metrics-service-v2.ts index 308c4192dc..e766110e44 100644 --- a/src/lib/services/client-metrics/metrics-service-v2.ts +++ b/src/lib/services/client-metrics/metrics-service-v2.ts @@ -1,5 +1,5 @@ import { Logger } from '../../logger'; -import { IUnleashConfig } from '../../types'; +import { IFlagResolver, IUnleashConfig } from '../../types'; import { IUnleashStores } from '../../types'; import { ToggleMetricsSummary } from '../../types/models/metrics'; import { @@ -21,7 +21,6 @@ import { LastSeenService } from './last-seen-service'; import { generateHourBuckets } from '../../util/time-utils'; import { ClientMetricsSchema } from 'lib/openapi'; import { nameSchema } from '../../schema/feature-schema'; -import { BadDataError } from '../../error'; export default class ClientMetricsServiceV2 { private config: IUnleashConfig; @@ -34,6 +33,8 @@ export default class ClientMetricsServiceV2 { private lastSeenService: LastSeenService; + private flagResolver: Pick; + private logger: Logger; constructor( @@ -48,6 +49,7 @@ export default class ClientMetricsServiceV2 { this.logger = config.getLogger( '/services/client-metrics/client-metrics-service-v2.ts', ); + this.flagResolver = config.flagResolver; this.timers.push( setInterval(() => { @@ -62,6 +64,32 @@ export default class ClientMetricsServiceV2 { ); } + async filterValidToggleNames(toggleNames: string[]): Promise { + const nameValidations: Promise< + PromiseFulfilledResult<{ name: string }> | PromiseRejectedResult + >[] = toggleNames.map((toggleName) => + nameSchema.validateAsync({ name: toggleName }), + ); + const badNames = (await Promise.allSettled(nameValidations)).filter( + (r) => r.status === 'rejected', + ); + if (badNames.length > 0) { + this.logger.warn( + `Got a few toggles with invalid names: ${JSON.stringify( + badNames, + )}`, + ); + + if (this.flagResolver.isEnabled('filterInvalidClientMetrics')) { + const justNames = badNames.map( + (r: PromiseRejectedResult) => r.reason._original.name, + ); + return toggleNames.filter((name) => !justNames.includes(name)); + } + } + return toggleNames; + } + async registerBulkMetrics(metrics: IClientMetricsEnv[]): Promise { this.unsavedMetrics = collapseHourlyMetrics([ ...this.unsavedMetrics, @@ -83,28 +111,30 @@ export default class ClientMetricsServiceV2 { ), ); - for (const toggle of toggleNames) { - if (!(await nameSchema.validateAsync({ name: toggle }))) { - throw new BadDataError( - `Invalid feature toggle name "${toggle}"`, - ); - } + const validatedToggleNames = await this.filterValidToggleNames( + toggleNames, + ); + + this.logger.debug( + `Got ${toggleNames.length} (${validatedToggleNames.length} valid) metrics from ${clientIp}`, + ); + + if (validatedToggleNames.length > 0) { + const clientMetrics: IClientMetricsEnv[] = validatedToggleNames.map( + (name) => ({ + featureName: name, + appName: value.appName, + environment: value.environment ?? 'default', + timestamp: value.bucket.start, //we might need to approximate between start/stop... + yes: value.bucket.toggles[name].yes ?? 0, + no: value.bucket.toggles[name].no ?? 0, + variants: value.bucket.toggles[name].variants, + }), + ); + await this.registerBulkMetrics(clientMetrics); + + this.config.eventBus.emit(CLIENT_METRICS, value); } - - this.logger.debug(`got metrics from ${clientIp}`); - - const clientMetrics: IClientMetricsEnv[] = toggleNames.map((name) => ({ - featureName: name, - appName: value.appName, - environment: value.environment, - timestamp: value.bucket.start, //we might need to approximate between start/stop... - yes: value.bucket.toggles[name].yes, - no: value.bucket.toggles[name].no, - variants: value.bucket.toggles[name].variants, - })); - await this.registerBulkMetrics(clientMetrics); - - this.config.eventBus.emit(CLIENT_METRICS, value); } async bulkAdd(): Promise { diff --git a/src/lib/types/experimental.ts b/src/lib/types/experimental.ts index 3a92a314ed..9dd9625aac 100644 --- a/src/lib/types/experimental.ts +++ b/src/lib/types/experimental.ts @@ -26,7 +26,8 @@ export type IFlagKey = | 'newProjectLayout' | 'slackAppAddon' | 'emitPotentiallyStaleEvents' - | 'configurableFeatureTypeLifetimes'; + | 'configurableFeatureTypeLifetimes' + | 'filterInvalidClientMetrics'; export type IFlags = Partial<{ [key in IFlagKey]: boolean | Variant }>; @@ -121,6 +122,10 @@ const flags: IFlags = { process.env.UNLEASH_EXPERIMENTAL_CONFIGURABLE_FEATURE_TYPE_LIFETIMES, false, ), + filterInvalidClientMetrics: parseEnvVarBoolean( + process.env.FILTER_INVALID_CLIENT_METRICS, + false, + ), }; export const defaultExperimentalOptions: IExperimentalOptions = {