From 642b209b9d868af31f8a6d2efda30983c57c88b2 Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Wed, 16 Jul 2025 14:02:25 +0200 Subject: [PATCH] fix: don't overwrite `seenClients` in instance-service; merge if same client appears again (#10357) Fixes a bug where `registerInstance` and `register{Frontend|Backend}Client` would overwrite each other's data in the instance service, leading to the bulk update being made with partial data, often missing SDK version. There's a different issue in the actual store that causes sdk version and type to be overwritten when it's updated (because we don't use `setLastSeen` anymore), but I'll handle that in a different PR. This PR adds tests for the changes I've made. Additionally, I've made these semi-related bonus changes: - In registerInstance, don't expect a partial `IClientApp`. We used to validate that it was actual a metrics object instead. Instead, update the signature to expect the actual properties we need from the cilent metrics schema and set a default for instanceId the way Joi did. - In `metrics.ts`, use the `ClientMetricsSchema` type in the function signature, so that the request body is correctly typed in the function (instead of being `any`). - Delete two unused properties from the`createApplicationSchema`. They would get ignored and were never used as far as I can tell. (`appName` is taken from the URL, and applications don't store `sdkVersion` information). - Add `sdkVersion` to `IClientApp` because it's used in instance service. I've been very confused about all the weird type shenanigans we do in the instance service (expecting `IClientApp`, then validating with a different Joi schema etc). I think this makes it a little bit better and updates the bits I'm touching, but I'm happy to take input if you disagree. --- .../metrics/instance/instance-service.test.ts | 126 ++++++++++++++++++ .../metrics/instance/instance-service.ts | 32 +++-- src/lib/features/metrics/instance/metrics.ts | 15 ++- .../openapi/spec/create-application-schema.ts | 11 -- src/lib/types/model.ts | 1 + .../e2e/api/admin/applications.e2e.test.ts | 1 + 6 files changed, 160 insertions(+), 26 deletions(-) diff --git a/src/lib/features/metrics/instance/instance-service.test.ts b/src/lib/features/metrics/instance/instance-service.test.ts index 3268f9b23f..8292783e45 100644 --- a/src/lib/features/metrics/instance/instance-service.test.ts +++ b/src/lib/features/metrics/instance/instance-service.test.ts @@ -263,3 +263,129 @@ test('filter out private projects from overview', async () => { featureCount: 0, }); }); + +test('`registerInstance` sets `instanceId` to `default` if it is not provided', async () => { + const instanceService = new ClientInstanceService( + {} as any, + config, + {} as any, + ); + + await instanceService.registerInstance( + { + appName: 'appName', + environment: '', + }, + '::1', + ); + + expect(instanceService.seenClients.appName_default).toMatchObject({ + appName: 'appName', + instanceId: 'default', + }); +}); + +describe('upserting into `seenClients`', () => { + test('registerInstance merges its data', async () => { + const instanceService = new ClientInstanceService( + {} as any, + config, + {} as any, + ); + + const client = { + appName: 'appName', + instanceId: 'instanceId', + }; + + const key = instanceService.clientKey(client); + + instanceService.seenClients = { + [key]: { ...client, sdkVersion: 'my-sdk' }, + }; + + await instanceService.registerInstance( + { + ...client, + environment: 'blue', + }, + '::1', + ); + + expect(instanceService.seenClients[key]).toMatchObject({ + appName: 'appName', + instanceId: 'instanceId', + environment: 'blue', + sdkVersion: 'my-sdk', + }); + }); + test('registerBackendClient merges its data', async () => { + const instanceService = new ClientInstanceService( + {} as any, + config, + {} as any, + ); + + const client = { + appName: 'appName', + instanceId: 'instanceId', + }; + + const key = instanceService.clientKey(client); + + instanceService.seenClients = { + [key]: { ...client, environment: 'blue' }, + }; + + await instanceService.registerBackendClient( + { + ...client, + sdkVersion: 'my-sdk', + started: new Date(), + interval: 5, + }, + '::1', + ); + + expect(instanceService.seenClients[key]).toMatchObject({ + appName: 'appName', + instanceId: 'instanceId', + environment: 'blue', + sdkVersion: 'my-sdk', + }); + }); + test('registerFrontendClient merges its data', async () => { + const instanceService = new ClientInstanceService( + {} as any, + config, + {} as any, + ); + + const client = { + appName: 'appName', + instanceId: 'instanceId', + }; + + const key = instanceService.clientKey(client); + + instanceService.seenClients = { + [key]: { ...client, metricsCount: 10 }, + }; + + instanceService.registerFrontendClient({ + ...client, + sdkVersion: 'my-sdk', + sdkType: 'frontend', + environment: 'black', + }); + + expect(instanceService.seenClients[key]).toMatchObject({ + appName: 'appName', + instanceId: 'instanceId', + sdkVersion: 'my-sdk', + sdkType: 'frontend', + environment: 'black', + metricsCount: 10, + }); + }); +}); diff --git a/src/lib/features/metrics/instance/instance-service.ts b/src/lib/features/metrics/instance/instance-service.ts index 30773e59f7..961a014426 100644 --- a/src/lib/features/metrics/instance/instance-service.ts +++ b/src/lib/features/metrics/instance/instance-service.ts @@ -21,8 +21,6 @@ import type { import { clientRegisterSchema } from '../shared/schema.js'; import type { IClientMetricsStoreV2 } from '../client-metrics/client-metrics-store-v2-type.js'; -import { clientMetricsSchema } from '../shared/schema.js'; -import type { PartialSome } from '../../../types/partial.js'; import type { IPrivateProjectChecker } from '../../private-project/privateProjectCheckerType.js'; import { type ApplicationCreatedEvent, @@ -35,6 +33,7 @@ import { findOutdatedSDKs, isOutdatedSdk } from './findOutdatedSdks.js'; import type { OutdatedSdksSchema } from '../../../openapi/spec/outdated-sdks-schema.js'; import { CLIENT_REGISTERED } from '../../../metric-events.js'; import { NotFoundError } from '../../../error/index.js'; +import type { ClientMetricsSchema, PartialSome } from '../../../server-impl.js'; export default class ClientInstanceService { apps = {}; @@ -99,24 +98,33 @@ export default class ClientInstanceService { ); } + private updateSeenClient = (data: IClientApp) => { + const current = this.seenClients[this.clientKey(data)]; + this.seenClients[this.clientKey(data)] = { + ...current, + ...data, + }; + }; + public async registerInstance( - data: PartialSome, + data: Pick< + ClientMetricsSchema, + 'appName' | 'instanceId' | 'environment' + >, clientIp: string, ): Promise { - const value = await clientMetricsSchema.validateAsync(data); - - this.seenClients[this.clientKey(value)] = { - appName: value.appName, - instanceId: value.instanceId, - environment: value.environment, + this.updateSeenClient({ + appName: data.appName, + instanceId: data.instanceId ?? 'default', + environment: data.environment, clientIp: clientIp, - }; + }); } public registerFrontendClient(data: IFrontendClientApp): void { data.createdBy = SYSTEM_USER.username!; - this.seenClients[this.clientKey(data)] = data; + this.updateSeenClient(data); } public async registerBackendClient( @@ -127,7 +135,7 @@ export default class ClientInstanceService { value.clientIp = clientIp; value.createdBy = SYSTEM_USER.username!; value.sdkType = 'backend'; - this.seenClients[this.clientKey(value)] = value; + this.updateSeenClient(value); this.eventBus.emit(CLIENT_REGISTERED, value); if (value.sdkVersion && value.sdkVersion.indexOf(':') > -1) { diff --git a/src/lib/features/metrics/instance/metrics.ts b/src/lib/features/metrics/instance/metrics.ts index 66a057f41b..29507bbfd9 100644 --- a/src/lib/features/metrics/instance/metrics.ts +++ b/src/lib/features/metrics/instance/metrics.ts @@ -27,7 +27,11 @@ import { CLIENT_METRICS } from '../../../events/index.js'; import type { CustomMetricsSchema } from '../../../openapi/spec/custom-metrics-schema.js'; import type { StoredCustomMetric } from '../custom/custom-metrics-store.js'; import type { CustomMetricsService } from '../custom/custom-metrics-service.js'; -import type { MetricsTranslator } from '../impact/metrics-translator.js'; +import type { + Metric, + MetricsTranslator, +} from '../impact/metrics-translator.js'; +import type { ClientMetricsSchema } from '../../../server-impl.js'; export default class ClientMetricsController extends Controller { logger: Logger; @@ -147,7 +151,10 @@ export default class ClientMetricsController extends Controller { // Note: Custom metrics GET endpoints are now handled by the admin API } - async registerMetrics(req: IAuthRequest, res: Response): Promise { + async registerMetrics( + req: IAuthRequest, + res: Response, + ): Promise { if (this.config.flagResolver.isEnabled('disableMetrics')) { res.status(204).end(); } else { @@ -169,7 +176,9 @@ export default class ClientMetricsController extends Controller { this.flagResolver.isEnabled('impactMetrics') && impactMetrics ) { - await this.metricsV2.registerImpactMetrics(impactMetrics); + await this.metricsV2.registerImpactMetrics( + impactMetrics as Metric[], + ); } res.getHeaderNames().forEach((header) => diff --git a/src/lib/openapi/spec/create-application-schema.ts b/src/lib/openapi/spec/create-application-schema.ts index 3b93daa528..4d840b8464 100644 --- a/src/lib/openapi/spec/create-application-schema.ts +++ b/src/lib/openapi/spec/create-application-schema.ts @@ -5,17 +5,6 @@ export const createApplicationSchema = { type: 'object', description: 'Reported application information from Unleash SDKs', properties: { - appName: { - description: 'Name of the application', - type: 'string', - example: 'accounting', - }, - sdkVersion: { - description: - 'Which SDK and version the application reporting uses. Typically represented as `:`', - type: 'string', - example: 'unleash-client-java:8.0.0', - }, strategies: { description: 'Which [strategies](https://docs.getunleash.io/topics/the-anatomy-of-unleash#activation-strategies) the application has loaded. Useful when trying to figure out if your [custom strategy](https://docs.getunleash.io/reference/custom-activation-strategies) has been loaded in the SDK', diff --git a/src/lib/types/model.ts b/src/lib/types/model.ts index 3cb9906280..50884b5bda 100644 --- a/src/lib/types/model.ts +++ b/src/lib/types/model.ts @@ -555,6 +555,7 @@ export interface IClientApp { yggdrasilVersion?: string; specVersion?: string; sdkType?: 'frontend' | 'backend' | null; + sdkVersion?: string; } export interface IAppFeature { diff --git a/src/test/e2e/api/admin/applications.e2e.test.ts b/src/test/e2e/api/admin/applications.e2e.test.ts index 12a70452a2..38e0303ea6 100644 --- a/src/test/e2e/api/admin/applications.e2e.test.ts +++ b/src/test/e2e/api/admin/applications.e2e.test.ts @@ -85,6 +85,7 @@ beforeEach(async () => { db.stores.featureToggleStore.deleteAll(), db.stores.clientApplicationsStore.deleteAll(), ]); + app.services.clientInstanceService.seenClients = {}; }); afterAll(async () => {