Skip to content

Aggregate prom-client metrics across cluster workers for /metrics - #667

Open
silvadenisaraujo wants to merge 5 commits into
masterfrom
night/aggregate-prom-client-metrics-across-cluster-wor
Open

Aggregate prom-client metrics across cluster workers for /metrics#667
silvadenisaraujo wants to merge 5 commits into
masterfrom
night/aggregate-prom-client-metrics-across-cluster-wor

Conversation

@silvadenisaraujo

@silvadenisaraujo silvadenisaraujo commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Summary

VTEX IO runtimes run as a Node.js cluster of min(cpus, 4) workers, each keeping its own prom-client registry, and /metrics is answered by whichever worker the shared-port round-robin hands the scrape to. Because keep-alive (5s) is shorter than the scrape interval, nearly every scrape hits a different worker, so Prometheus stores an interleaved braid of 4 independent counters under one instance label — treating every downward sample as a counter reset and inflating increase()/rate() on runtime_* counters by orders of magnitude (~×40 measured live). This PR fixes that by aggregating across workers with prom-client's AggregatorRegistry (pinned ^14.2.0): the master builds a single merged, monotonic view over the existing cluster IPC and the worker answering the scrape serves that aggregate. Single-worker / LINKED mode stays unchanged.

Spec

Accepted. See specs/aggregate-prom-client-metrics-across-cluster-wor.md.

Implementation

  • src/service/metrics/clusterMetricsAggregator.ts (new): the master-side AggregatorRegistry plus a small tagged-IPC protocol. A worker answering a /metrics scrape asks the master over the existing cluster IPC (AGG_METRICS_REQ), the master builds the merged, monotonic cluster view via clusterMetrics() and replies (AGG_METRICS_RES). prom-client's own prom-client:* cluster messages are recognised and ignored by our handlers. On timeout/error the worker falls back to its local register.metrics() so /metrics never hangs.
  • src/service/master.ts: initialises the master aggregator when workers > 1 and routes worker aggregate requests / prom-client messages in onMessage.
  • src/service/worker/index.ts: installs the worker-side cluster responder when workers > 1 and routes the master's aggregate response in onMessage.
  • src/service/worker/runtime/builtIn/middlewares.ts: prometheusLoggerMiddleware(workers) serves the cluster aggregate in multi-worker mode and the local default registry in single-worker mode. Scrapes are still excluded from request counters (early return preserved) and metric names/labels are unchanged.

Covered by tests proving monotonicity across whichever worker answers, sum-across-N-workers, single-worker default-registry behaviour, and presence of default process metrics.

Testing

  • Beta tag: v7.4.1-beta.X

Validation

Validated live on cluster dv-dj-iotest-eks-use1c-ju2, deployment
vtex-search-resolver-1-106-1-023ca1f28e29b7a (image service-node:7.7.13-beta.0,
2 workers).

Method: port-forward one pod and scrape GET /metrics:5050 repeatedly. Each
curl opens a new connection (round-robined across workers — the condition that
exposed the original bug), asserting no runtime_http_requests_total series
decreases between consecutive scrapes.

Results (fixed build):

Scrapes total runtime_http_requests_total first → last Monotonicity violations
15 1373 → 1418 0
30 2271 → 2278 0
100 2513 → 2645 0

The aggregate is stable and strictly non-decreasing regardless of which worker
answers — no braiding, no counter resets. No fallback/timeout warnings in logs.

Baseline contrast (service-node:6.42.0, pre-fix): the identical scrape method
produced braided per-worker output (e.g. catalog,200 cycling 285 → 104 → 167 → …),
the behavior that inflated Prometheus rate()/increase().

Live coverage: AC1 (monotonic) ✅, AC2 (sum across workers) ✅, AC4 (default metrics
present) ✅. AC3 (single-worker path) covered by unit tests.


Internal task id: task-58ee1acb (bean)

Specification only (no implementation) for fixing per-worker Prometheus
registry interleaving on /metrics via prom-client AggregatorRegistry.

Internal task id: task-58ee1acb (bean)
@silvadenisaraujo silvadenisaraujo self-assigned this Jul 16, 2026
silvadenisaraujo and others added 2 commits July 17, 2026 11:11
Multi-worker runtimes kept a per-worker prom-client registry, so /metrics
answered from whichever worker the shared-port round-robin picked. Because
keep-alive (5s) is shorter than the scrape interval, Prometheus stored an
interleaved braid of independent counters under one instance label, treating
every downward sample as a counter reset and inflating rate()/increase() on
runtime_* counters by orders of magnitude.

Aggregate across workers with prom-client's AggregatorRegistry: the master
builds a single merged, monotonic view over the existing cluster IPC and the
worker answering the scrape serves that aggregate. Single-worker / LINKED mode
keeps serving the local default registry unchanged.

- src/service/metrics/clusterMetricsAggregator.ts: IPC request/response
  protocol, master aggregator, worker responder, timeout fallback to local
  registry.
- master.ts / worker/index.ts: wire the tagged IPC messages and set up the
  aggregator only when workers > 1.
- builtIn/middlewares.ts: prometheusLoggerMiddleware serves the aggregate in
  multi-worker mode, local registry otherwise; /metrics still excluded from
  request counters.

Includes unit tests proving cross-worker sum, monotonicity, single-worker
behavior, and default-metric presence.

Co-authored-by: Cursor <cursoragent@cursor.com>
This analysis artifact belongs to a separate investigation and was
included in this PR by mistake.

Co-authored-by: Cursor <cursoragent@cursor.com>
Comment thread specs/aggregate-prom-client-metrics-across-cluster-wor.md Outdated
Comment thread specs/aggregate-prom-client-metrics-across-cluster-wor.md Outdated
Comment thread src/service/metrics/clusterMetricsAggregator.ts Outdated
Comment thread src/service/metrics/clusterMetricsAggregator.ts Outdated
Comment thread src/service/worker/index.ts Outdated
silvadenisaraujo and others added 2 commits July 22, 2026 16:35
Review comments:
- spec: drop redundant task-id/branch metadata lines; mark Status accepted
  now that the implementation ships in this PR
- clusterMetricsAggregator: use optional chaining in the message type guards
- worker/index: pass an explicit callback and initial value to .reduce()
  instead of the bare mergeDeepRight function

SonarQube quality gate:
- raise new-code coverage: clusterMetricsAggregator.ts to 100% (registry
  lifecycle + error/fallback paths) and cover the new master/worker IPC
  onMessage branches via exported handlers

Co-authored-by: Cursor <cursoragent@cursor.com>
@silvadenisaraujo
silvadenisaraujo marked this pull request as ready for review July 23, 2026 19:21
@sonar-workflows

Copy link
Copy Markdown

Failed Quality Gate failed

  • 315 New Issues (is greater than 0)
  • 31.00% Coverage on New Code (is less than 75.00%)
  • 0.00% Security Hotspots Reviewed on New Code (is less than 100.00%)
  • D Reliability Rating on New Code (is worse than A)

Project ID: node-vtex-api

View in SonarQube

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes incorrect Prometheus counter behavior in multi-worker Node.js cluster mode by serving /metrics from a cluster-wide aggregated prom-client registry (via AggregatorRegistry + IPC to the master), preventing the “braided counters” effect that caused inflated rate()/increase().

Changes:

  • Add a master/worker IPC protocol and AggregatorRegistry wiring to build and serve a merged, monotonic /metrics output in multi-worker mode.
  • Update the /metrics Koa middleware to serve aggregated metrics when workers > 1, while preserving the single-worker (LINKED / workers:1) behavior.
  • Add Jest coverage for aggregation semantics, routing/guarding of IPC messages, and /metrics middleware behavior.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/service/worker/runtime/builtIn/middlewares.ts Serve cluster-aggregated metrics for /metrics when in multi-worker mode; keep single-worker behavior unchanged.
src/service/worker/runtime/builtIn/tests/middlewares.test.ts Tests for /metrics branching (single vs multi-worker) and ensuring scrapes aren’t counted.
src/service/worker/index.ts Install worker-side AggregatorRegistry responder; route aggregation responses; pass workers into middleware.
src/service/worker/tests/onMessage.test.ts Tests that worker IPC routing handles aggregation responses and ignores prom-client protocol messages.
src/service/metrics/clusterMetricsAggregator.ts New aggregation module: message types/guards, master request handler, worker request/response correlation + timeout fallback.
src/service/metrics/tests/clusterMetricsAggregator.test.ts Tests for aggregation properties (sum, monotonicity, defaults) and IPC request/response handling.
src/service/master.ts Initialize master-side aggregator and route aggregation requests; ignore prom-client protocol messages.
src/service/tests/master.test.ts Tests that master IPC routing handles aggregation requests and ignores prom-client protocol messages.
specs/aggregate-prom-client-metrics-across-cluster-wor.md Accepted spec documenting motivation, design, and test plan.
package.json Bump version to 7.4.1-beta.1.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +86 to +94
export const handleWorkerMetricsRequest = async (worker: Worker, message: AggMetricsReqMessage): Promise<void> => {
const aggregator = initMasterAggregatorRegistry()
try {
const body = await aggregator.clusterMetrics()
worker.send({ type: AGG_METRICS_RES, id: message.id, body })
} catch (err) {
worker.send({ type: AGG_METRICS_RES, id: message.id, error: (err as Error)?.message ?? String(err) })
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems low, but I dont know how often a worker's availability could lead to this scenario

@juliobguedes juliobguedes left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NITs only. LGTM

Comment thread src/service/master.ts
broadcastStatusTrack()
} else if (isAggMetricsRequest(message)) {
handleWorkerMetricsRequest(worker, message)
} else if (isPromClientMessage(message)) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
} else if (isPromClientMessage(message)) {
} else if (!isPromClientMessage(message)) {
// place else condition here instead of having an empty if to skip

trackStatus()
} else if (isAggMetricsResponse(message)) {
handleMasterMetricsResponse(message)
} else if (isPromClientMessage(message)) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

Comment on lines +86 to +94
export const handleWorkerMetricsRequest = async (worker: Worker, message: AggMetricsReqMessage): Promise<void> => {
const aggregator = initMasterAggregatorRegistry()
try {
const body = await aggregator.clusterMetrics()
worker.send({ type: AGG_METRICS_RES, id: message.id, body })
} catch (err) {
worker.send({ type: AGG_METRICS_RES, id: message.id, error: (err as Error)?.message ?? String(err) })
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems low, but I dont know how often a worker's availability could lead to this scenario

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants