Introduce a way to provide custom micrometer metrics tags #34434
Conversation
|
/cc @brunobat (micrometer), @ebullient (micrometer) |
|
|
||
| Tags configure(Context context); | ||
|
|
||
| interface Context { |
There was a problem hiding this comment.
The reason for using this additional interface is that it allows us to introduce new items items to the API without breaking anything (as opposed to adding a new parameter to configure)
| * <p> | ||
| * The implementations of this interface are meant to be registered via CDI beans. | ||
| */ | ||
| public interface AdditionalHttpServerMetricsTagsProvider { |
There was a problem hiding this comment.
I very much dislike this name, so if you have ideas, I'm all ears
There was a problem hiding this comment.
I've updated it after talking to @ebullient
| HttpCommonTags.status(response.statusCode())); | ||
| if (!additionalHttpServerMetricsTagsProviders.isEmpty()) { | ||
| AdditionalHttpServerMetricsTagsProvider.Context context = new DefaultContext(requestMetric.request()); | ||
| for (int i = 0; i < additionalHttpServerMetricsTagsProviders.size(); i++) { |
There was a problem hiding this comment.
The reason for using foreach here is that I want to avoid creating garbage on this path
| if (!additionalHttpServerMetricsTagsProviders.isEmpty()) { | ||
| AdditionalHttpServerMetricsTagsProvider.Context context = new DefaultContext(requestMetric.request()); | ||
| for (int i = 0; i < additionalHttpServerMetricsTagsProviders.size(); i++) { | ||
| allTags = allTags.and(additionalHttpServerMetricsTagsProviders.get(i).configure(context)); |
There was a problem hiding this comment.
What if configure throws a RuntimeException.. This was not the case until now, as tags were fix and resolved by Quarkus itself.. Propagate only?
There was a problem hiding this comment.
Good question. @ebullient what's the best practice in this case?
There was a problem hiding this comment.
I'd say skip adding the new tags.
|
Given there is space between Vert.x and Quarkus here.. Can we have one interface for http tag contributors with default empty methods for hooking in server and client metrics? Just an idea, maybe it is bad. Could be discovered/anchored off of the HttpBinderConfig (which hosts both server things and client things), as http metrics are emitted in a few different places. Server side is straight-up. Client side is a PITA... |
|
I'll have a look |
|
I think it's would actually be kind of weird to have the same CDI bean for configuring client and server tags. I would personally prefer having two different interfaces. |
|
✔️ The latest workflow run for the pull request has completed successfully. It should be safe to merge provided you have a look at the other checks in the summary. |
Closes: #33313