fix: Client metrics name validation (#4339) (#4342)

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

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": {

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);
+});

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<IFlagResolver, 'isEnabled'>;
+
     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<string[]> {
+        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<void> {
         this.unsavedMetrics = collapseHourlyMetrics([
             ...this.unsavedMetrics,
@@ -83,29 +111,31 @@ export default class ClientMetricsServiceV2 {
                 ),
         );
 
-        for (const toggle of toggleNames) {
+        const validatedToggleNames = await this.filterValidToggleNames(
-            if (!(await nameSchema.validateAsync({ name: toggle }))) {
+            toggleNames,
-                throw new BadDataError(
-                    `Invalid feature toggle name "${toggle}"`,
         );
-            }
-        }
 
-        this.logger.debug(`got metrics from ${clientIp}`);
+        this.logger.debug(
+            `Got ${toggleNames.length} (${validatedToggleNames.length} valid) metrics from ${clientIp}`,
+        );
 
-        const clientMetrics: IClientMetricsEnv[] = toggleNames.map((name) => ({
+        if (validatedToggleNames.length > 0) {
+            const clientMetrics: IClientMetricsEnv[] = validatedToggleNames.map(
+                (name) => ({
                     featureName: name,
                     appName: value.appName,
-            environment: value.environment,
+                    environment: value.environment ?? 'default',
                     timestamp: value.bucket.start, //we might need to approximate between start/stop...
-            yes: value.bucket.toggles[name].yes,
+                    yes: value.bucket.toggles[name].yes ?? 0,
-            no: value.bucket.toggles[name].no,
+                    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<void> {
         if (this.unsavedMetrics.length > 0) {

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 = {