Skip to content

Add --exclude-metrics-path to filter request paths from metrics#213

Open
lewispb wants to merge 2 commits into
mainfrom
exclude-metrics-path
Open

Add --exclude-metrics-path to filter request paths from metrics#213
lewispb wants to merge 2 commits into
mainfrom
exclude-metrics-path

Conversation

@lewispb
Copy link
Copy Markdown
Member

@lewispb lewispb commented May 5, 2026

Summary

Adds a per-service --exclude-metrics-path flag on deploy so apps can opt specific paths out of the Prometheus request counters and in-flight gauge. Matches are exact and the flag is repeatable.

Motivation

High-volume healthcheck traffic from upstream load balancers and uptime monitors both inflates the metrics pipeline (volume → cardinality, storage, scrape cost) and dominates aggregate measures like request rate, latency percentiles, and error rates — so the dashboards no longer reflect real user traffic.

Behavior

  • Exact path match against r.URL.Path, repeatable via --exclude-metrics-path /up --exclude-metrics-path /healthz (or comma-separated)
  • Persists in ServiceOptions alongside other per-service config; legacy state without the field deserializes fine
  • Excluded requests are still logged — only kamal_proxy_http_requests_total, kamal_proxy_http_request_duration_seconds, and kamal_proxy_http_in_flight_requests are skipped

Test plan

  • go test ./... passes
  • go vet ./... clean
  • New unit test for ServiceOptions.IsMetricsExcluded
  • New integration-style test asserting Service.ServeHTTP flips the request-context flag for matching paths only
  • Manual smoke: deploy a service with --exclude-metrics-path /up, hit /up and /other, confirm /up is absent from /metrics output

lewispb and others added 2 commits May 5, 2026 16:51
Lets services opt specific paths (typically health checks from upstream
load balancers or uptime monitors) out of the Prometheus request and
in-flight metrics. Matches are exact, can be repeated, and only suppress
metrics — request logs are still emitted.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The motivation for `--exclude-metrics-path` is that high-volume
healthcheck traffic distorts aggregate metrics (request rate, latency
percentiles, error rates) and inflates the metrics pipeline — not that
the output is "noisy".

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings May 5, 2026 16:07
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds a per-service --exclude-metrics-path deploy option so specific request paths (e.g., health checks) can be excluded from Prometheus request counters/duration and the in-flight gauge, reducing metrics noise while keeping request logs intact.

Changes:

  • Persist per-service excluded paths in ServiceOptions and detect exact matches against r.URL.Path.
  • Skip Prometheus request tracking (counters/histogram) and in-flight gauge updates for excluded paths.
  • Add CLI flag, documentation, and unit/integration-style tests for the new behavior.

Tip

If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Show a summary per file
File Description
README.md Documents the new per-service flag and exact-match behavior for excluding paths from metrics.
internal/server/service.go Stores excluded paths and marks matching requests to skip metrics; avoids in-flight tracking for excluded requests.
internal/server/service_test.go Adds coverage for IsMetricsExcluded and validates the request-context flag is set for excluded paths.
internal/server/logging_middleware.go Conditionally skips TrackRequest when the request context indicates metrics should be excluded.
internal/cmd/deploy.go Introduces the repeatable --exclude-metrics-path deploy flag wired into ServiceOptions.

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

@kevinmcconnell
Copy link
Copy Markdown
Collaborator

@lewispb we should probably always exclude health check requests from metrics. We could exclude anything where IsHealthCheckRequest is true. Then it would do the right thing automatically, and we would avoid introducing additional configuration options. Seems reasonable to say that metrics never include healthcheck requests by definition, I think.

Would doing that cover your use case? Or do you have a real need to exclude multiple other paths too?

(btw I'm still out for the next couple weeks so am not working on this; I just happened to see this PR in passing ;))

@lewispb
Copy link
Copy Markdown
Member Author

lewispb commented May 6, 2026

No rush @kevinmcconnell, we can chat about it at the meetup if you like.

We could exclude some common health check endpoints automatically, but it's hard to know what any particular app may consider as a health check (we have several that we use internally for different reasons).

It's also an area where opinions differ - health checks are real requests, and there are some legitimate cases for actually monitoring the performance of them.

Happy to chat in person anyways.

@kevinmcconnell
Copy link
Copy Markdown
Collaborator

Sounds good @lewispb! let’s chat when we meet 👍

We could exclude some common health check endpoints automatically, but it's hard to know what any particular app may consider as a health check

Just to clarify that one part, the proxy already knows the app’s own healthcheck path from the deployment. We have some special handling on that path already (for maintenance mode; to ensure downstream LBs don’t think mark an app in maintenance as unhealthy). So that’s the same endpoint that I would propose excluding, at least by default. We wouldn’t be matching on common paths, we’d be using the one that’s already defined.

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