Skip to content

fix: drop metrics for unmatched routes to prevent cardinality explosion#152

Merged
rohilsurana merged 5 commits into
mainfrom
fix/drop-metrics-for-unmatched-routes
Jun 10, 2026
Merged

fix: drop metrics for unmatched routes to prevent cardinality explosion#152
rohilsurana merged 5 commits into
mainfrom
fix/drop-metrics-for-unmatched-routes

Conversation

@rohilsurana

@rohilsurana rohilsurana commented Jun 10, 2026

Copy link
Copy Markdown
Member

Summary

  • Scanner/bot traffic hitting arbitrary paths (e.g. /.env, /.aws/credentials) creates thousands of unique route label values in metrics
  • During attacks this inflated the Mimir tenant from ~230K to 500K+ active series, triggering per_user_series_limit discards for all services
  • Instead of pattern-matching known routes, we skip metrics for all 404 responses — scanner traffic always 404s, so this eliminates the cardinality problem without maintaining a route allowlist
  • Logging is preserved so scanner traffic remains visible for security auditing

Closes #151

Test plan

  • Existing toEndpoint tests still pass
  • New test verifies unmatched paths still map to /docs/:slug (toEndpoint unchanged)
  • Deploy and confirm 404 requests no longer produce metric series

@vercel

vercel Bot commented Jun 10, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
chronicle Ready Ready Preview, Comment Jun 10, 2026 10:43am

@coderabbitai

coderabbitai Bot commented Jun 10, 2026

Copy link
Copy Markdown

Review Change Stack

Caution

Review failed

Pull request was closed or merged during review

📝 Walkthrough

Summary by CodeRabbit

  • Bug Fixes
    • Improved telemetry tracking for 404 responses to ensure accurate monitoring data collection and error reporting.

Walkthrough

The telemetry plugin now normalizes 404 responses to a uniform endpoint label across SSR render and HTTP request metrics. When status is 404, both metric recording points use '/api/not-found' instead of transforming the route through toEndpoint().

Changes

404 Metrics Endpoint Normalization

Layer / File(s) Summary
SSR and HTTP 404 endpoint normalization
packages/chronicle/src/server/plugins/telemetry.ts
Both the chronicle:ssr-rendered hook (lines 85–86) and HTTP response hook (line 99) now conditionally set route/endpoint metric attributes to '/api/not-found' when status === 404 or res.status === 404, otherwise use toEndpoint(route).

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~2 minutes

Possibly related PRs

  • raystack/chronicle#115: Adds toEndpoint normalization and template-based route filtering for metrics cardinality control; this PR applies 404-specific labeling within that same normalization layer.

Suggested reviewers

  • whoAbhishekSah
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: dropping metrics for 404 responses (unmatched routes) to prevent cardinality explosion.
Description check ✅ Passed The description is directly related to the changeset, explaining the problem, solution, and test plan for dropping metrics on 404 responses.
Linked Issues check ✅ Passed The code changes directly address issue #151 by dropping metrics for 404 responses instead of pattern-matching routes, preserving logging for security auditing.
Out of Scope Changes check ✅ Passed All changes are scoped to the telemetry metric labeling for 404 responses in the two hooks, directly related to the stated objectives.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/drop-metrics-for-unmatched-routes

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot 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.

Actionable comments posted: 1

🧹 Nitpick comments (1)
packages/chronicle/src/server/plugins/telemetry.test.ts (1)

43-55: ⚡ Quick win

Add regression cases for docs root endpoints.

Please add assertions for /docs, /developer, and /v1 mapping to '/docs/:slug' so root docs paths don’t regress to null.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/chronicle/src/server/plugins/telemetry.test.ts` around lines 43 -
55, The test missing regression cases is in the "versioned docs map to
/docs/:slug" block using the toEndpoint() helper; add assertions that
toEndpoint('/docs'), toEndpoint('/developer'), and toEndpoint('/v1') each return
'/docs/:slug' so those root docs paths don't regress to null. Update the test
cases inside the same test function (referencing toEndpoint) to include these
three expectations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@packages/chronicle/src/server/plugins/telemetry.ts`:
- Around line 40-49: The DOC_PATH_PREFIX regex in telemetry.ts currently
requires a trailing slash so endpoints like "/docs", "/developer", and "/v1"
don't match and are dropped; update the DOC_PATH_PREFIX constant used by
toEndpoint to match either a trailing slash or end-of-string (e.g., change
/^\/(?:docs|developer|v\d+)\// to /^\/(?:docs|developer|v\d+)(?:\/|$)/) so
DOC_PATH_PREFIX.test(pathname) will return true for both the root docs paths and
their subpaths, ensuring toEndpoint returns ROUTES.DOCS for those URLs.

---

Nitpick comments:
In `@packages/chronicle/src/server/plugins/telemetry.test.ts`:
- Around line 43-55: The test missing regression cases is in the "versioned docs
map to /docs/:slug" block using the toEndpoint() helper; add assertions that
toEndpoint('/docs'), toEndpoint('/developer'), and toEndpoint('/v1') each return
'/docs/:slug' so those root docs paths don't regress to null. Update the test
cases inside the same test function (referencing toEndpoint) to include these
three expectations.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 13d49542-fac1-4eea-9ddd-aa4dec9537d3

📥 Commits

Reviewing files that changed from the base of the PR and between 54995c1 and 7a42e89.

📒 Files selected for processing (2)
  • packages/chronicle/src/server/plugins/telemetry.test.ts
  • packages/chronicle/src/server/plugins/telemetry.ts

Comment thread packages/chronicle/src/server/plugins/telemetry.ts Outdated

requestCounter.add(1, { method, endpoint, status: res.status })
requestDuration.record(duration, { method, endpoint, status: res.status })
if (res.status !== 404) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We can just remove the endpoint. We shouldn't remove the metrics capture.

@rohilsurana rohilsurana merged commit 133698b into main Jun 10, 2026
7 of 9 checks passed
@rohilsurana rohilsurana deleted the fix/drop-metrics-for-unmatched-routes branch June 10, 2026 10:42
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.

feat: drop or normalize non-matching routes from metrics to prevent cardinality breach

2 participants