From 632f3a04cb762dac211004945a3629b3b97d5d72 Mon Sep 17 00:00:00 2001 From: Mateusz Kwasniewski Date: Thu, 19 Jun 2025 14:46:36 +0200 Subject: [PATCH] feat: validate impact metrics (#10181) --- .../client-metrics/metrics-service-v2.ts | 15 +++++++++- .../metrics/impact/impact-metrics.e2e.test.ts | 23 +++++++++++++-- src/lib/features/metrics/instance/metrics.ts | 4 +-- src/lib/features/metrics/shared/schema.ts | 29 +++++++++++++++++++ src/lib/routes/client-api/index.ts | 9 +----- 5 files changed, 66 insertions(+), 14 deletions(-) diff --git a/src/lib/features/metrics/client-metrics/metrics-service-v2.ts b/src/lib/features/metrics/client-metrics/metrics-service-v2.ts index 00271320a9..d2e2da83f2 100644 --- a/src/lib/features/metrics/client-metrics/metrics-service-v2.ts +++ b/src/lib/features/metrics/client-metrics/metrics-service-v2.ts @@ -6,7 +6,7 @@ import type { IClientMetricsEnv, IClientMetricsStoreV2, } from './client-metrics-store-v2-type.js'; -import { clientMetricsSchema } from '../shared/schema.js'; +import { clientMetricsSchema, impactMetricsSchema } from '../shared/schema.js'; import { compareAsc, secondsToMilliseconds } from 'date-fns'; import { CLIENT_METRICS, @@ -30,6 +30,11 @@ import { MAX_UNKNOWN_FLAGS, type UnknownFlagsService, } from '../unknown-flags/unknown-flags-service.js'; +import { + type Metric, + MetricsTranslator, +} from '../impact/metrics-translator.js'; +import { impactRegister } from '../impact/impact-register.js'; export default class ClientMetricsServiceV2 { private config: IUnleashConfig; @@ -46,6 +51,8 @@ export default class ClientMetricsServiceV2 { private logger: Logger; + private impactMetricsTranslator: MetricsTranslator; + private cachedFeatureNames: () => Promise; constructor( @@ -69,6 +76,7 @@ export default class ClientMetricsServiceV2 { maxAge: secondsToMilliseconds(10), }, ); + this.impactMetricsTranslator = new MetricsTranslator(impactRegister); } async clearMetrics(hoursAgo: number) { @@ -187,6 +195,11 @@ export default class ClientMetricsServiceV2 { this.lastSeenService.updateLastSeen(metrics); } + async registerImpactMetrics(impactMetrics: Metric[]) { + const value = await impactMetricsSchema.validateAsync(impactMetrics); + this.impactMetricsTranslator.translateMetrics(value); + } + async registerClientMetrics( data: ClientMetricsSchema, clientIp: string, diff --git a/src/lib/features/metrics/impact/impact-metrics.e2e.test.ts b/src/lib/features/metrics/impact/impact-metrics.e2e.test.ts index fcea2ff321..a1eb68ffb3 100644 --- a/src/lib/features/metrics/impact/impact-metrics.e2e.test.ts +++ b/src/lib/features/metrics/impact/impact-metrics.e2e.test.ts @@ -11,7 +11,7 @@ import type { Metric } from './metrics-translator.js'; let app: IUnleashTest; let db: ITestDb; -const sendImpactMetrics = async (impactMetrics: Metric[]) => +const sendImpactMetrics = async (impactMetrics: Metric[], status = 202) => app.request .post('/api/client/metrics') .send({ @@ -24,7 +24,7 @@ const sendImpactMetrics = async (impactMetrics: Metric[]) => }, impactMetrics, }) - .expect(202); + .expect(status); beforeAll(async () => { db = await dbInit('impact_metrics', getLogger); @@ -71,6 +71,25 @@ test('should store impact metrics in memory and be able to retrieve them', async }, ]); + await sendImpactMetrics([]); + // missing help + await sendImpactMetrics( + [ + // @ts-expect-error + { + name: 'labeled_counter', + type: 'counter', + samples: [ + { + labels: { foo: 'bar' }, + value: 10, + }, + ], + }, + ], + 400, + ); + const response = await app.request .get('/internal-backstage/impact/metrics') .expect('Content-Type', /text/) diff --git a/src/lib/features/metrics/instance/metrics.ts b/src/lib/features/metrics/instance/metrics.ts index 577eed59ac..8eabc96a82 100644 --- a/src/lib/features/metrics/instance/metrics.ts +++ b/src/lib/features/metrics/instance/metrics.ts @@ -58,7 +58,6 @@ export default class ClientMetricsController extends Controller { | 'customMetricsService' >, config: IUnleashConfig, - metricsTranslator: MetricsTranslator, ) { super(config); const { getLogger } = config; @@ -69,7 +68,6 @@ export default class ClientMetricsController extends Controller { this.metricsV2 = clientMetricsServiceV2; this.customMetricsService = customMetricsService; this.flagResolver = config.flagResolver; - this.metricsTranslator = metricsTranslator; this.route({ method: 'post', @@ -171,7 +169,7 @@ export default class ClientMetricsController extends Controller { this.flagResolver.isEnabled('impactMetrics') && impactMetrics ) { - this.metricsTranslator.translateMetrics(impactMetrics); + await this.metricsV2.registerImpactMetrics(impactMetrics); } res.getHeaderNames().forEach((header) => diff --git a/src/lib/features/metrics/shared/schema.ts b/src/lib/features/metrics/shared/schema.ts index 683e00a8b9..fa5d1ed586 100644 --- a/src/lib/features/metrics/shared/schema.ts +++ b/src/lib/features/metrics/shared/schema.ts @@ -85,6 +85,35 @@ export const customMetricsSchema = joi metrics: joi.array().items(customMetricSchema).required(), }); +export const metricSampleSchema = joi + .object() + .options({ stripUnknown: true }) + .keys({ + value: joi.number().required(), + labels: joi + .object() + .pattern( + joi.string(), + joi.alternatives().try(joi.string(), joi.number()), + ) + .optional(), + }); + +export const impactMetricSchema = joi + .object() + .options({ stripUnknown: true }) + .keys({ + name: joi.string().required(), + help: joi.string().required(), + type: joi.string().required(), + samples: joi.array().items(metricSampleSchema).required(), + }); + +export const impactMetricsSchema = joi + .array() + .items(impactMetricSchema) + .empty(); + export const batchMetricsSchema = joi .object() .options({ stripUnknown: true }) diff --git a/src/lib/routes/client-api/index.ts b/src/lib/routes/client-api/index.ts index cb7cf3cd0c..b58dc6824f 100644 --- a/src/lib/routes/client-api/index.ts +++ b/src/lib/routes/client-api/index.ts @@ -4,20 +4,13 @@ import MetricsController from '../../features/metrics/instance/metrics.js'; import RegisterController from '../../features/metrics/instance/register.js'; import type { IUnleashConfig } from '../../types/index.js'; import type { IUnleashServices } from '../../services/index.js'; -import { impactRegister } from '../../features/metrics/impact/impact-register.js'; -import { MetricsTranslator } from '../../features/metrics/impact/metrics-translator.js'; export default class ClientApi extends Controller { constructor(config: IUnleashConfig, services: IUnleashServices) { super(config); - const metricsTranslator = new MetricsTranslator(impactRegister); - this.use('/features', new FeatureController(services, config).router); - this.use( - '/metrics', - new MetricsController(services, config, metricsTranslator).router, - ); + this.use('/metrics', new MetricsController(services, config).router); this.use('/register', new RegisterController(services, config).router); } }