-
Notifications
You must be signed in to change notification settings - Fork 15
fix(metrics): never emit runtime_http_* samples without a handler label #673
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 3 commits
729b14e
ee897b2
9581066
5272de5
2924d2f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,6 +6,17 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/) | |
| and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html). | ||
|
|
||
| ## [Unreleased] | ||
| ### Fixed | ||
| - `runtime_http_*` metrics no longer emit samples without a `handler` label. Requests | ||
| that never reach a named handler (unmatched paths, replica-level rate limit | ||
| rejections, errors before the route pipeline) were counted with | ||
| `handler: undefined`; Node's cluster IPC serializes worker registries as JSON, which | ||
| drops `undefined` values, so the aggregated `/metrics` exposed a second, unnamed | ||
| series that Prometheus reads as `handler=""`. Those requests are now labelled | ||
| `handler="undefined"` — the same value prom-client rendered locally before cluster | ||
| aggregation — keeping dashboards and alerts that filter on it working. | ||
| - `/_status` requests are now reported as `handler="builtin:status-track"`, matching | ||
| the other builtin handlers, instead of falling into the unnamed bucket. | ||
|
|
||
| ## [7.4.0] - 2026-06-22 | ||
| ### Changed | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [general.documentation-gap] 🔵 SUGGEST The new Action: Ask the author to confirm and backfill the missing To dismiss: |
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -47,8 +47,14 @@ const parseSeries = (text: string, metric: string): Record<string, number> => { | |
| return out | ||
| } | ||
|
|
||
| // Node's cluster IPC serializes messages as JSON, so worker registries reach the | ||
| // master through a JSON round-trip. Reproducing it here keeps these tests honest | ||
| // about what the master actually merges (notably: JSON drops `undefined` label | ||
| // values, which used to strip the `handler` label from the aggregated output). | ||
| const overClusterIpc = <T>(payload: T): T => JSON.parse(JSON.stringify(payload)) | ||
|
|
||
| const aggregateRegistries = async (registries: Array<Registry>): Promise<string> => { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Evolvability.Organizational] 🔵 SUGGEST
Action: Extract the helper into a shared test utility (e.g. To dismiss: |
||
| const jsons = await Promise.all(registries.map((r) => r.getMetricsAsJSON())) | ||
| const jsons = await Promise.all(registries.map(async (r) => overClusterIpc(await r.getMetricsAsJSON()))) | ||
| const merged = AggregatorRegistry.aggregate(jsons) | ||
| return merged.metrics() | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,129 @@ | ||
| import { EventEmitter } from 'events' | ||
| import { AggregatorRegistry, register } from 'prom-client' | ||
|
|
||
| import { requestHandlerLabel, UNNAMED_REQUEST_HANDLER } from '../requestHandlerLabel' | ||
| import { addRequestMetricsMiddleware } from '../requestMetricsMiddleware' | ||
|
|
||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Functional.Check] 🔵 SUGGEST The new regression suite only exercises Action: Add an equivalent test for To dismiss: |
||
| // Node's cluster IPC serializes messages as JSON, which drops properties whose | ||
| // value is `undefined`. This is what the master receives from each worker. | ||
| const overClusterIpc = <T>(payload: T): T => JSON.parse(JSON.stringify(payload)) | ||
|
|
||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Evolvability.Organizational] 🔵 SUGGEST The Action: Extract To dismiss: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Evolvability.Organizational] 🔵 SUGGEST The Action: Extract To dismiss: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Evolvability.Organizational] 🔵 SUGGEST
Action: Extract To dismiss: |
||
| const aggregatedMetrics = async (): Promise<string> => { | ||
| const workerRegistryJson = overClusterIpc(await register.getMetricsAsJSON()) | ||
| return AggregatorRegistry.aggregate([workerRegistryJson]).metrics() | ||
| } | ||
|
|
||
| // Minimal ServiceContext stand-in for addRequestMetricsMiddleware: it only needs | ||
| // `req`/`res` emitters and a `response` with `length` and `status`. | ||
| const buildCtx = (requestHandlerName?: string) => { | ||
| const res = new EventEmitter() | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [quality.new-logic-enforcement] 🔵 SUGGEST
Action: Add a case where To dismiss: |
||
| return { | ||
| req: new EventEmitter(), | ||
| requestHandlerName, | ||
| res, | ||
| response: { length: 128, status: 200 }, | ||
| } | ||
| } | ||
|
|
||
| // Closing the response inside `next()` makes the middleware finish its timings | ||
| // synchronously, so no stream plumbing is needed. | ||
| const runRequest = async (middleware: any, ctx: any) => { | ||
| await middleware(ctx, async () => { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Evolvability.SupportedByLanguage] 🔵 SUGGEST
Action: Type To dismiss: |
||
| ctx.res.emit('close') | ||
| }) | ||
| } | ||
|
|
||
| const samplesOf = (text: string, metric: string) => | ||
| text | ||
| .split('\n') | ||
| .filter((line) => line.startsWith(metric) && !line.startsWith('# ')) | ||
|
|
||
| describe('requestHandlerLabel', () => { | ||
| it('falls back to "undefined" so the label is never empty or missing', () => { | ||
| expect(UNNAMED_REQUEST_HANDLER).toBe('undefined') | ||
| expect(requestHandlerLabel(undefined)).toBe(UNNAMED_REQUEST_HANDLER) | ||
| expect(requestHandlerLabel('')).toBe(UNNAMED_REQUEST_HANDLER) | ||
| }) | ||
|
|
||
| it('keeps a named handler untouched', () => { | ||
| expect(requestHandlerLabel('private-handler:ssr')).toBe('private-handler:ssr') | ||
| }) | ||
| }) | ||
|
|
||
| describe('addRequestMetricsMiddleware handler label', () => { | ||
| beforeEach(() => { | ||
| // The instruments register into the default registry on construction. | ||
| register.clear() | ||
| }) | ||
|
|
||
| it('labels requests that never reached a named handler', async () => { | ||
| const middleware = addRequestMetricsMiddleware() | ||
| await runRequest(middleware, buildCtx(undefined)) | ||
|
|
||
| const local = await register.metrics() | ||
| expect(samplesOf(local, 'runtime_http_requests_total')).toEqual([ | ||
| 'runtime_http_requests_total{handler="undefined",status_code="200"} 1', | ||
| ]) | ||
| }) | ||
|
|
||
| // Regression: with `handler: undefined` the label key survived in-process but was | ||
| // dropped by the cluster IPC JSON serialization, so the aggregated /metrics | ||
| // exposed `runtime_http_requests_total{status_code="200"}` — a second, unnamed | ||
| // series (Prometheus reads an absent label as `handler=""`). | ||
| it('keeps the handler label through the cluster IPC round-trip', async () => { | ||
| const middleware = addRequestMetricsMiddleware() | ||
| await runRequest(middleware, buildCtx(undefined)) | ||
|
|
||
| const aggregated = await aggregatedMetrics() | ||
| const samples = samplesOf(aggregated, 'runtime_http_requests_total') | ||
|
|
||
| expect(samples).toEqual(['runtime_http_requests_total{handler="undefined",status_code="200"} 1']) | ||
| expect(aggregated).not.toContain('runtime_http_requests_total{status_code=') | ||
| }) | ||
|
|
||
| it('emits no sample with a missing or empty handler label', async () => { | ||
| const middleware = addRequestMetricsMiddleware() | ||
| await runRequest(middleware, buildCtx(undefined)) | ||
| await runRequest(middleware, buildCtx('private-handler:ssr')) | ||
|
|
||
| const aggregated = await aggregatedMetrics() | ||
| const handlerLabelledMetrics = [ | ||
| 'runtime_http_requests_total', | ||
| 'runtime_http_requests_duration_milliseconds', | ||
| 'runtime_http_response_size_bytes', | ||
| ] | ||
|
|
||
| handlerLabelledMetrics.forEach((metric) => { | ||
| samplesOf(aggregated, metric).forEach((sample) => { | ||
| expect(sample).toMatch(/handler="[^"]+"/) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Functional.Check] 🔵 SUGGEST The 'emits no sample with a missing or empty handler label' test nests its only assertion inside Action: Assert the sample list is non-empty before iterating, e.g. To dismiss: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Evolvability.SolutionApproach] 🔵 SUGGEST The Action: Assert To dismiss: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Functional.Check] 🟡 RESTRICT The assertion in 'emits no sample with a missing or empty handler label' is vacuous: Action: Assert the sample set is non-empty before iterating, e.g. To dismiss: |
||
| }) | ||
| }) | ||
| }) | ||
|
|
||
| it('reports named and unnamed handlers as separate series', async () => { | ||
| const middleware = addRequestMetricsMiddleware() | ||
| await runRequest(middleware, buildCtx('private-handler:ssr')) | ||
| await runRequest(middleware, buildCtx(undefined)) | ||
|
|
||
| const aggregated = await aggregatedMetrics() | ||
| expect(samplesOf(aggregated, 'runtime_http_requests_total').sort()).toEqual([ | ||
| 'runtime_http_requests_total{handler="private-handler:ssr",status_code="200"} 1', | ||
| 'runtime_http_requests_total{handler="undefined",status_code="200"} 1', | ||
| ]) | ||
| }) | ||
|
|
||
| it('labels aborted requests that never reached a named handler', async () => { | ||
| const middleware = addRequestMetricsMiddleware() | ||
| const ctx: any = buildCtx(undefined) | ||
|
|
||
| await middleware(ctx, async () => { | ||
| ctx.req.emit('aborted') | ||
| ctx.res.emit('close') | ||
| }) | ||
|
|
||
| const aggregated = await aggregatedMetrics() | ||
| expect(samplesOf(aggregated, 'runtime_http_aborted_requests_total')).toEqual([ | ||
| 'runtime_http_aborted_requests_total{handler="undefined"} 1', | ||
| ]) | ||
| }) | ||
| }) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,7 @@ import { finished as onStreamFinished } from 'stream' | |
| import { hrToMillisFloat } from '../../utils' | ||
| import { ServiceContext } from '../worker/runtime/typings' | ||
| import { getOtelInstruments, OtelRequestInstruments, RequestsMetricLabels } from './metrics' | ||
| import { requestHandlerLabel } from './requestHandlerLabel' | ||
|
|
||
| const INSTRUMENTS_INITIALIZATION_TIMEOUT = 500 | ||
|
|
||
|
|
@@ -38,7 +39,9 @@ export const addOtelRequestMetricsMiddleware = () => { | |
|
|
||
| ctx.req.once('aborted', () => { | ||
| if (instruments) { | ||
| instruments.abortedRequests.add(1, { [RequestsMetricLabels.REQUEST_HANDLER]: ctx.requestHandlerName }) | ||
| instruments.abortedRequests.add(1, { | ||
| [RequestsMetricLabels.REQUEST_HANDLER]: requestHandlerLabel(ctx.requestHandlerName), | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [quality.new-logic-enforcement] 🔵 SUGGEST
Action: Add a test that drives To dismiss: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [quality.new-logic-enforcement] 🔵 SUGGEST
Action: Add a test for To dismiss: |
||
| }) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Functional.Check] 🔵 SUGGEST
Action: Add at least one case driving To dismiss: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Functional.Interface] 🟡 RESTRICT The Action: Either state explicitly in the CHANGELOG that the diagnostics/OTel To dismiss: |
||
| } | ||
| }) | ||
|
|
||
|
|
@@ -53,7 +56,7 @@ export const addOtelRequestMetricsMiddleware = () => { | |
| instruments.responseSizes.record( | ||
| responseLength, | ||
| { | ||
| [RequestsMetricLabels.REQUEST_HANDLER]: ctx.requestHandlerName, | ||
| [RequestsMetricLabels.REQUEST_HANDLER]: requestHandlerLabel(ctx.requestHandlerName), | ||
| [RequestsMetricLabels.STATUS_CODE]: ctx.response.status, | ||
| [RequestsMetricLabels.ACCOUNT_NAME]: ctx.vtex?.account || 'unknown', | ||
| } | ||
|
|
@@ -64,7 +67,7 @@ export const addOtelRequestMetricsMiddleware = () => { | |
| instruments.totalRequests.add( | ||
| 1, | ||
| { | ||
| [RequestsMetricLabels.REQUEST_HANDLER]: ctx.requestHandlerName, | ||
| [RequestsMetricLabels.REQUEST_HANDLER]: requestHandlerLabel(ctx.requestHandlerName), | ||
| [RequestsMetricLabels.STATUS_CODE]: ctx.response.status, | ||
| [RequestsMetricLabels.ACCOUNT_NAME]: ctx.vtex?.account || 'unknown', | ||
| } | ||
|
|
@@ -76,7 +79,7 @@ export const addOtelRequestMetricsMiddleware = () => { | |
| instruments.requestTimings.record( | ||
| hrToMillisFloat(process.hrtime(start)), | ||
| { | ||
| [RequestsMetricLabels.REQUEST_HANDLER]: ctx.requestHandlerName, | ||
| [RequestsMetricLabels.REQUEST_HANDLER]: requestHandlerLabel(ctx.requestHandlerName), | ||
| [RequestsMetricLabels.STATUS_CODE]: ctx.response.status, | ||
| [RequestsMetricLabels.ACCOUNT_NAME]: ctx.vtex?.account || 'unknown', | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,28 @@ | ||
| /** | ||
| * Label value reported for requests that never reached a named handler: unmatched | ||
| * paths (Koa answers its default 404), requests rejected before the route pipeline | ||
| * (replica-level rate limiter, errors in compress/recorder), and handlers that | ||
| * don't set `ctx.requestHandlerName`. | ||
| * | ||
| * The value must be a non-empty string. `ctx.requestHandlerName` is `undefined` | ||
| * for those requests, and Node's cluster IPC serializes each worker's registry as | ||
| * JSON, which drops properties whose value is `undefined`. The sample then reaches | ||
| * the aggregated `/metrics` with the `handler` label missing altogether, which | ||
| * Prometheus reads as `handler=""` — a distinct, unnamed series that shows up as | ||
| * an extra "Value" line in dashboards. | ||
| * | ||
| * `'undefined'` is deliberate rather than a nicer word: prom-client's local | ||
| * exposition already rendered `handler: undefined` as `handler="undefined"` before | ||
| * the cluster aggregation was introduced, so keeping that value makes the | ||
| * aggregated output match the historical series identity and keeps existing | ||
| * dashboards, filters and alerts (e.g. `handler!~"builtin:.*|undefined"`) working. | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [general.broken-references] 🔵 SUGGEST The whole design decision — choosing the literal string Action: Ask the author to double-check both references — the prom-client behaviour for the pinned version and at least one real dashboard/alert using that filter — and cite them (dashboard URL or alert rule) in the comment so a future reader can safely change the value. To dismiss: |
||
| */ | ||
| export const UNNAMED_REQUEST_HANDLER = 'undefined' | ||
|
|
||
| /** | ||
| * Resolves the `handler` label value for a request, falling back to | ||
| * {@link UNNAMED_REQUEST_HANDLER} when the pipeline never named the handler. | ||
| * Empty strings fall back too, so the label is never emitted empty. | ||
| */ | ||
| export const requestHandlerLabel = (requestHandlerName?: string): string => | ||
| requestHandlerName || UNNAMED_REQUEST_HANDLER | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| import { statusTrackHandler } from '../statusTrack' | ||
| import { ServiceContext } from '../typings' | ||
|
|
||
| describe('statusTrackHandler', () => { | ||
| // /_status is served by a handler that answers 200, so its samples must carry a | ||
| // handler label like every other builtin (healthcheck, whoami, metrics-logger) | ||
| // instead of landing in the catch-all `handler="undefined"` bucket. | ||
| it('names the request so metrics are not reported as unnamed', async () => { | ||
| const setOperationName = jest.fn() | ||
| const ctx: any = { | ||
| body: undefined, | ||
| tracing: { currentSpan: { setOperationName } }, | ||
| } | ||
|
|
||
| await statusTrackHandler(ctx as ServiceContext) | ||
|
|
||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Functional.Resource] 🔴 BLOCK The new test calls Action: Stub the IPC channel in the test, e.g. To dismiss: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Functional.Resource] 🟡 RESTRICT The new test calls Action: Stub the channel in the test (e.g. To dismiss: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Functional.Resource] 🔴 BLOCK The new test invokes Action: Stub the IPC channel in the test, following the convention already used in To dismiss: |
||
| expect(ctx.requestHandlerName).toBe('builtin:status-track') | ||
| expect(setOperationName).toHaveBeenCalledWith('builtin:status-track') | ||
| expect(ctx.body).toEqual([]) | ||
| }) | ||
|
|
||
| it('works when tracing is disabled for the path', async () => { | ||
| const ctx: any = { body: undefined, tracing: undefined } | ||
|
|
||
| await statusTrackHandler(ctx as ServiceContext) | ||
|
|
||
| expect(ctx.requestHandlerName).toBe('builtin:status-track') | ||
| }) | ||
| }) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[general.documentation-gap] 🔵 SUGGEST
The changelog entry scopes the fix to
runtime_http_*Prometheus metrics and justifies it entirely with Node cluster IPC JSON serialization, butrequestHandlerLabelwas also applied tootelRequestMetricsMiddleware.ts(aborted requests, response sizes, total requests, request timings). OpenTelemetry metrics do not go through the cluster IPC round-trip; for that exporter this silently changes thehandlerattribute from absent/undefined to the literal string"undefined", which is a series-identity change for any OTel-backed dashboard or alert. That impact is undocumented.Action: Add a changelog bullet covering the OpenTelemetry request metrics as well, stating that the
handlerattribute is now always present and is"undefined"for unnamed handlers, so consumers of the OTel pipeline can adjust queries.To dismiss:
/dk-review dismiss 9a4d7e21-c3f6-4b58-8e0a-71d5c9f2b6e3 [reason]