-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Introduced a DynamicValueTag class to determine tag values at runtime #2285
base: main
Are you sure you want to change the base?
Introduced a DynamicValueTag class to determine tag values at runtime #2285
Conversation
6603ddd
to
843809a
Compare
@jackhammer2k and @jkschneider do you have any objections about this PR? |
micrometer-core/src/main/java/io/micrometer/core/instrument/Tag.java
Outdated
Show resolved
Hide resolved
I really like the PR and could only find little things in the Javadocs. |
} | ||
|
||
@Override | ||
public boolean equals(@Nullable Object o) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MeterRegistry
stores all meters in a map, where Meter.Id
is used as key. But this means that Meter.Id.hashcode()
or Meter.Id.equals()
will be called and must not change over time. But because
Meter.Id.hashcode()
calls Tags.hashcode()
which calls DynamicValueTag.hashcode()
, your implementation must only take the key into account to show the required stable bahavior. However I'm not completely sure if its an issue to not take the value into account in equality checks. Maybe thats the issue jkschneider was talking about.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's see what @jkschneider or maybe @izeye think about the equals
check.
@SimonScholz Sorry for the big delay. Could you please give use some concrete use-cases with examples? The thing that you are doing in the description seems dangerous since it attaches high-cardinality tags to the meters ( Also, could you please check if the new |
If you would like us to look at this PR, please provide the requested information. If the information is not provided within the next 7 days this PR will be closed. |
@jonatan-ivanov Thanks for asking. Meanwhile I left the company, where we've had the need for this feature, but you can find my description of the need here: #2223 (comment) Basically we used to visualize metrics exposed by a third party (Resilience4J) in our dashboards and wanted to attach certain dynamic tags (e.g., obtain headers from the originating thread local request) to metrics not owned by us. |
Hi,
In our application we'd love to have common tags for each and every metric, but these common tags might change during runtime. Passing these dynamic common tag values to each and every counter or timer we create is really inconvenient!
This PR is related to this issue: #2223
I was also already able to test this new feature, by adding the following code to the
io.micrometer.boot2.samples.components.ServiceLevelObjectiveConfiguration
:Then when I started the
io.micrometer.boot2.samples.LoggingRegistrySample
application together with theMeterRegistryCustomizer
mentioned above, I was able to see the differentcurrentTimeMillis
in the system out.Having this feature would save us a lot of effort, because otherwise we would need to pass these dynamic tag values to each and every counter or timer we create in our code.