Adds a client tag to the default micrometer observation #2333
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
iCiaran
commented
Feb 21, 2024
micrometer/src/main/java/feign/micrometer/FeignObservationDocumentation.java
Outdated
Show resolved
Hide resolved
iCiaran
commented
Feb 21, 2024
Comment on lines
98
to
104
| private void assertTags() { | ||
| Id requestsId = meterRegistry.get(METER_NAME).meter().getId(); | ||
|
|
||
| assertMetricIdIncludesMethod(requestsId); | ||
| assertMetricIdIncludesURI(requestsId); | ||
| assertMetricIdIncludesStatus(requestsId); | ||
| assertsMetricIdIncludesClient(requestsId); | ||
| } |
There was a problem hiding this comment.
This might not be the right place for these tests, however since they're only applicable to the micrometer observation meters adding to AbstractMetricsTestBase.java would require some changes to the dropwizard 4/5 and MicrometerCapability tests too so I've avoided that for now.
iCiaran
force-pushed
the
observation-capability-client-tag
branch
from
4401f37
to
3d0621b
Compare
velo
approved these changes
Feb 29, 2024
| @@ -48,14 +49,16 @@ public String getContextualName(FeignContext context) { | |||
|
|
|||
| @Override | |||
| public KeyValues getLowCardinalityKeyValues(FeignContext context) { | |||
| String templatedUrl = context.getCarrier().requestTemplate().methodMetadata().template().url(); | |||
| RequestTemplate requestTemplate = context.getCarrier().requestTemplate(); | |||
There was a problem hiding this comment.
Hey guys, just came across this PR, as I needed to include the client name and was writing a similar implementation, but noticed something in this change. Isn't context.getCarrier() nullable and shouldn't we check for null like it is done in getMethodString(@Nullable Request request) a bit further down?
velo
pushed a commit
that referenced
this pull request
Oct 7, 2024
* Add client tag to default micrometer observation * Update license headers * Update tag name to clientName
velo
pushed a commit
that referenced
this pull request
Oct 8, 2024
* Add client tag to default micrometer observation * Update license headers * Update tag name to clientName
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Include a client tag on the default feign micrometer observation, so it's added to the
http.client.requestsmeters, in the same way it's added to thefeign.*meters.This is useful for applications with more than one feign client, to avoid merging unrelated metrics, and improve observability of multiple downstreams which may have different latency etc
Opened to address #2332