Add a way to access request context data in HttpServerMetricsTagsContributor
#47887
Conversation
|
/cc @brunobat (micrometer), @ebullient (micrometer) |
adutra
left a comment
adutra
left a comment
There was a problem hiding this comment.
Thank you for the quick implementation!
...er/runtime/src/main/java/io/quarkus/micrometer/runtime/HttpServerMetricsTagsContributor.java
Outdated
Show resolved
Hide resolved
cescoffier
left a comment
cescoffier
left a comment
There was a problem hiding this comment.
I'm not a big fan of reading duplicated content after requests, but we can find a better way later.
|
Yeah, we will likely need a better solution in the future. The good thing with this, is that the use of DC is not exposed to the user |
geoand
added
the
triage/waiting-for-ci
This comment has been minimized.
This comment has been minimized.
``` - Refactor context local data handling across Micrometer integration to improve type safety with generics. - Simplify and clean up code by removing unnecessary type casts in `ContextLocalTag`. ```
|
@gastaldi actually now that I think of it we don't want that because it ties the type of the key and the type of value. |
No, they are totally unrelated. I can write |
|
👌 |
Status for workflow
|
quarkus-bot
bot
added
kind/enhancement
| public class PathTemplateResource { | ||
| @GET | ||
| public String get(@PathParam("value") String value) { | ||
| ContextLocals.put("context-local", "val-" + value); |
There was a problem hiding this comment.
This is a bad practice. We should not encourage ppl to add tags from path params. Even if they think the nr of different values are limited, they cannot really control it.
There was a problem hiding this comment.
It might be bad practice, but people shouldn't be limited by accidental technically reasons from doing it.
ThreadLocals should not be used generally, but that doesn't mean we should prevent their use.
There was a problem hiding this comment.
Fair, but this should come with a big warning that they will shoot themselves in the foot if they don't consider cardinatility.
See for example: #users > Are you using path parameters? (metrics)
I think a warning should be placed in the docs and a in the test.
There was a problem hiding this comment.
Because in the end, ppl will complain and we will spend time debugging this stuff.
There was a problem hiding this comment.
I would add we should also place a warning in the Javadoc
There was a problem hiding this comment.
If you want to add something in the various places, I'll gladly approve
There was a problem hiding this comment.
For context: my initial use case is not related to path params, it's related to capturing the tenant ID associated with the request. And tenant IDs are guaranteed to form a finite set, although they could be like a thousand or so. Are you saying that even this use case is bad practice?
There was a problem hiding this comment.
Yes. It must not be used like that.
It will blow the dimensions exponentially. Remember that for each tenant id you will have also different metrics per endpoint, response status and method.
Place that data in the span and later query the APM's span attributes and/or correlate with exemplars.
5 participants
HttpServerMetricsTagsContributor#47886