-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Micrometer metrics tags extension #1322
Micrometer metrics tags extension #1322
Conversation
@Kuvaldis looks like I may have merged your pull requests out of order. Can you check this one? |
# Conflicts: # micrometer/src/main/java/feign/micrometer/MeteredClient.java # micrometer/src/main/java/feign/micrometer/MeteredEncoder.java
@kdavisk6 All done. |
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.
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.
Overall, I think is a good improvement.
Things I would like to see
a) Not sure if make sense or no, but finally
added to all try/catch blocks.
b) use Tag[]
or Tags
instead of List<Tag>
+ toArray(new Tag[] {})
combo
c) move metric naming to MetricName
giving people one place for all customization needs.
import io.micrometer.core.instrument.MeterRegistry; | ||
import io.micrometer.core.instrument.Tag; | ||
import io.micrometer.core.instrument.Tags; | ||
import io.micrometer.core.instrument.Timer; | ||
import java.io.IOException; |
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.
it would be nice not to change import ordering... but, don't worry too much about it =)
return result; | ||
} | ||
|
||
protected List<Tag> extraSummaryTags(Response response, Type type) { |
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 assume you are using this for some internal metrics (I would be curious to know what you are going =] )
I would prefer to see code to live in the MetricName
interface... that would give a single place for all your tag customization needs.
We would have a few methods, one called requestExtraTags
other one responseExtraTags
, another like exceptionExtraTags
What do you think?
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 extracted tags resolution to a separate extendable class. However, seems like depending on where we create tags (i.e. client, encoder/decoder, method handler) we have really different parameters to build tags upon. I added defaultTags method, which might be helpful for customization in general.
Basically, there is |
is there any example depicting this use case? |
* Micrometer metrics tags extension * Merge parent * Fix license * Add tags to response code counter metric * Fix format * Improve tags resolution for exceptions, decrease amount of similar methods * Make MeteredEncoder timer and counter methods protected * Fix formatting * Add finally blocks Co-authored-by: Nikolay Fadin <[email protected]> Co-authored-by: Kuvaldis <a1N9u8t9I1k> Co-authored-by: Marvin Froeder <[email protected]>
* Micrometer metrics tags extension * Merge parent * Fix license * Add tags to response code counter metric * Fix format * Improve tags resolution for exceptions, decrease amount of similar methods * Make MeteredEncoder timer and counter methods protected * Fix formatting * Add finally blocks Co-authored-by: Nikolay Fadin <[email protected]> Co-authored-by: Kuvaldis <a1N9u8t9I1k> Co-authored-by: Marvin Froeder <[email protected]>
Following Micrometer features added:
Fixes #1301