Skip to content
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 posibility to have extra custom tags in HTTP metrics #33313

Closed
ivansenic opened this issue May 11, 2023 · 22 comments · Fixed by #34434
Closed

Add posibility to have extra custom tags in HTTP metrics #33313

ivansenic opened this issue May 11, 2023 · 22 comments · Fixed by #34434

Comments

@ivansenic
Copy link
Contributor

ivansenic commented May 11, 2023

Description

Currently Quarkus provides out-of-the-box metrics for the client and server HTTP requests. This metric has a fixed set of tags: uri, status, outcome and method.

I would like to have a possibility to register some kind of custom metric provider that can add extra tags for each HTTP requests. The provider can have an interface like:

interface CustomHttpTagsProvider {

   Tags tags(HttpRequest request, HttpResponse response);
   
}

Use case

In some cases, the HTTP APIs are designed in the way that a single endpoint serves for multiple different operations. Take, for example, the command pattern. In such situation you can have an endpoints that can receive different types of the commands. Since metrics are bounded only to uri & method at the moment, in such use-case I can not differentiate and have metrics split for different commands.

I am pretty sure there are other use-cases here.

Implementation ideas

  • Allow users to provide implementation of the tags provider
  • If not provided, fall back to the no-op provider that returns no tags

NOTE: I would be up in contributing here if we agree how this should be done in general and get some guidelines on the preferred solution from Quarkus team.

@quarkus-bot
Copy link

quarkus-bot bot commented May 11, 2023

/cc @ebullient (metrics), @jmartisk (metrics)

@chevaris
Copy link

chevaris commented May 26, 2023

I have exactly the same use case; A POST endpoint that is specified to send different operations in the body. Ideally I would like to have the ability to add a custom tag per operation while keeping existing tags (status...)

I see that there are other very similar requests to this (for instance #29739)

Vert.x allows to define Custom tag providers (vert-x3/vertx-micrometer-metrics#89). Not sure if could be possible to utilize this Vert.x function from Quarkus today. According to this thread (#29739) it is NOT possible

In the end I guess it is something similar to WebFluxTagsContributor when using using Spring Webflux.

@AndreasPetersen
Copy link

Yes please 🙏, much needed.

Right now I have to disable Quarkus' HTTP server filter. Instead I have to make my own filter, or use Vertx's Micrometer filter, which provides easy access to add request specific tags.

@ivansenic
Copy link
Contributor Author

@geoand @ebullient Can we make a step forward here? Seems I am not the only one that needs smth like this..

@geoand
Copy link
Contributor

geoand commented Jun 20, 2023

I personally think it makes sense, but this is up to @brunobat and @ebullient to decide (I can help with the implementation if needed)

@brunobat
Copy link
Contributor

If soo many ppl need this, I think it makes sense to add something. Probably by taking advantage of vert-x3/vertx-micrometer-metrics#89

@geoand
Copy link
Contributor

geoand commented Jun 21, 2023

@ivansenic in your original request, was there a specific reason you included HttpResponse?

@geoand
Copy link
Contributor

geoand commented Jun 21, 2023

Also, we don't seem to use vertx-micrometer-metrics at all in Quarkus

@ivansenic
Copy link
Contributor Author

@ivansenic in your original request, was there a specific reason you included HttpResponse?

I just though this would be a good and clean solution. Similar to the status code tag, you might want to add tags based on the response. I don't have a concrete use-case in my mind, but still I think for completes it would be nice.

@geoand
Copy link
Contributor

geoand commented Jun 21, 2023

Asking because the vertx-micrometer-metrics solution only uses the request - which does seem like the safer thing to do.

@ivansenic
Copy link
Contributor Author

Would agree that is safer.

@geoand
Copy link
Contributor

geoand commented Jun 21, 2023

I can probably have a look at this soon but I'll need to get up to speed on how our metrics on done before commiting to anything :)

@ebullient
Copy link
Member

Also, we don't seem to use vertx-micrometer-metrics at all in Quarkus

It was not feasible to use vertx-micrometer-metrics directly. We can emulate what they're doing, but using that directly was not an option.

@geoand
Copy link
Contributor

geoand commented Jun 21, 2023

That's exactly the kind of context I need to get up to speen on, thanks!

@geoand
Copy link
Contributor

geoand commented Jun 22, 2023

One thing that strikes me as add in the vertx-micrometer-metrics handles this is that there is no control over the event for which the tags are added.

Do we care about that or not?

@ebullient
Copy link
Member

What do you mean?

@geoand
Copy link
Contributor

geoand commented Jun 22, 2023

That the tag names are the same regardless of whether responsePushed, requestReset or responseEnd is called.
I am wondering whether it makes sense to have a different set for each

@ebullient
Copy link
Member

unlikely. If anything, should be a tag. This is broader metrics convention conversation

@geoand
Copy link
Contributor

geoand commented Jun 22, 2023

That definitely simplifies things :)

@geoand
Copy link
Contributor

geoand commented Jun 30, 2023

It seems that I had misread the code and the tags are only relevant for responseEnd.

@geoand
Copy link
Contributor

geoand commented Jun 30, 2023

#34434 is what I have in mind. Feel free to check it out and let me know what you think.
If we agree it's the proper direction, I can add a note to the docs and undraft it.

@ivansenic
Copy link
Contributor Author

Looks good imo..

geoand added a commit to geoand/quarkus that referenced this issue Jun 30, 2023
geoand added a commit to geoand/quarkus that referenced this issue Jul 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants