Mirrors/unleash.unleash
1
0
Fork 0
mirror of https://github.com/Unleash/unleash.git synced 2025-07-31 13:47:02 +02:00
Code Issues Projects Releases Wiki Activity

fix: don't overwrite seenClients in instance-service; merge if same client appears again (#10357)

Browse Source 
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.
This commit is contained in:
Thomas Heartman 2025-07-16 14:02:25 +02:00 committed by GitHub
parent 8455134688
commit 642b209b9d
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
6 changed files with 160 additions and 26 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

126
src/lib/features/metrics/instance/instance-service.test.ts
View File

@ -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,
});
});
});

32
src/lib/features/metrics/instance/instance-service.ts
View File

@ -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<IClientApp, 'instanceId'>,
data: Pick<
ClientMetricsSchema,
'appName' | 'instanceId' | 'environment'
>,
clientIp: string,
): Promise<void> {
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) {

15
src/lib/features/metrics/instance/metrics.ts
View File

@ -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<void> {
async registerMetrics(
req: IAuthRequest<void, void, ClientMetricsSchema>,
res: Response,
): Promise<void> {
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) =>

11
src/lib/openapi/spec/create-application-schema.ts
View File

@ -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 `<identifier>:<version>`',
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',

1
src/lib/types/model.ts
View File

@ -555,6 +555,7 @@ export interface IClientApp {
yggdrasilVersion?: string;
specVersion?: string;
sdkType?: 'frontend' | 'backend' | null;
sdkVersion?: string;
}
export interface IAppFeature {

1
src/test/e2e/api/admin/applications.e2e.test.ts
View File

@ -85,6 +85,7 @@ beforeEach(async () => {
db.stores.featureToggleStore.deleteAll(),
db.stores.clientApplicationsStore.deleteAll(),
]);
app.services.clientInstanceService.seenClients = {};
});
afterAll(async () => {