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

[FEATURE] Group unhandled paths #10

Closed
wants to merge 2 commits into from
Closed

[FEATURE] Group unhandled paths #10

wants to merge 2 commits into from

Conversation

tsotnikov
Copy link
Contributor

In order to reduce cardinality of prometheus metrics and labels,
this pull request adds an option to group all metrics that do not match any route.

This solves the problem of random path requests generating unwanted metrics (each requested path generates around 23 lines of metrics), which could potentially be a big issue if exposed to the internet.

@tsotnikov tsotnikov requested a review from perdy February 9, 2020 00:33
@tsotnikov
Copy link
Contributor Author

@perdy any comment on this PR?

@perdy
Copy link
Owner

perdy commented Apr 28, 2020

Thanks for your PR.

I'm not sure if that's the best solution for the case you're presenting. Your issue here is that you want to filter those requests that don't match any route, but instead of filtering them you're pushing them into a metric that I don't know if it's gonna be useful for the general use case.

I agree that those unwanted requests generate so much noise and it's desirable to filter them but do we need to store a metric that counts every one of them?

This also leads me to another question: do we need a blacklisting filtering in this middleware?

@tsotnikov
Copy link
Contributor Author

tsotnikov commented Apr 28, 2020

I am ok also with dropping those metrics.

In my initial thought I considered that it could be relevant to have some info about the unwanted requests (at least the counters), but in a production setup this could be extracted from other sources like nginx.

So we definitely need at least this simple form of blacklisting to drop the metrics for requests that do not match any path.

About a more granular blacklisting (matching by method and/or path), it could be useful for some use case do drop some metrics that you do not care about, so in my opinion is a nice feature to have but not essential (in the worst case the number of metrics is limited to the paths you have defined in the API).

If you agree I will create another PR filtering the metrics that do not match any path instead of storing them.

@tsotnikov tsotnikov mentioned this pull request Apr 29, 2020
@tsotnikov tsotnikov closed this Apr 29, 2020
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.

None yet

2 participants