Add ability to exclude paths from metrics

nlopes / actix-web-prom

Actix-web middleware to expose Prometheus metrics

MIT License

89 stars 56 forks source link

Add ability to exclude paths from metrics #46

Closed kppullin closed 1 year ago

kppullin commented 3 years ago

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

lerouxrgd commented 3 years ago

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

kppullin commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 1 year ago

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?