From 106e1d137672889cfa85a4117fb463e920991930 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gast=C3=B3n=20Fournier?= Date: Mon, 19 Aug 2024 22:17:52 +0200 Subject: [PATCH] fix: last seen metrics exceeding table limits (#7923) ## About the changes When storing last seen metrics we no longer validate at insert time that the feature exists. Instead, there's a job cleaning up on a regular interval. Metrics for features with more than 255 characters, makes the whole batch to fail, resulting in metrics being lost. This PR helps mitigate the issue while also logs long name feature names --- .../client-metrics/metrics-service-v2.test.ts | 1 - .../last-seen/createLastSeenService.ts | 14 +------ .../metrics/last-seen/last-seen-service.ts | 37 +++++++++---------- .../metrics/last-seen/last-seen-store.ts | 1 - .../tests/last-seen-service.e2e.test.ts | 31 ++++++++++++++++ .../last-seen/tests/last-seen-service.test.ts | 1 - 6 files changed, 50 insertions(+), 35 deletions(-) diff --git a/src/lib/features/metrics/client-metrics/metrics-service-v2.test.ts b/src/lib/features/metrics/client-metrics/metrics-service-v2.test.ts index 127f23aedb..ac5102eb62 100644 --- a/src/lib/features/metrics/client-metrics/metrics-service-v2.test.ts +++ b/src/lib/features/metrics/client-metrics/metrics-service-v2.test.ts @@ -31,7 +31,6 @@ function initClientMetrics(flagEnabled = true) { const lastSeenService = new LastSeenService( { lastSeenStore: stores.lastSeenStore, - featureToggleStore: stores.featureToggleStore, }, config, ); diff --git a/src/lib/features/metrics/last-seen/createLastSeenService.ts b/src/lib/features/metrics/last-seen/createLastSeenService.ts index 095992ad4c..d2510b81eb 100644 --- a/src/lib/features/metrics/last-seen/createLastSeenService.ts +++ b/src/lib/features/metrics/last-seen/createLastSeenService.ts @@ -1,5 +1,3 @@ -import FakeFeatureToggleStore from '../../feature-toggle/fakes/fake-feature-toggle-store'; -import FeatureToggleStore from '../../feature-toggle/feature-toggle-store'; import type { Db, IUnleashConfig } from '../../../server-impl'; import { FakeLastSeenStore } from './fake-last-seen-store'; import { LastSeenService } from './last-seen-service'; @@ -15,21 +13,13 @@ export const createLastSeenService = ( config.getLogger, ); - const featureToggleStore = new FeatureToggleStore( - db, - config.eventBus, - config.getLogger, - config.flagResolver, - ); - - return new LastSeenService({ lastSeenStore, featureToggleStore }, config); + return new LastSeenService({ lastSeenStore }, config); }; export const createFakeLastSeenService = ( config: IUnleashConfig, ): LastSeenService => { const lastSeenStore = new FakeLastSeenStore(); - const featureToggleStore = new FakeFeatureToggleStore(); - return new LastSeenService({ lastSeenStore, featureToggleStore }, config); + return new LastSeenService({ lastSeenStore }, config); }; diff --git a/src/lib/features/metrics/last-seen/last-seen-service.ts b/src/lib/features/metrics/last-seen/last-seen-service.ts index e657c4213a..ba76e28c5e 100644 --- a/src/lib/features/metrics/last-seen/last-seen-service.ts +++ b/src/lib/features/metrics/last-seen/last-seen-service.ts @@ -2,11 +2,7 @@ import type { Logger } from '../../../logger'; import type { IUnleashConfig } from '../../../server-impl'; import type { IClientMetricsEnv } from '../client-metrics/client-metrics-store-v2-type'; import type { ILastSeenStore } from './types/last-seen-store-type'; -import type { - IFeatureToggleStore, - IFlagResolver, - IUnleashStores, -} from '../../../types'; +import type { IUnleashStores } from '../../../types'; export type LastSeenInput = { featureName: string; @@ -20,36 +16,37 @@ export class LastSeenService { private lastSeenStore: ILastSeenStore; - private featureToggleStore: IFeatureToggleStore; - - private config: IUnleashConfig; - - private flagResolver: IFlagResolver; - constructor( - { - featureToggleStore, - lastSeenStore, - }: Pick, + { lastSeenStore }: Pick, config: IUnleashConfig, ) { this.lastSeenStore = lastSeenStore; - this.featureToggleStore = featureToggleStore; this.logger = config.getLogger( '/services/client-metrics/last-seen-service.ts', ); - this.flagResolver = config.flagResolver; - this.config = config; } async store(): Promise { const count = this.lastSeenToggles.size; if (count > 0) { - const lastSeenToggles = Array.from(this.lastSeenToggles.values()); - this.lastSeenToggles = new Map(); + const lastSeenToggles = Array.from( + this.lastSeenToggles.values(), + ).filter((lastSeen) => lastSeen.featureName.length <= 255); + if (lastSeenToggles.length < this.lastSeenToggles.size) { + this.logger.warn( + `Toggles with long names ${JSON.stringify( + Array.from(this.lastSeenToggles.values()) + .filter( + (lastSeen) => lastSeen.featureName.length > 255, + ) + .map((lastSeen) => lastSeen.featureName), + )}`, + ); + } this.logger.debug( `Updating last seen for ${lastSeenToggles.length} toggles`, ); + this.lastSeenToggles = new Map(); await this.lastSeenStore.setLastSeen(lastSeenToggles); } diff --git a/src/lib/features/metrics/last-seen/last-seen-store.ts b/src/lib/features/metrics/last-seen/last-seen-store.ts index 3905cc06da..165120281f 100644 --- a/src/lib/features/metrics/last-seen/last-seen-store.ts +++ b/src/lib/features/metrics/last-seen/last-seen-store.ts @@ -48,7 +48,6 @@ export default class LastSeenStore implements ILastSeenStore { async setLastSeen(data: LastSeenInput[]): Promise { try { const inserts = prepareLastSeenInput(data); - const batchSize = 500; for (let i = 0; i < inserts.length; i += batchSize) { diff --git a/src/lib/features/metrics/last-seen/tests/last-seen-service.e2e.test.ts b/src/lib/features/metrics/last-seen/tests/last-seen-service.e2e.test.ts index 366055374d..61c87f935f 100644 --- a/src/lib/features/metrics/last-seen/tests/last-seen-service.e2e.test.ts +++ b/src/lib/features/metrics/last-seen/tests/last-seen-service.e2e.test.ts @@ -133,3 +133,34 @@ test('should clean unknown feature flag environments from last seen store', asyn expect(stored.rows.length).toBe(2); }); + +test('should not fail with feature names longer than 255 chars', async () => { + const { lastSeenService } = app.services; + + const longFeatureNames = [ + { name: 'a'.repeat(254), environment: 'default' }, + { name: 'b'.repeat(255), environment: 'default' }, + { name: 'c'.repeat(256), environment: 'default' }, // this one should be filtered out + ]; + + const inserts = [...longFeatureNames].map((feature) => { + return { + featureName: feature.name, + environment: feature.environment, + yes: 1, + no: 0, + appName: 'test', + timestamp: new Date(), + }; + }); + + lastSeenService.updateLastSeen(inserts); + await lastSeenService.store(); + + // We have no method to get these from the last seen service or any other service or store + const stored = await db.rawDatabase.raw( + 'SELECT * FROM last_seen_at_metrics;', + ); + + expect(stored.rows.length).toBe(2); +}); diff --git a/src/lib/features/metrics/last-seen/tests/last-seen-service.test.ts b/src/lib/features/metrics/last-seen/tests/last-seen-service.test.ts index d343f61422..b14fe8145c 100644 --- a/src/lib/features/metrics/last-seen/tests/last-seen-service.test.ts +++ b/src/lib/features/metrics/last-seen/tests/last-seen-service.test.ts @@ -23,7 +23,6 @@ function initLastSeenService(flagEnabled = true) { const lastSeenService = new LastSeenService( { lastSeenStore: stores.lastSeenStore, - featureToggleStore: stores.featureToggleStore, }, config, );