From 8d03ce340d60704a08f9b374c6c897689d058092 Mon Sep 17 00:00:00 2001 From: Mateusz Kwasniewski Date: Mon, 22 Sep 2025 17:14:07 +0200 Subject: [PATCH] feat: histogram impact metric ingestion (#10674) --- .../client-metrics/metrics-service-v2.ts | 2 +- .../metrics/impact/batch-histogram.test.ts | 56 ++++++++++--- .../metrics/impact/batch-histogram.ts | 15 ++-- .../metrics/impact/impact-metrics.e2e.test.ts | 75 +++++++++++++++++- .../metrics/impact/metrics-translator.test.ts | 42 ++++++++++ .../metrics/impact/metrics-translator.ts | 78 ++++++++++++++++--- src/lib/features/metrics/shared/schema.ts | 34 +++++++- 7 files changed, 272 insertions(+), 30 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 996a3985f6..20f754ea5b 100644 --- a/src/lib/features/metrics/client-metrics/metrics-service-v2.ts +++ b/src/lib/features/metrics/client-metrics/metrics-service-v2.ts @@ -259,7 +259,7 @@ export default class ClientMetricsServiceV2 { this.impactMetricsTranslator.translateMetrics(value); } catch (e) { // impact metrics should not affect other metrics on failure - this.logger.warn(e); + this.logger.warn('Impact metrics registration failed:', e); } } diff --git a/src/lib/features/metrics/impact/batch-histogram.test.ts b/src/lib/features/metrics/impact/batch-histogram.test.ts index 6e7e0e7019..f30eeaff2f 100644 --- a/src/lib/features/metrics/impact/batch-histogram.test.ts +++ b/src/lib/features/metrics/impact/batch-histogram.test.ts @@ -117,9 +117,9 @@ describe('BatchHistogram', () => { expect(metrics).toMatch(/test_histogram_count{label1="value1"} 8/); }); - test('should record different labels separately', async () => { + test('should record different labels separately and handle special characters', async () => { histogram.recordBatch( - { service: 'api', app: 'my_app' }, + { service: 'api', url: 'http://example.com:8080/api' }, { count: 3, sum: 1.5, @@ -131,7 +131,7 @@ describe('BatchHistogram', () => { ); histogram.recordBatch( - { service: 'web', app: 'my_app' }, + { service: 'web', url: 'https://app.example.com/dashboard' }, { count: 2, sum: 3.0, @@ -145,29 +145,30 @@ describe('BatchHistogram', () => { const metrics = await registry.metrics(); expect(metrics).toMatch( - /test_histogram_bucket{app="my_app",service="api",le="1"} 2/, + /test_histogram_bucket{service="api",url="http:\/\/example\.com:8080\/api",le="1"} 2/, ); expect(metrics).toMatch( - /test_histogram_bucket{app="my_app",service="api",le="\+Inf"} 3/, + /test_histogram_bucket{service="api",url="http:\/\/example\.com:8080\/api",le="\+Inf"} 3/, ); expect(metrics).toMatch( - /test_histogram_sum{app="my_app",service="api"} 1\.5/, + /test_histogram_sum{service="api",url="http:\/\/example\.com:8080\/api"} 1\.5/, ); expect(metrics).toMatch( - /test_histogram_count{app="my_app",service="api"} 3/, + /test_histogram_count{service="api",url="http:\/\/example\.com:8080\/api"} 3/, ); + // Web service metrics (with HTTPS URL) expect(metrics).toMatch( - /test_histogram_bucket{app="my_app",service="web",le="1"} 1/, + /test_histogram_bucket{service="web",url="https:\/\/app\.example\.com\/dashboard",le="1"} 1/, ); expect(metrics).toMatch( - /test_histogram_bucket{app="my_app",service="web",le="\+Inf"} 2/, + /test_histogram_bucket{service="web",url="https:\/\/app\.example\.com\/dashboard",le="\+Inf"} 2/, ); expect(metrics).toMatch( - /test_histogram_sum{app="my_app",service="web"} 3/, + /test_histogram_sum{service="web",url="https:\/\/app\.example\.com\/dashboard"} 3/, ); expect(metrics).toMatch( - /test_histogram_count{app="my_app",service="web"} 2/, + /test_histogram_count{service="web",url="https:\/\/app\.example\.com\/dashboard"} 2/, ); }); @@ -193,4 +194,37 @@ describe('BatchHistogram', () => { expect(metrics).toMatch(/test_histogram_sum{client="sdk"} 12\.3/); expect(metrics).toMatch(/test_histogram_count{client="sdk"} 5/); }); + + test('should handle unsorted bucket input', async () => { + histogram.recordBatch( + { service: 'test' }, + { + count: 5, + sum: 7.5, + buckets: [ + { le: '+Inf', count: 5 }, // Infinity first (unsorted) + { le: 2.5, count: 4 }, // Out of order + { le: 0.5, count: 2 }, // Out of order + { le: 1, count: 3 }, // Out of order + ], + }, + ); + + const metrics = await registry.metrics(); + + expect(metrics).toMatch( + /test_histogram_bucket{service="test",le="0.5"} 2/, + ); + expect(metrics).toMatch( + /test_histogram_bucket{service="test",le="1"} 3/, + ); + expect(metrics).toMatch( + /test_histogram_bucket{service="test",le="2.5"} 4/, + ); + expect(metrics).toMatch( + /test_histogram_bucket{service="test",le="\+Inf"} 5/, + ); + expect(metrics).toMatch(/test_histogram_sum{service="test"} 7\.5/); + expect(metrics).toMatch(/test_histogram_count{service="test"} 5/); + }); }); diff --git a/src/lib/features/metrics/impact/batch-histogram.ts b/src/lib/features/metrics/impact/batch-histogram.ts index 4b604c294b..9e2df6ce7b 100644 --- a/src/lib/features/metrics/impact/batch-histogram.ts +++ b/src/lib/features/metrics/impact/batch-histogram.ts @@ -15,6 +15,7 @@ export class BatchHistogram { private name: string; private help: string; private registry: Registry; + public labelNames: string[] = []; // Store accumulated data for each label combination private store: Map< @@ -30,10 +31,12 @@ export class BatchHistogram { name: string; help: string; registry: Registry; + labelNames?: string[]; }) { this.name = config.name; this.help = config.help; this.registry = config.registry; + this.labelNames = config.labelNames || []; this.registry.registerMetric(this as any); } @@ -65,7 +68,7 @@ export class BatchHistogram { private createLabelKey(labels: Record): string { const sortedKeys = Object.keys(labels).sort(); - return sortedKeys.map((key) => `${key}:${labels[key]}`).join(','); + return JSON.stringify(sortedKeys.map((key) => [key, labels[key]])); } reset(): void { @@ -78,10 +81,12 @@ export class BatchHistogram { for (const [labelKey, data] of this.store) { const labels: Record = {}; if (labelKey) { - labelKey.split(',').forEach((pair) => { - const [key, value] = pair.split(':'); - labels[key] = value; - }); + const parsedLabels = JSON.parse(labelKey); + parsedLabels.forEach( + ([key, value]: [string, string | number]) => { + labels[key] = value; + }, + ); } for (const [le, cumulativeCount] of Array.from( 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 7b5a92a5ee..724ccb8652 100644 --- a/src/lib/features/metrics/impact/impact-metrics.e2e.test.ts +++ b/src/lib/features/metrics/impact/impact-metrics.e2e.test.ts @@ -6,12 +6,15 @@ import dbInit, { type ITestDb, } from '../../../../test/e2e/helpers/database-init.js'; import getLogger from '../../../../test/fixtures/no-logger.js'; -import type { Metric } from './metrics-translator.js'; +import type { NumericMetric, BucketMetric } from './metrics-translator.js'; let app: IUnleashTest; let db: ITestDb; -const sendImpactMetrics = async (impactMetrics: Metric[], status = 202) => +const sendImpactMetrics = async ( + impactMetrics: (NumericMetric | BucketMetric)[], + status = 202, +) => app.request .post('/api/client/metrics') .send({ @@ -27,7 +30,7 @@ const sendImpactMetrics = async (impactMetrics: Metric[], status = 202) => .expect(status); const sendBulkMetricsWithImpact = async ( - impactMetrics: Metric[], + impactMetrics: (NumericMetric | BucketMetric)[], status = 202, ) => { return app.request @@ -170,3 +173,69 @@ test('should store impact metrics sent via bulk metrics endpoint', async () => { /unleash_counter_bulk_counter{unleash_source="bulk",unleash_origin="sdk"} 15/, ); }); + +test('should store histogram metrics with batch data', async () => { + await sendImpactMetrics([ + { + name: 'response_time', + help: 'Response time histogram', + type: 'histogram', + samples: [ + { + labels: { foo: 'bar' }, + count: 10, + sum: 8.5, + buckets: [ + { le: 1, count: 7 }, + { le: '+Inf', count: 10 }, + ], + }, + ], + }, + ]); + + await sendImpactMetrics([ + { + name: 'response_time', + help: 'Response time histogram', + type: 'histogram', + samples: [ + { + labels: { foo: 'bar' }, + count: 5, + sum: 3.2, + buckets: [ + { le: 1, count: 3 }, + { le: '+Inf', count: 5 }, + ], + }, + ], + }, + ]); + + const response = await app.request + .get('/internal-backstage/impact/metrics') + .expect('Content-Type', /text/) + .expect(200); + + const metricsText = response.text; + + expect(metricsText).toContain( + '# HELP unleash_histogram_response_time Response time histogram', + ); + expect(metricsText).toContain( + '# TYPE unleash_histogram_response_time histogram', + ); + expect(metricsText).toContain( + 'unleash_histogram_response_time_bucket{unleash_foo="bar",unleash_origin="sdk",le="1"} 10', + ); + expect(metricsText).toContain( + 'unleash_histogram_response_time_bucket{unleash_foo="bar",unleash_origin="sdk",le="+Inf"} 15', + ); + expect(metricsText).toContain( + 'unleash_histogram_response_time_sum{unleash_foo="bar",unleash_origin="sdk"} 11.7', + ); + expect(metricsText).toContain( + 'unleash_histogram_response_time_count{unleash_foo="bar",unleash_origin="sdk"} 15', + ); +}); diff --git a/src/lib/features/metrics/impact/metrics-translator.test.ts b/src/lib/features/metrics/impact/metrics-translator.test.ts index 387ad143b8..ad3cb604f9 100644 --- a/src/lib/features/metrics/impact/metrics-translator.test.ts +++ b/src/lib/features/metrics/impact/metrics-translator.test.ts @@ -193,6 +193,22 @@ describe('MetricsTranslator', () => { }, ], }, + { + name: 'histogram_with_labels', + help: 'histogram with labels', + type: 'histogram' as const, + samples: [ + { + labels: { service: 'api' }, + count: 5, + sum: 2.5, + buckets: [ + { le: 1, count: 3 }, + { le: '+Inf' as const, count: 5 }, + ], + }, + ], + }, ]; const result1 = await translator.translateAndSerializeMetrics(metrics1); @@ -202,6 +218,9 @@ describe('MetricsTranslator', () => { expect(result1).toContain( 'unleash_gauge_gauge_with_labels{unleash_env="prod",unleash_origin="sdk"} 10', ); + expect(result1).toContain( + 'unleash_histogram_histogram_with_labels_count{unleash_origin="sdk",unleash_service="api"} 5', + ); const metrics2 = [ { @@ -226,6 +245,23 @@ describe('MetricsTranslator', () => { }, ], }, + { + name: 'histogram_with_labels', + help: 'histogram with labels', + type: 'histogram' as const, + buckets: [1], + samples: [ + { + labels: { service: 'api', region: 'us-east' }, // Added a new label + count: 3, + sum: 1.8, + buckets: [ + { le: 1, count: 2 }, + { le: '+Inf' as const, count: 3 }, + ], + }, + ], + }, ]; const result2 = await translator.translateAndSerializeMetrics(metrics2); @@ -236,5 +272,11 @@ describe('MetricsTranslator', () => { expect(result2).toContain( 'unleash_gauge_gauge_with_labels{unleash_env="prod",unleash_region="us-east",unleash_origin="sdk"} 20', ); + expect(result2).toContain( + 'unleash_histogram_histogram_with_labels_count{unleash_origin="sdk",unleash_region="us-east",unleash_service="api"} 3', + ); + expect(result2).not.toContain( + 'unleash_histogram_histogram_with_labels_count{unleash_origin="sdk",unleash_service="api"} 5', + ); }); }); diff --git a/src/lib/features/metrics/impact/metrics-translator.ts b/src/lib/features/metrics/impact/metrics-translator.ts index dcd577f141..1cb7764599 100644 --- a/src/lib/features/metrics/impact/metrics-translator.ts +++ b/src/lib/features/metrics/impact/metrics-translator.ts @@ -1,17 +1,34 @@ import { Counter, Gauge, type Registry } from 'prom-client'; +import { BatchHistogram } from './batch-histogram.js'; -export interface MetricSample { +export interface NumericMetricSample { labels?: Record; value: number; } -export interface Metric { +export interface BucketMetricSample { + labels?: Record; + count: number; + sum: number; + buckets: Array<{ le: number | '+Inf'; count: number }>; +} + +export interface NumericMetric { name: string; help: string; type: 'counter' | 'gauge'; - samples: MetricSample[]; + samples: NumericMetricSample[]; } +export interface BucketMetric { + name: string; + help: string; + type: 'histogram'; + samples: BucketMetricSample[]; +} + +export type Metric = NumericMetric | BucketMetric; + export class MetricsTranslator { private registry: Registry; @@ -22,13 +39,11 @@ export class MetricsTranslator { sanitizeName(name: string): string { const regex = /[^a-zA-Z0-9_]/g; - const sanitized = name.replace(regex, '_'); - - return sanitized; + return name.replace(regex, '_'); } private hasNewLabels( - existingMetric: Counter | Gauge, + existingMetric: Counter | Gauge | BatchHistogram, newLabelNames: string[], ): boolean { const existingLabelNames = (existingMetric as any).labelNames || []; @@ -50,7 +65,7 @@ export class MetricsTranslator { } private addOriginLabel( - sample: MetricSample, + sample: NumericMetricSample, ): Record { return { ...(sample.labels || {}), @@ -58,7 +73,9 @@ export class MetricsTranslator { }; } - translateMetric(metric: Metric): Counter | Gauge | null { + translateMetric( + metric: Metric, + ): Counter | Gauge | BatchHistogram | null { const sanitizedName = this.sanitizeName(metric.name); const prefixedName = `unleash_${metric.type}_${sanitizedName}`; const existingMetric = this.registry.getSingleMetric(prefixedName); @@ -142,6 +159,49 @@ export class MetricsTranslator { } return gauge; + } else if (metric.type === 'histogram') { + if (!metric.samples || metric.samples.length === 0) { + return null; + } + + let histogram: BatchHistogram; + + if (existingMetric && existingMetric instanceof BatchHistogram) { + if (this.hasNewLabels(existingMetric, labelNames)) { + this.registry.removeSingleMetric(prefixedName); + + histogram = new BatchHistogram({ + name: prefixedName, + help: metric.help, + registry: this.registry, + labelNames, + }); + } else { + histogram = existingMetric as BatchHistogram; + } + } else { + histogram = new BatchHistogram({ + name: prefixedName, + help: metric.help, + registry: this.registry, + labelNames, + }); + } + + for (const sample of metric.samples) { + const transformedLabels = this.transformLabels({ + ...sample.labels, + origin: sample.labels?.origin || 'sdk', + }); + + histogram.recordBatch(transformedLabels, { + count: sample.count, + sum: sample.sum, + buckets: sample.buckets, + }); + } + + return histogram; } return null; diff --git a/src/lib/features/metrics/shared/schema.ts b/src/lib/features/metrics/shared/schema.ts index 6670e5630d..9b7a0c0021 100644 --- a/src/lib/features/metrics/shared/schema.ts +++ b/src/lib/features/metrics/shared/schema.ts @@ -99,6 +99,33 @@ export const metricSampleSchema = joi .optional(), }); +export const histogramSampleSchema = joi + .object() + .options({ stripUnknown: true }) + .keys({ + count: joi.number().required(), + sum: joi.number().required(), + buckets: joi + .array() + .items( + joi.object({ + le: joi + .alternatives() + .try(joi.number(), joi.string().valid('+Inf')) + .required(), + count: joi.number().required(), + }), + ) + .required(), + labels: joi + .object() + .pattern( + joi.string(), + joi.alternatives().try(joi.string(), joi.number()), + ) + .optional(), + }); + export const impactMetricSchema = joi .object() .options({ stripUnknown: true }) @@ -106,7 +133,12 @@ export const impactMetricSchema = joi name: joi.string().required(), help: joi.string().required(), type: joi.string().required(), - samples: joi.array().items(metricSampleSchema).required(), + buckets: joi.array().items(joi.number()).optional(), + samples: joi.when('type', { + is: 'histogram', + then: joi.array().items(histogramSampleSchema).required(), + otherwise: joi.array().items(metricSampleSchema).required(), + }), }); export const impactMetricsSchema = joi