Add opt-in target host and port observation tags - #3546
Conversation
* Add a convention that emits the configured target host and explicit port. * Keep the default low-cardinality key set unchanged. * Cover explicit, omitted, and URL-less targets and document opt-in usage. Fixes OpenFeign#2630
f2be94e to
6e282cf
Compare
velo
left a comment
There was a problem hiding this comment.
Nice addition — subclassing DefaultFeignObservationConvention and wiring in through MicrometerObservationCapability's existing customConvention extension point is exactly the right seam for this. A couple of things before merging:
-
supportsContextis overridden with a body identical to the inherited default (context instanceof FeignContext). The PR description says it's there so japicmp can validate the new convention, but that doesn't apply — this is a brand-new class with no prior release baseline, and the siblingDefaultFeignObservationConventionit extends doesn't override this either. Please drop the override; inheritance already does the right thing. -
targetUri()catchesURISyntaxException | UnsupportedOperationExceptionaroundfeignTarget().url(), butRequestTemplate.feignTarget()can returnnullfor any RequestTemplate not routed throughRequestTemplateFactoryResolver(e.g. a hand-built RequestTemplate from a custom Client or test double). That path would NPE uncaught, which defeats the class's stated guarantee that "a missing or invalid url never breaks the observation." Please null-checkfeignTarget()explicitly (or widen the catch) so that guarantee actually holds.
Smaller, non-blocking:
- No test exercises the
URISyntaxExceptionbranch intargetUri()— worth a case for parity with the other catch arm. - README typo: "urless" → "url-less".
Happy to merge once 1 and 2 are addressed.
Summary
TargetHostAndPortFeignObservationConventionnet.peer.hostand an explicitly configurednet.peer.portsupportsContextis declared explicitly with the existing semantics so JApiCmp can validate the new public convention.Testing
./mvnw -Pdev -Dtoolchain.skip=true -pl micrometer -am verifyFixes #2630