Skip to content

Add opt-in Prometheus /metrics endpoint for tool calls - #1131

Merged
helpcodeai merged 4 commits into
HelpCode-ai:mainfrom
rangarajan19:feat/prom-metrics-1003
Oct 20, 2026
Merged

helpcodeai merged 4 commits into
HelpCode-ai:mainfrom
rangarajan19:feat/prom-metrics-1003

Conversation

@rangarajan19

Copy link
Copy Markdown
Contributor

Summary

Adds an opt-in Prometheus /metrics endpoint for tool calls, per #1003 (October Challenge #997).

  • Off by default (METRICS_ENABLED=false). When on, starts a second, separate node:http listener (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 on tracing.ts), using its own prom-client Registry (not the global default), so "disabled means nothing collected" is actually true.
  • Metrics (prefix 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 from processGauges via a collect() callback at scrape time, plus collectDefaultMetrics().
  • Recorded from the single logToolCall() call site in dynamic-mcp-tools.ts, so the metric and the tool_call log line can never drift apart. status is lowercased to success|error to match the issue.
  • Cloud rules: refuses to start without METRICS_TOKEN; METRICS_TOOL_LABELS is always ignored (no connector_id/tool label ever appears), even if set — tested adversarially (passing the labels anyway and asserting they don't leak).
  • connector_id uses the connector's cuid (tool.connectorId → connector.id), never its display name.
  • Extracted sameSecret() (constant-time token comparison) out of sentry-verify.controller.ts into common/secrets.util.ts so it isn't duplicated.
  • Docs: new "## 6. Prometheus metrics (opt-in)" section in docs/operations/observability.md with a sample scrape config, plus the five env vars added to the table in docs/deployment.md. docker-compose.yml is intentionally untouched (METRICS_PORT not 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 _sum actually reflects the observed duration, not just the metric name appearing).
  • packages/backend/src/common/secrets.util.ts — added sameSecret().
  • packages/backend/src/health/sentry-verify.controller.ts — now imports sameSecret() instead of defining its own copy.
  • packages/backend/src/mcp-server/dynamic-mcp-tools.ts — calls recordToolCall() from logToolCall().
  • packages/backend/src/main.ts — starts the listener after app.listen(), stops it in the existing SIGTERM/SIGINT handler.
  • packages/backend/package.json / package-lock.json — added prom-client@15.1.3.
  • docs/operations/observability.md, docs/deployment.md — new section + env var rows.

Type

  • New feature
  • Documentation

Testing

  • Added/updated unit tests
  • Tested manually (describe steps below)

Commands run from packages/backend unless 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 (no secrets/health spec files exist, so only metrics and dynamic-mcp-tools ran — confirms the logToolCall() 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-existing RUN_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.
  • From repo root: node scripts/check-runtime-deps.mjs → "No backend production dependency is nested: the runtime image gets them all." (confirms prom-client hoists cleanly, per the CI check this PR's dependency needs to pass).
  • From repo root: 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 (including ConnectorsModule/McpServerModule, which import metrics.ts) initialized with zero module-resolution errors before the (expected) DB-connection failure in LicenseService.onModuleInit. No "Cannot find module" anywhere.

Checklist

  • My code follows the existing code style
  • I have tested my changes locally
  • I have updated documentation (if applicable)
  • All existing tests pass (backend: lint, typecheck, and the suites listed above)
  • This PR has a descriptive title

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.

)

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 helpcodeai left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.
@rangarajan19

Copy link
Copy Markdown
Contributor Author

@helpcodeai thanks for the thorough review — pushed a follow-up commit (1bdfb42) addressing all of it:

  1. METRICS_TOKEN is now required whenever METRICS_ENABLED=true, self-hosted included — matches SENTRY_VERIFY_TOKEN. Docs updated in both deployment.md and observability.md.
  2. Added a server.on('error', ...) handler — a taken port now logs and leaves metrics off instead of crashing the backend.
  3. tool_call_upstream_errors_total now only increments on an actual 4xx/5xx; a success (or no upstream status) is never counted.
  4. Added METRICS_* to both docker-compose.yml and docker-compose.cloud.yml.
  5. Fixed the sample scrape target to app:9464 (the real compose service name).
  6. Reset package-lock.json to main and reinstalled prom-client — diff is now 30 insertions only (prom-client, tdigest, bintrees), verified with a real npm ci too.

Also expanded metrics.spec.ts to cover all three behavior changes (12 tests total), and re-ran tsc --noEmit plus the full backend suite — all green.

Please let me know if anything's still off — happy to fix it.

@helpcodeai helpcodeai left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @rangarajan19, everything from the last review is in, and done well:

  • METRICS_TOKEN is required whenever METRICS_ENABLED=true, self-hosted included, and the docs say so.
  • The error handler keeps the backend up when 9464 is taken, and there's a test for it.
  • tool_call_upstream_errors_total only moves on a 4xx/5xx.
  • METRICS_* is in both compose files, and the sample target is app: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:

  1. npm now marks prom-client deprecated ("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/client reaches 1.0 we should switch, and keeping everything behind metrics.ts will make that a one-file change. I'll open an issue for it.
  2. Other secrets in secrets.util.ts must be at least 32 characters. A warning when METRICS_TOKEN is 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.

@helpcodeai
helpcodeai enabled auto-merge (squash) October 20, 2026 10:58
@helpcodeai
helpcodeai merged commit 5e60fde into HelpCode-ai:main Oct 20, 2026
14 checks passed
@github-actions github-actions Bot locked and limited conversation to collaborators Oct 20, 2026
@helpcodeai

Copy link
Copy Markdown
Contributor

Merged, thanks again @rangarajan19! It ships in the next Cloud release. The follow-ups from the review are tracked in #1169 (move to @prometheus-io/client once it reaches 1.0); the METRICS_TOKEN length warning is up for grabs if you fancy a small one.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

🎃 Add an opt-in Prometheus /metrics endpoint for tool calls

2 participants