feat: prevent double daily metrics insert (#5906)

Unleash · Jan 16, 2024 · f6c0624 · f6c0624
1 parent 967ee13
commit f6c0624
Show file tree

Hide file tree

Showing 7 changed files with 130 additions and 20 deletions.
diff --git a/src/lib/db/client-metrics-store-v2.test.ts b/src/lib/db/client-metrics-store-v2.test.ts
@@ -216,7 +216,8 @@ test('count previous day metrics', async () => {
         },
     ]);
 
-    const result = await clientMetricsStore.countPreviousDayMetricsBuckets();
+    const result =
+        await clientMetricsStore.countPreviousDayHourlyMetricsBuckets();
 
     expect(result).toMatchObject({ enabledCount: 2, variantCount: 4 });
 });
diff --git a/src/lib/db/client-metrics-store-v2.ts b/src/lib/db/client-metrics-store-v2.ts
@@ -339,7 +339,7 @@ export class ClientMetricsStoreV2 implements IClientMetricsStoreV2 {
             .del();
     }
 
-    async countPreviousDayMetricsBuckets(): Promise<{
+    async countPreviousDayHourlyMetricsBuckets(): Promise<{
         enabledCount: number;
         variantCount: number;
     }> {
@@ -363,6 +363,30 @@ export class ClientMetricsStoreV2 implements IClientMetricsStoreV2 {
         };
     }
 
+    async countPreviousDayMetricsBuckets(): Promise<{
+        enabledCount: number;
+        variantCount: number;
+    }> {
+        const enabledCountQuery = this.db(DAILY_TABLE)
+            .whereRaw("date >= CURRENT_DATE - INTERVAL '1 day'")
+            .andWhereRaw('date < CURRENT_DATE')
+            .count()
+            .first();
+        const variantCountQuery = this.db(DAILY_TABLE_VARIANTS)
+            .whereRaw("date >= CURRENT_DATE - INTERVAL '1 day'")
+            .andWhereRaw('date < CURRENT_DATE')
+            .count()
+            .first();
+        const [enabledCount, variantCount] = await Promise.all([
+            enabledCountQuery,
+            variantCountQuery,
+        ]);
+        return {
+            enabledCount: Number(enabledCount?.count || 0),
+            variantCount: Number(variantCount?.count || 0),
+        };
+    }
+
     // aggregates all hourly metrics from a previous day into daily metrics
     async aggregateDailyMetrics(): Promise<void> {
         const rawQuery: string = `

diff --git a/src/lib/features/instance-stats/instance-stats-service.ts b/src/lib/features/instance-stats/instance-stats-service.ts
@@ -255,7 +255,7 @@ export class InstanceStatsService {
             this.eventStore.filteredCount({ type: FEATURES_EXPORTED }),
             this.eventStore.filteredCount({ type: FEATURES_IMPORTED }),
             this.getProductionChanges(),
-            this.clientMetricsStore.countPreviousDayMetricsBuckets(),
+            this.clientMetricsStore.countPreviousDayHourlyMetricsBuckets(),
         ]);
 
         return {

diff --git a/src/lib/services/client-metrics/metrics-service-v2.test.ts b/src/lib/services/client-metrics/metrics-service-v2.test.ts
@@ -235,20 +235,37 @@ test('get hourly client metrics for a toggle', async () => {
     ]);
 });
 
-test('aggregate previous day metrics when metrics count is below limit', async () => {
-    const enabledCount = 2;
-    const variantCount = 4;
-    let limit = 5;
+type MetricSetup = {
+    enabledCount: number;
+    variantCount: number;
+    enabledDailyCount: number;
+    variantDailyCount: number;
+    limit: number;
+};
+const setupMetricsService = ({
+    enabledCount,
+    variantCount,
+    enabledDailyCount,
+    variantDailyCount,
+    limit,
+}: MetricSetup) => {
     let aggregationCalled = false;
     let recordedWarning = '';
+
     const clientMetricsStoreV2 = {
         aggregateDailyMetrics() {
             aggregationCalled = true;
             return Promise.resolve();
         },
-        countPreviousDayMetricsBuckets() {
+        countPreviousDayHourlyMetricsBuckets() {
             return { enabledCount, variantCount };
         },
+        countPreviousDayMetricsBuckets() {
+            return {
+                enabledCount: enabledDailyCount,
+                variantCount: variantDailyCount,
+            };
+        },
     } as unknown as IClientMetricsStoreV2;
     const config = {
         flagResolver: {
@@ -273,18 +290,62 @@ test('aggregate previous day metrics when metrics count is below limit', async (
         config,
         lastSeenService,
     );
+    return {
+        service,
+        aggregationCalled: () => aggregationCalled,
+        recordedWarning: () => recordedWarning,
+    };
+};
+
+test('do not aggregate previous day metrics when metrics already calculated', async () => {
+    const { service, recordedWarning, aggregationCalled } = setupMetricsService(
+        {
+            enabledCount: 2,
+            variantCount: 4,
+            enabledDailyCount: 2,
+            variantDailyCount: 4,
+            limit: 6,
+        },
+    );
+
+    await service.aggregateDailyMetrics();
+
+    expect(recordedWarning()).toBe('');
+    expect(aggregationCalled()).toBe(false);
+});
+
+test('do not aggregate previous day metrics when metrics count is below limit', async () => {
+    const { service, recordedWarning, aggregationCalled } = setupMetricsService(
+        {
+            enabledCount: 2,
+            variantCount: 4,
+            enabledDailyCount: 0,
+            variantDailyCount: 0,
+            limit: 5,
+        },
+    );
 
     await service.aggregateDailyMetrics();
 
-    expect(recordedWarning).toBe(
+    expect(recordedWarning()).toBe(
         'Skipping previous day metrics aggregation. Too many results. Expected max value: 5, Actual value: 6',
     );
-    expect(aggregationCalled).toBe(false);
+    expect(aggregationCalled()).toBe(false);
+});
+
+test('aggregate previous day metrics', async () => {
+    const { service, recordedWarning, aggregationCalled } = setupMetricsService(
+        {
+            enabledCount: 2,
+            variantCount: 4,
+            enabledDailyCount: 0,
+            variantDailyCount: 0,
+            limit: 6,
+        },
+    );
 
-    recordedWarning = '';
-    limit = 6;
     await service.aggregateDailyMetrics();
 
-    expect(recordedWarning).toBe('');
-    expect(aggregationCalled).toBe(true);
+    expect(recordedWarning()).toBe('');
+    expect(aggregationCalled()).toBe(true);
 });
diff --git a/src/lib/services/client-metrics/metrics-service-v2.ts b/src/lib/services/client-metrics/metrics-service-v2.ts
@@ -61,7 +61,15 @@ export default class ClientMetricsServiceV2 {
 
     async aggregateDailyMetrics() {
         if (this.flagResolver.isEnabled('extendedUsageMetrics')) {
-            const { enabledCount, variantCount } =
+            const {
+                enabledCount: hourlyEnabledCount,
+                variantCount: hourlyVariantCount,
+            } =
+                await this.clientMetricsStoreV2.countPreviousDayHourlyMetricsBuckets();
+            const {
+                enabledCount: dailyEnabledCount,
+                variantCount: dailyVariantCount,
+            } =
                 await this.clientMetricsStoreV2.countPreviousDayMetricsBuckets();
             const { payload } = this.flagResolver.getVariant(
                 'extendedUsageMetrics',
@@ -72,15 +80,21 @@ export default class ClientMetricsServiceV2 {
                     ? parseInt(payload?.value)
                     : 3600000;
 
-            const totalCount = enabledCount + variantCount;
+            const totalHourlyCount = hourlyEnabledCount + hourlyVariantCount;
+            const totalDailyCount = dailyEnabledCount + dailyVariantCount;
+            const previousDayDailyCountCalculated =
+                totalDailyCount > totalHourlyCount / 24; // heuristic
 
-            if (totalCount <= limit) {
-                await this.clientMetricsStoreV2.aggregateDailyMetrics();
-            } else {
+            if (previousDayDailyCountCalculated) {
+                return;
+            }
+            if (totalHourlyCount > limit) {
                 this.logger.warn(
-                    `Skipping previous day metrics aggregation. Too many results. Expected max value: ${limit}, Actual value: ${totalCount}`,
+                    `Skipping previous day metrics aggregation. Too many results. Expected max value: ${limit}, Actual value: ${totalHourlyCount}`,
                 );
+                return;
             }
+            await this.clientMetricsStoreV2.aggregateDailyMetrics();
         }
     }
 

diff --git a/src/lib/types/stores/client-metrics-store-v2.ts b/src/lib/types/stores/client-metrics-store-v2.ts
@@ -39,6 +39,10 @@ export interface IClientMetricsStoreV2
     ): Promise<string[]>;
     clearMetrics(hoursAgo: number): Promise<void>;
     clearDailyMetrics(daysAgo: number): Promise<void>;
+    countPreviousDayHourlyMetricsBuckets(): Promise<{
+        enabledCount: number;
+        variantCount: number;
+    }>;
     countPreviousDayMetricsBuckets(): Promise<{
         enabledCount: number;
         variantCount: number;

diff --git a/src/test/fixtures/fake-client-metrics-store-v2.ts b/src/test/fixtures/fake-client-metrics-store-v2.ts
@@ -29,6 +29,12 @@ export default class FakeClientMetricsStoreV2
     clearDailyMetrics(daysBack: number): Promise<void> {
         return Promise.resolve();
     }
+    countPreviousDayHourlyMetricsBuckets(): Promise<{
+        enabledCount: number;
+        variantCount: number;
+    }> {
+        return Promise.resolve({ enabledCount: 0, variantCount: 0 });
+    }
     countPreviousDayMetricsBuckets(): Promise<{
         enabledCount: number;
         variantCount: number;