chore: sift metrics on both endpoints (#10375)

https://linear.app/unleash/issue/2-3696/report-unknown-flags-when-sent-to-the-bulk-metrics-endpoint

Unifies metrics sifting logic across both metrics endpoints:
- `/metrics`
- `/metrics/bulk`

This PR improves consistency between the `/metrics` and `/metrics/bulk`
endpoints by introducing a shared `siftMetrics` method, now used within
`registerBulkMetrics`. Both endpoints already call this method at the
end of their respective logic flows, ensuring that metrics are sifted in
the same way regardless of the path taken.

While the primary goal was to enable reporting of unknown flags via the
`/metrics/bulk` endpoint, this change also improves bulk processing by
consistently dropping invalid or unknown flags before insertion, just
like in the regular `/metrics` endpoint.

src/lib/features/metrics/client-metrics/metrics-service-v2.test.ts

@@ -113,7 +113,7 @@ test('process metrics properly even when some names are not url friendly, filter
     );
 
     // only toggle with a bad name gets filtered out
-    expect(eventBus.emit).not.toHaveBeenCalled();
+    expect(eventBus.emit).toHaveBeenCalledTimes(1);
     expect(lastSeenService.updateLastSeen).not.toHaveBeenCalled();
 });
 

src/lib/features/metrics/client-metrics/metrics-service-v2.ts

@@ -32,6 +32,7 @@ import {
     MetricsTranslator,
 } from '../impact/metrics-translator.js';
 import { impactRegister } from '../impact/impact-register.js';
+import type { UnknownFlag } from '../unknown-flags/unknown-flags-store.js';
 
 export default class ClientMetricsServiceV2 {
     private config: IUnleashConfig;
@@ -178,12 +179,78 @@ export default class ClientMetricsServiceV2 {
         return toggleNames;
     }
 
+    private async siftMetrics(
+        metrics: IClientMetricsEnv[],
+    ): Promise<IClientMetricsEnv[]> {
+        if (!metrics.length) return [];
+
+        const metricsByToggle = new Map<string, IClientMetricsEnv[]>();
+        for (const m of metrics) {
+            if (m.yes === 0 && m.no === 0) continue;
+            let arr = metricsByToggle.get(m.featureName);
+            if (!arr) {
+                arr = [];
+                metricsByToggle.set(m.featureName, arr);
+            }
+            arr.push(m);
+        }
+        if (metricsByToggle.size === 0) return [];
+
+        const toggleNames = Array.from(metricsByToggle.keys());
+
+        const { validatedToggleNames, unknownToggleNames } =
+            await this.filterExistingToggleNames(toggleNames);
+
+        const validatedSet = new Set(validatedToggleNames);
+        const unknownSet = new Set(unknownToggleNames);
+
+        const invalidCount = toggleNames.length - validatedSet.size;
+        this.logger.debug(
+            `Got ${toggleNames.length} metrics (${invalidCount > 0 ? `${invalidCount} invalid` : 'all valid'}).`,
+        );
+
+        const unknownFlags: UnknownFlag[] = [];
+        for (const [featureName, group] of metricsByToggle) {
+            if (unknownSet.has(featureName)) {
+                for (const m of group) {
+                    unknownFlags.push({
+                        name: featureName,
+                        appName: m.appName,
+                        seenAt: m.timestamp,
+                        environment: m.environment,
+                    });
+                }
+            }
+        }
+        if (unknownFlags.length) {
+            const sample = unknownFlags
+                .slice(0, 10)
+                .map((f) => `"${f.name}"`)
+                .join(', ');
+            this.logger.debug(
+                `Registering ${unknownFlags.length} unknown flags; sample: ${sample}`,
+            );
+            this.unknownFlagsService.register(unknownFlags);
+        }
+
+        const siftedMetrics: IClientMetricsEnv[] = [];
+        for (const [featureName, group] of metricsByToggle) {
+            if (validatedSet.has(featureName)) {
+                siftedMetrics.push(...group);
+            }
+        }
+        return siftedMetrics;
+    }
+
     async registerBulkMetrics(metrics: IClientMetricsEnv[]): Promise<void> {
+        const siftedMetrics = await this.siftMetrics(metrics);
+        if (siftedMetrics.length === 0) return;
+
         this.unsavedMetrics = collapseHourlyMetrics([
             ...this.unsavedMetrics,
-            ...metrics,
+            ...siftedMetrics,
         ]);
-        this.lastSeenService.updateLastSeen(metrics);
+        this.lastSeenService.updateLastSeen(siftedMetrics);
     }
 
     async registerImpactMetrics(impactMetrics: Metric[]) {
@@ -202,22 +269,6 @@ export default class ClientMetricsServiceV2 {
         clientIp: string,
     ): Promise<void> {
         const value = await clientMetricsSchema.validateAsync(data);
-        const toggleNames = Object.keys(value.bucket.toggles).filter(
-            (name) =>
-                !(
-                    value.bucket.toggles[name].yes === 0 &&
-                    value.bucket.toggles[name].no === 0
-                ),
-        );
-
-        const { validatedToggleNames, unknownToggleNames } =
-            await this.filterExistingToggleNames(toggleNames);
-
-        const invalidToggleNames =
-            toggleNames.length - validatedToggleNames.length;
-        this.logger.debug(
-            `Got ${toggleNames.length} (${invalidToggleNames > 0 ? `${invalidToggleNames} invalid ones` : 'all valid'}) metrics from ${value.appName}`,
-        );
 
         if (data.sdkVersion) {
             const [sdkName, sdkVersion] = data.sdkVersion.split(':');
@@ -235,38 +286,20 @@ export default class ClientMetricsServiceV2 {
             this.config.eventBus.emit(CLIENT_REGISTER, heartbeatEvent);
         }
 
-        const environment = value.environment ?? 'default';
+        const clientMetrics: IClientMetricsEnv[] = Object.keys(
+            value.bucket.toggles,
+        ).map((name) => ({
+            featureName: name,
+            appName: value.appName,
+            environment: value.environment ?? 'default',
+            timestamp: value.bucket.stop, //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,
+        }));
 
-        if (unknownToggleNames.length > 0) {
-            const unknownFlags = unknownToggleNames.map((name) => ({
-                name,
-                appName: value.appName,
-                seenAt: value.bucket.stop,
-                environment,
-            }));
-            this.logger.debug(
-                `Registering ${unknownFlags.length} unknown flags from ${value.appName} in the ${environment} environment. Some of the unknown flag names include: ${unknownFlags
-                    .slice(0, 10)
-                    .map(({ name }) => `"${name}"`)
-                    .join(', ')}`,
-            );
-            this.unknownFlagsService.register(unknownFlags);
-        }
-
-        if (validatedToggleNames.length > 0) {
-            const clientMetrics: IClientMetricsEnv[] = validatedToggleNames.map(
-                (name) => ({
-                    featureName: name,
-                    appName: value.appName,
-                    environment,
-                    timestamp: value.bucket.stop, //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,
-                }),
-            );
+        if (clientMetrics.length) {
             await this.registerBulkMetrics(clientMetrics);
-
             this.config.eventBus.emit(CLIENT_METRICS, clientMetrics);
         }
     }

src/lib/features/metrics/instance/metrics.test.ts

@@ -414,6 +414,11 @@ describe('bulk metrics', () => {
     test('without access to production environment due to no auth setup, we can only access the default env', async () => {
         const now = new Date();
 
+        // @ts-expect-error - cachedFeatureNames is a private property in ClientMetricsServiceV2
+        services.clientMetricsServiceV2.cachedFeatureNames = vi
+            .fn<() => Promise<string[]>>()
+            .mockResolvedValue(['test_feature_one', 'test_feature_two']);
+
         await request
             .post('/api/client/metrics/bulk')
             .send({

src/lib/features/metrics/unknown-flags/unknown-flags.e2e.test.ts

@@ -0,0 +1,158 @@
+import supertest, { type Test } from 'supertest';
+import getApp from '../../../app.js';
+import { createTestConfig } from '../../../../test/config/test-config.js';
+import {
+    type IUnleashServices,
+    createServices,
+} from '../../../services/index.js';
+import type {
+    IUnleashConfig,
+    IUnleashOptions,
+    IUnleashStores,
+} from '../../../types/index.js';
+import dbInit, {
+    type ITestDb,
+} from '../../../../test/e2e/helpers/database-init.js';
+import { startOfHour } from 'date-fns';
+import type TestAgent from 'supertest/lib/agent.d.ts';
+import type { BulkRegistrationSchema } from '../../../openapi/index.js';
+
+let db: ITestDb;
+let config: IUnleashConfig;
+
+async function getSetup(opts?: IUnleashOptions) {
+    config = createTestConfig(opts);
+    db = await dbInit('unknown_flags', config.getLogger);
+
+    const services = createServices(db.stores, config, db.rawDatabase);
+    const app = await getApp(config, db.stores, services);
+    return {
+        request: supertest(app),
+        stores: db.stores,
+        services,
+        db: db.rawDatabase,
+        destroy: db.destroy,
+    };
+}
+
+let request: TestAgent<Test>;
+let stores: IUnleashStores;
+let services: IUnleashServices;
+let destroy: () => Promise<void>;
+
+beforeAll(async () => {
+    const setup = await getSetup({
+        experimental: {
+            flags: {
+                reportUnknownFlags: true,
+            },
+        },
+    });
+    request = setup.request;
+    stores = setup.stores;
+    destroy = setup.destroy;
+    services = setup.services;
+});
+
+afterAll(async () => {
+    await destroy();
+});
+
+afterEach(async () => {
+    await stores.unknownFlagsStore.deleteAll();
+});
+
+describe('should register unknown flags', () => {
+    test('/metrics endpoint', async () => {
+        // @ts-expect-error - cachedFeatureNames is a private property in ClientMetricsServiceV2
+        services.clientMetricsServiceV2.cachedFeatureNames = vi
+            .fn<() => Promise<string[]>>()
+            .mockResolvedValue(['existing_flag']);
+
+        await request
+            .post('/api/client/metrics')
+            .send({
+                appName: 'demo',
+                instanceId: '1',
+                bucket: {
+                    start: Date.now(),
+                    stop: Date.now(),
+                    toggles: {
+                        existing_flag: {
+                            yes: 200,
+                            no: 0,
+                        },
+                        unknown_flag: {
+                            yes: 100,
+                            no: 50,
+                        },
+                    },
+                },
+            })
+            .expect(202);
+
+        await services.unknownFlagsService.flush();
+        const unknownFlags = await services.unknownFlagsService.getAll();
+
+        expect(unknownFlags).toHaveLength(1);
+        expect(unknownFlags[0]).toMatchObject({
+            name: 'unknown_flag',
+            environment: 'development',
+            appName: 'demo',
+            seenAt: expect.any(Date),
+        });
+    });
+
+    test('/metrics/bulk endpoint', async () => {
+        // @ts-expect-error - cachedFeatureNames is a private property in ClientMetricsServiceV2
+        services.clientMetricsServiceV2.cachedFeatureNames = vi
+            .fn<() => Promise<string[]>>()
+            .mockResolvedValue(['existing_flag']);
+
+        const unknownFlag: BulkRegistrationSchema = {
+            appName: 'demo',
+            instanceId: '1',
+            environment: 'development',
+            sdkVersion: 'unleash-client-js:1.0.0',
+            sdkType: 'frontend',
+        };
+
+        await request
+            .post('/api/client/metrics/bulk')
+            .send({
+                applications: [unknownFlag],
+                metrics: [
+                    {
+                        featureName: 'existing_flag',
+                        environment: 'development',
+                        appName: 'demo',
+                        timestamp: startOfHour(new Date()),
+                        yes: 200,
+                        no: 0,
+                        variants: {},
+                    },
+                    {
+                        featureName: 'unknown_flag',
+                        environment: 'development',
+                        appName: 'demo',
+                        timestamp: startOfHour(new Date()),
+                        yes: 100,
+                        no: 50,
+                        variants: {},
+                    },
+                ],
+            })
+            .expect(202);
+
+        await services.unknownFlagsService.flush();
+        const unknownFlags = await services.unknownFlagsService.getAll();
+
+        expect(unknownFlags).toHaveLength(1);
+        expect(unknownFlags[0]).toMatchObject({
+            name: 'unknown_flag',
+            environment: 'development',
+            appName: 'demo',
+            seenAt: expect.any(Date),
+        });
+    });
+});