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 an option to customize meter name for HttpClient metrics #3734

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

beatfreaker
Copy link

Provides an option to customize meter name for metrics generated for requests that goes through HttpClient and HttpAsyncClient

Resolved gh-3706

…equests that goes through HttpClient and HttpAsyncClient

Resolved micrometer-metricsgh-3706
@shakuzen
Copy link
Member

shakuzen commented Apr 4, 2023

Thank you for the pull request. It looks like CircleCI is having trouble loading the CI config. Could you try logging into CircleCI once? I believe that should fix the issue. You may need to update the branch to trigger another build if you can't do it from CircleCI's UI.

Copy link
Member

@shakuzen shakuzen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the pull request. This mostly looks good, but I guess we haven't really figured out what to do about this comment. If you make a test where ObservationRegistry is configured on the instrumentation, I think you will see that your custom name does not get applied to the created Timer.

Chintan Radia added 2 commits April 4, 2023 20:05
# 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
@beatfreaker
Copy link
Author

@shakuzen added tests that results in showing that the custom metric name is not supported when ObservationRegistry is configured.
Also, the Javadoc added on Builder metricName(String name) stating that overridden metric name works only if ObservationRegistry is not configured should be fine right?

@shakuzen
Copy link
Member

It's fine in so much as the behavior is consistent with the JavaDoc, but I don't think it's ideal behavior to be adding at this point where we would expect people in general to be migrating to using the ObservationRegistry in instrumentation and yet we're adding a new configuration method that does not do anything in that case. We could make it so DefaultApacheHttpClientObservationConvention took the configured name so that it would be used for the observation too unless a custom convention is configured. The unfortunate thing is that currently DefaultApacheHttpClientObservationConvention is designed as a singleton, so that's going to be broken if we make a constructor that takes a name. The no arg constructor can continue to use the existing default name, though. I think that might be a better path forward. @jonatan-ivanov what do you think?

@jonatan-ivanov
Copy link
Member

I haven’t checked/tried this but if we want to make this configurable for that instrumentation, we might be able to provide a convention that extends the default and overrides the name to the user provided one but if we do that why isn’t the user the one then who provides it in the first place?

@shakuzen
Copy link
Member

shakuzen commented May 1, 2023

why isn’t the user the one then who provides it in the first place?

They certainly could provide a custom convention, but the main issue is that we will be adding a configuration for name that does nothing in case observation-based instrumentation is used. Users could simply pass a name for Timer-based instrumentation but have to provide a custom convention to do the same with Observation-based instrumentation.

I made this commit demonstrating what I was suggesting. It's certainly not without flaws. We didn't initially intend for default conventions to be used that way. I'm not sure which way is better. (I didn't make the equivalent changes to the hc5 package since it was to demonstrate my suggestion for review first)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide a way to give custom metric name to MicrometerHttpRequestExecutor
3 participants