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

Provide a way to give custom metric name to MicrometerHttpRequestExecutor #3706

Open
beatfreaker opened this issue Mar 22, 2023 · 11 comments · May be fixed by #3734
Open

Provide a way to give custom metric name to MicrometerHttpRequestExecutor #3706

beatfreaker opened this issue Mar 22, 2023 · 11 comments · May be fixed by #3734
Assignees
Labels
enhancement A general enhancement waiting for feedback We need additional information before we can continue
Milestone

Comments

@beatfreaker
Copy link

Please describe the feature request.
Currently the metric name for MicrometerHttpRequestExecutor is fixed to httpcomponents.httpclient.request, requesting to provide a way to give custom metric name like we have such option in OkHttpMetricsEventListener

Rationale
We have different libraries which use different HTTP client libraries to make API calls i.e RestTemplate, OkHttp and HttpClient

Now we want to make sure that all the metrics generated by all these three clients are reported with same metric name. I know we can customize spring-boot's RestTemplate and OkHttp client metric name to httpcomponents.httpclient.request so that all metrics are reported under httpcomponents.httpclient.request but we like to name the metric as http.client.requests so need an option to provide custom metric name to MicrometerHttpRequestExecutor

Additional context

@jonatan-ivanov
Copy link
Member

jonatan-ivanov commented Mar 22, 2023

Since okhttp has it I think it would make sense to add it to this one too. Are you up to filing a PR for this?

In the meantime: you can customize any Meter using a MeterFilter, in your case:

new MeterFilter() {
    @Override
    public Meter.Id map(Meter.Id id) {
       if (id.getName().equals("httpcomponents.httpclient.request")) {
          return id.withName("http.client.requests");
       }
       return id;
    }
}

See the docs: https://micrometer.io/docs/concepts#_transforming_metrics

@jonatan-ivanov jonatan-ivanov added enhancement A general enhancement and removed waiting-for-triage labels Mar 22, 2023
@jonatan-ivanov jonatan-ivanov added this to the 1.next milestone Mar 22, 2023
@beatfreaker
Copy link
Author

@jonatan-ivanov Sure, you can assign this issue to me. I will work on a PR to resolve it.

@beatfreaker
Copy link
Author

Hi @jonatan-ivanov ,

Should I create PR against main branch?

Also is it fine to implement a similar capability in MicrometerHttpClientInterceptor?

@jonatan-ivanov
Copy link
Member

Should I create PR against main branch?

Yes, since this is a new functionality (it's not really a bug: a MeterFilter can be used to change the name).

Also is it fine to implement a similar capability in MicrometerHttpClientInterceptor?

I've just took a deeper look and it seems MicrometerHttpRequestExecutor is using the ObservationOrTimerCompatibleInstrumentation so it can create an Observation or a Timer. If an Observation is created (and the meter handler is registered) the created Timer will get its name from the ObservationConvention but if ObservationOrTimerCompatibleInstrumentation creates a Timer instead of an Observation, it gets its name from METER_NAME. I'm thinking adding a meterName(...) method to the builder would make this somewhat confusing since it will not have any effect in case an Observation is created. Though the same is true for the convention (does not have any effect if a Timer is created directly) so maybe adding some javadoc that describes this is fine.

MicrometerHttpClientInterceptor seems to be a simpler case, I think it's ok to add a new ctor for this.

@shakuzen What do you think?

beatfreaker pushed a commit to beatfreaker/micrometer that referenced this issue Apr 1, 2023
…equests that goes through HttpClient and HttpAsyncClient

Resolved micrometer-metricsgh-3706
beatfreaker pushed a commit to beatfreaker/micrometer that referenced this issue Apr 4, 2023
# Conflicts:
#	micrometer-core/src/main/java/io/micrometer/core/instrument/binder/httpcomponents/MicrometerHttpRequestExecutor.java
#	micrometer-core/src/main/java/io/micrometer/core/instrument/binder/httpcomponents/hc5/MicrometerHttpRequestExecutor.java
@cachescrubber
Copy link
Contributor

Exactly what I was looking for. Is there a chance the PR will make it into 1.11?

@cachescrubber
Copy link
Contributor

Just a note to other people investigating how to combine different instrumentations under a unified metric name: The suggested MeterFilter to change the metric name lead me directly to #877, which is basically an unresolved micrometer / prometheus integration issue. Both instrumentations must use the exact same tags / dimensions, otherwise the data of one of them is not exported to prometheus. I wanted to combine cxf.client.requests and httpcomponents.httpclient.request which are both Timer for http request durations.

@marcingrzejszczak
Copy link
Contributor

Sorry for the gigantic delay, we are refining our backlog and will do our best to give some feedback on the PR and this issue ASAP

@cachescrubber
Copy link
Contributor

Hi @marcingrzejszczak,

the PR linked to this issue targets the now depreciated MicrometerHttpClientInterceptor and MicrometerHttpRequestExecutor. You probably would not add features to deprecated implementations I guess.

The current implementation could be customized, albeit not very user-friendly. On order to use a custom metric, DefaultApacheHttpClientObservationConvention has to be extended:

    static class CustomApacheHttpClientObservationConvention extends DefaultApacheHttpClientObservationConvention {
        @Override
        public String getName() {
            return "custom.httpclient.request";
        }
    }
    private final ObservationExecChainHandler customHandler = new ObservationExecChainHandler(observationRegistry, new CustomApacheHttpClientObservationConvention());

I'm happy to come up with a PR providing a more easily customizable solution. Is there a best practice / convention on how to implement customizable ObservationConvention which would integrate nicely in the project?

@marcingrzejszczak
Copy link
Contributor

Actually what you did with the observation conventions is exactly what we suggest. You can read about that in the docs.

@marcingrzejszczak marcingrzejszczak added the waiting for feedback We need additional information before we can continue label Jan 10, 2024
@cachescrubber
Copy link
Contributor

Ok. I asked because there are still ObservationConvention having for example a constructor parameter for the metric name. DefaultOkHttpObservationConvention for example.

Would It be feasible to have a CustomizableApacheHttpClientObservationConvention extends DefaultApacheHttpClientObservationConvention with support for a custom metric name, extra tags and some options like exportTagsForRote?

@marcingrzejszczak
Copy link
Contributor

Ah ok - yes, I understand now, IMO it makes sense, WDYT @shakuzen @jonatan-ivanov ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A general enhancement waiting for feedback We need additional information before we can continue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants