Aggregate Micrometer "uri" label values under uri template #1493
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…abels
* e.g. "/get/{id}" rather than "/get/123"
* this provides better aggregation, protecting the time series database
martinacat
commented
Aug 26, 2021
martinacat
commented
Aug 26, 2021
martinacat
commented
Aug 26, 2021
martinacat
changed the title
|
Might be of interest for @velo and @tommyk-gears 😊 |
martinacat
changed the title
velo
approved these changes
Sep 1, 2021
velo
reviewed
Sep 1, 2021
| } | ||
|
|
||
| @Test | ||
| public void clientMetricsHaveUriLabel() { |
There was a problem hiding this comment.
Can we move this tests to the AbstractMetricsTestBase, likely this will fail for dropwizard-metrics which would need to be made consistent.
There was a problem hiding this comment.
I see - I originally placed it in AbstractMetricsTestBase, however I realised that dropwizard-metrics don't attach uri and thought that was a design decision. If necessary I can add uri on dropwizard 4 and 5 - is that what we would like? Thanks!
There was a problem hiding this comment.
I have moved the tests in AbstractMetricsTestBase and made dropwizard-metrics 4 & 5 consistent :)
velo
approved these changes
Sep 1, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
Use path expression rather than explicit value for Micrometer
urilabelsThis new default means that the value of the
urimetric label becomes/get/{id}rather than/get/123urilabelMotivation
This change provides better aggregation, protecting time series databases such as Prometheus from a large influx of time series (high-cardinality metrics).
Currently - as noted in #1481 - enabling feign client metrics from config on clients with a parametrized urls creates a scenario where a new time series is produced for each value of the provided parameter. This provides extremely high-cardinality metrics at a fast rate endangering the underlying time series database. (Resolves #1481, related to #873)
When traffic is significant, collecting client metrics with an uri expression such as
reservation/{reservationId}can generate tens of thousands of time series (one for each value of the label, a potentially infinite set), putting Prometheus at risk. Enabling OOTB metrics without being aware of the high cardinality of some metrics is quite easy to do, so we thought we could protect some users by changing the default behaviorTesting
Questions
Thank you for your time! 😊