From 2806c060a56b3255d1449124f14363dd5edeb5fb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gast=C3=B3n=20Fournier?= Date: Tue, 25 Jul 2023 12:49:09 +0000 Subject: [PATCH] fix: Client metrics name validation (#4339) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## About the changes 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 + src/lib/routes/client-api/metrics.ts | 11 +- .../client-metrics/metrics-service-v2.test.ts | 116 ++++++++++++++++++ .../client-metrics/metrics-service-v2.ts | 66 ++++++++-- src/lib/types/experimental.ts | 7 +- 5 files changed, 184 insertions(+), 18 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 0a5a0210bc..15b86a3838 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, "experimentalExtendedTelemetry": false, "featuresExportImport": true, + "filterInvalidClientMetrics": false, "googleAuthEnabled": false, "groupRootRoles": false, "maintenanceMode": false, @@ -113,6 +114,7 @@ exports[`should create default config 1`] = ` "embedProxyFrontend": true, "experimentalExtendedTelemetry": false, "featuresExportImport": true, + "filterInvalidClientMetrics": false, "googleAuthEnabled": false, "groupRootRoles": false, "maintenanceMode": false, diff --git a/src/lib/routes/client-api/metrics.ts b/src/lib/routes/client-api/metrics.ts index c879e2d975..253d7f2408 100644 --- a/src/lib/routes/client-api/metrics.ts +++ b/src/lib/routes/client-api/metrics.ts @@ -65,11 +65,14 @@ export default class ClientMetricsController extends Controller { } async registerMetrics(req: IAuthRequest, res: Response): Promise { - const { body: data, ip: clientIp, user } = req; - data.environment = this.metricsV2.resolveMetricsEnvironment(user, data); - await this.clientInstanceService.registerInstance(data, clientIp); - try { + const { body: data, ip: clientIp, user } = req; + data.environment = this.metricsV2.resolveMetricsEnvironment( + user, + data, + ); + await this.clientInstanceService.registerInstance(data, clientIp); + await this.metricsV2.registerClientMetrics(data, clientIp); res.status(202).end(); } catch (e) { 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 0b97023865..66ba4283af 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 { @@ -20,6 +20,7 @@ import { collapseHourlyMetrics } from '../../util/collapseHourlyMetrics'; import { LastSeenService } from './last-seen-service'; import { generateHourBuckets } from '../../util/time-utils'; import { ClientMetricsSchema } from 'lib/openapi'; +import { nameSchema } from '../../schema/feature-schema'; export default class ClientMetricsServiceV2 { private config: IUnleashConfig; @@ -32,6 +33,8 @@ export default class ClientMetricsServiceV2 { private lastSeenService: LastSeenService; + private flagResolver: Pick; + private logger: Logger; constructor( @@ -46,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(() => { @@ -60,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, @@ -81,20 +111,30 @@ export default class ClientMetricsServiceV2 { ), ); - this.logger.debug(`got metrics from ${clientIp}`); + const validatedToggleNames = await this.filterValidToggleNames( + toggleNames, + ); - 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.logger.debug( + `Got ${toggleNames.length} (${validatedToggleNames.length} valid) metrics from ${clientIp}`, + ); - this.config.eventBus.emit(CLIENT_METRICS, value); + 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); + } } async bulkAdd(): Promise { diff --git a/src/lib/types/experimental.ts b/src/lib/types/experimental.ts index 828f70ab1a..c801a47744 100644 --- a/src/lib/types/experimental.ts +++ b/src/lib/types/experimental.ts @@ -25,7 +25,8 @@ export type IFlagKey = | 'disableNotifications' | 'advancedPlayground' | 'customRootRoles' - | 'strategySplittedButton'; + | 'strategySplittedButton' + | 'filterInvalidClientMetrics'; export type IFlags = Partial<{ [key in IFlagKey]: boolean | Variant }>; @@ -118,6 +119,10 @@ const flags: IFlags = { process.env.UNLEASH_EXPERIMENTAL_CUSTOM_ROOT_ROLES, false, ), + filterInvalidClientMetrics: parseEnvVarBoolean( + process.env.FILTER_INVALID_CLIENT_METRICS, + false, + ), }; export const defaultExperimentalOptions: IExperimentalOptions = {