Repository navigation
Add opt-in Prometheus /metrics endpoint for tool calls - #1131
Conversation
) Adds a second, separate node:http listener (off by default) serving tool-call counters, a duration histogram, upstream-error classification and mirrored process gauges via prom-client. Required METRICS_TOKEN and dropped per-tool/per-connector labels on Cloud keep it instance-wide and leak-free across tenant workspaces.
helpcodeai
left a comment
There was a problem hiding this comment.
Thanks a lot for this, @rangarajan19, it's really well put together. Running the listener on its own port instead of a Nest route is a better call than what I suggested in the issue, and the Cloud label guard plus the tests are exactly what we needed.
A few things before we merge. First, the token: with METRICS_TOKEN unset, a self-hosted instance serves /metrics to anyone, and since the default host is 0.0.0.0 that includes bare-metal installs. Could you require the token whenever METRICS_ENABLED=true, the way SENTRY_VERIFY_TOKEN works, and update the docs to match? Second, server.listen has no error handler, so if 9464 is taken the whole backend goes down. A log line and carrying on without metrics would be better.
tool_call_upstream_errors_total currently goes up on every call, successes included as none. I'd only count 4xx/5xx there, or rename it. The compose files list env vars one by one, so METRICS_* needs adding to the backend service in docker-compose.yml and docker-compose.cloud.yml, otherwise Docker users can't switch it on. The sample scrape target should be app:9464.
Last one, the lockfile: it looks like it was regenerated with an older npm (lots of peer/libc churn, and @emnapi/runtime removed). Could you reset it to main and run npm i prom-client@^15.1.3 -w packages/backend with npm 11, so the diff only adds prom-client, tdigest and bintrees?
Happy to help with any of this. Thanks again!
…, exclude success from upstream errors - METRICS_TOKEN is now required whenever METRICS_ENABLED=true, on Cloud and self-hosted alike, matching SENTRY_VERIFY_TOKEN's pattern. - Add a listen() error handler so a taken port logs and leaves metrics off instead of crashing the backend. - tool_call_upstream_errors_total now only increments on a 4xx/5xx upstream status; a success (or no status) is never counted. - Add METRICS_* to docker-compose.yml and docker-compose.cloud.yml so Docker users can turn the feature on. - Fix the sample scrape target to app:9464, the real compose service name. - Reset package-lock.json to main and reinstall prom-client so the diff only adds prom-client, tdigest and bintrees.
|
@helpcodeai thanks for the thorough review — pushed a follow-up commit (1bdfb42) addressing all of it:
Also expanded Please let me know if anything's still off — happy to fix it. |
There was a problem hiding this comment.
Thanks @rangarajan19, everything from the last review is in, and done well:
METRICS_TOKENis required wheneverMETRICS_ENABLED=true, self-hosted included, and the docs say so.- The
errorhandler keeps the backend up when 9464 is taken, and there's a test for it. tool_call_upstream_errors_totalonly moves on a 4xx/5xx.METRICS_*is in both compose files, and the sample target isapp:9464.- The lockfile diff is now just prom-client, tdigest and bintrees.
I also checked that it merges cleanly with current main (33 commits ahead of your base), that the three gauge names match what processGauges actually registers, and that nothing outside the PR's scope changed. Approving.
Two notes, neither blocking:
- npm now marks
prom-clientdeprecated ("replaced by@prometheus-io/client", the new package under the Prometheus org, still at 0.16). 15.1.3 is fine for now. Once@prometheus-io/clientreaches 1.0 we should switch, and keeping everything behindmetrics.tswill make that a one-file change. I'll open an issue for it. - Other secrets in
secrets.util.tsmust be at least 32 characters. A warning whenMETRICS_TOKENis shorter would be consistent. Happy to take that as a follow-up rather than hold this PR.
I've updated the branch so CI runs on current main.
|
Merged, thanks again @rangarajan19! It ships in the next Cloud release. The follow-ups from the review are tracked in #1169 (move to |
Summary
Adds an opt-in Prometheus
/metricsendpoint for tool calls, per #1003 (October Challenge #997).METRICS_ENABLED=false). When on, starts a second, separatenode:httplistener (never the main app's port 4000), because/health*is already proxied publicly on both Cloud and self-hosted.src/common/metrics.ts: a plain, no-DI facade (modeled ontracing.ts), using its ownprom-clientRegistry(not the global default), so "disabled means nothing collected" is actually true.anythingmcp_):tool_calls_total{connector_type,status},tool_call_duration_seconds{connector_type,status}(histogram),tool_call_upstream_errors_total{connector_type,upstream_status_class}, gauges mirrored fromprocessGaugesvia acollect()callback at scrape time, pluscollectDefaultMetrics().logToolCall()call site indynamic-mcp-tools.ts, so the metric and thetool_calllog line can never drift apart.statusis lowercased tosuccess|errorto match the issue.METRICS_TOKEN;METRICS_TOOL_LABELSis always ignored (noconnector_id/toollabel ever appears), even if set — tested adversarially (passing the labels anyway and asserting they don't leak).connector_iduses the connector's cuid (tool.connectorId→connector.id), never its display name.sameSecret()(constant-time token comparison) out ofsentry-verify.controller.tsintocommon/secrets.util.tsso it isn't duplicated.docs/operations/observability.mdwith a sample scrape config, plus the five env vars added to the table indocs/deployment.md.docker-compose.ymlis intentionally untouched (METRICS_PORTnot published).Changes
packages/backend/src/common/metrics.ts(new) — the facade:recordToolCall(),startMetricsServer(),stopMetricsServer().packages/backend/src/common/metrics.spec.ts(new) — 10 tests covering every acceptance criterion plus two adversarial checks (label leak when disabled; histogram_sumactually reflects the observed duration, not just the metric name appearing).packages/backend/src/common/secrets.util.ts— addedsameSecret().packages/backend/src/health/sentry-verify.controller.ts— now importssameSecret()instead of defining its own copy.packages/backend/src/mcp-server/dynamic-mcp-tools.ts— callsrecordToolCall()fromlogToolCall().packages/backend/src/main.ts— starts the listener afterapp.listen(), stops it in the existing SIGTERM/SIGINT handler.packages/backend/package.json/package-lock.json— addedprom-client@15.1.3.docs/operations/observability.md,docs/deployment.md— new section + env var rows.Type
Testing
Commands run from
packages/backendunless noted:npx jest src/common/metrics→ 10/10 passed.npx jest src/common/metrics src/common/secrets src/health src/mcp-server/dynamic-mcp-tools --maxWorkers=2→ 45/45 passed (nosecrets/healthspec files exist, so onlymetricsanddynamic-mcp-toolsran — confirms thelogToolCall()wiring didn't break existing tool-call logging).npx jest src/connectors --maxWorkers=2→ 1444/1444 runnable tests passed (70/72 suites; 2 suites and 11 tests skipped — pre-existingRUN_ODATA_PUBLIC_LIVE/DB_URL-gated live/integration specs, unrelated to this change).npx tsc --noEmit -p tsconfig.json→ clean.npm run lint:check→ clean.node scripts/check-runtime-deps.mjs→ "No backend production dependency is nested: the runtime image gets them all." (confirmsprom-clienthoists cleanly, per the CI check this PR's dependency needs to pass).docker build -t anythingmcp-backend-test:metrics-1003 .→ built successfully, then booted directly (docker run --entrypoint node ... backend/dist/src/main.js) with fake secrets and an unreachable DB — all modules (includingConnectorsModule/McpServerModule, which importmetrics.ts) initialized with zero module-resolution errors before the (expected) DB-connection failure inLicenseService.onModuleInit. No "Cannot find module" anywhere.Checklist
Related Issues
Fixes #1003
@helpcodeai this is ready for review. I've tried to cover everything from the issue and from your follow-up comment (own
Registry, lowercase status labels, the runtime-deps check, and a Docker build+boot), but please don't hesitate to point out anything I've missed or got wrong — happy to fix it quickly.