-
Notifications
You must be signed in to change notification settings - Fork 961
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 shortcuts to assign dynamic tags to Meters #4097
Add shortcuts to assign dynamic tags to Meters #4097
Conversation
/cc: @gavlyukovskiy |
This looks absolutely awesome! |
micrometer-core/src/main/java/io/micrometer/core/instrument/Timer.java
Outdated
Show resolved
Hide resolved
micrometer-core/src/main/java/io/micrometer/core/instrument/Counter.java
Outdated
Show resolved
Hide resolved
Closes micrometer-metricsgh-535 See micrometer-metricsgh-4092 Co-authored-by: qweek <alnovoselov@mail.ru>
e4f6b4f
to
3d02c6d
Compare
@gavlyukovskiy @qweek I modified this a little bit, what do you think? |
* @param tags Tags to attach to the Meter about to be registered | ||
* @return A new or existing Meter | ||
*/ | ||
T withTags(Iterable<? extends Tag> tags); |
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.
Why withTags()
versus the more commonly used elsewhere tags()
?
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.
I don't want to speak for @jonatan-ivanov, but I think the idea was to differentiate from the tags()
method on the builders because those return a builder and can chain, whereas this method returns a registered Meter
and cannot be chained to add more tags. I spent some time trying to think of better naming, but I was not able to come up with something. This is definitely something we're hoping to get feedback on and improve. Ideally, we would not change the naming after RC1, but it may be worth making an exception in this case if we cannot come up with something better before merging this.
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.
Yeah, tags
might be confusing since that is on the Builder
as well as MeterProvider
.
This feature is a shortcut to create/get certain meters in a simpler way using the builder of that meter but without the need of recreating the builder every time.
This is what dynamic tagging looks like before this change (a bit longer and multiple builder instances will be created):
And this is what you can do after (a bit shorter and one builder instance will be created):
Closes gh-535
See gh-4092