Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ability to exclude paths from metrics #46

Merged
merged 1 commit into from
Jul 21, 2023

Conversation

kppullin
Copy link

@kppullin kppullin commented Feb 4, 2021

This is largely a clone of the Actix Logger middleware's
exclude and exclude_regex APIs.

This is largely a clone of the Actix Logger middleware's
`exclude` and `exclude_regex` APIs.
@lerouxrgd
Copy link

I feel that having a valid_paths RegexSet (instead of an excluding one) that would be optional would be better ?

@kppullin
Copy link
Author

I feel that having a valid_paths RegexSet (instead of an excluding one) that would be optional would be better ?

Possibly - can you describe your thinking?

A couple thoughts favoring the exclude style:

  • It follows the design/convention of how the Actix web logger is configured to exclude paths: https://actix.rs/actix-web/actix_web/middleware/struct.Logger.html#method.exclude_regex
  • In my case, there's just a couple paths I'd like to exclude. If I had to remember to include all new paths, I'd likely forget to do so. I'd only realize they're missing at the point I had a reason to go looking for them, and at that point I have lost all historical data (sure, I could group all paths under a common prefix and include the prefix... but not sure how this would play out more generally).

@lerouxrgd
Copy link

I see good points indeed.

I was thinking that by excluding paths it might easier for someone on the web to spam your server with random paths and completely pollute your registry (and Prometheus), as it can be tricky to have a regex excluding all bad possibilities (regex dosen't have look-ahead and backreferences for instance).

@kppullin
Copy link
Author

Interesting point - I had assumed that if a path isn't registered, then no metric would be recorded. I just tested and confirmed that unknown paths are recorded. I compared this with a Spring Boot service we run and it does not record the metric.

To me this feels like a denial-of-service risk and should be handled regardless and independent of this PR. As you point out, it seems trivial to bloat your service's metrics and have that spill over and run your prometheus instance out of storage.

@riordanp
Copy link
Contributor

Another use case for this is to exclude the /metrics endpoint from the metrics output itself as it adds a bit of noise, although the DoS risk is more pressing. What is needed to get this merged?

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.

5 participants