[henriquejsza]

Summary

• add an opt-in TargetHostAndPortFeignObservationConvention
• emit net.peer.host and an explicitly configured net.peer.port
• preserve the existing default low-cardinality key set
• safely omit target tags for URL-less or invalid targets
• document activation, port semantics, and cardinality considerations

supportsContext is declared explicitly with the existing semantics so JApiCmp can validate the new public convention.

Testing

• ./mvnw -Pdev -Dtoolchain.skip=true -pl micrometer -am verify

Fixes #2630

[velo]

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:

1. supportsContext is 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 sibling DefaultFeignObservationConvention it extends doesn't override this either. Please drop the override; inheritance already does the right thing.

2. targetUri() catches URISyntaxException | UnsupportedOperationException around feignTarget().url(), but RequestTemplate.feignTarget() can return null for any RequestTemplate not routed through RequestTemplateFactoryResolver (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-check feignTarget() explicitly (or widen the catch) so that guarantee actually holds.

Smaller, non-blocking:

• No test exercises the URISyntaxException branch in targetUri() — worth a case for parity with the other catch arm.
• README typo: "urless" → "url-less".

Happy to merge once 1 and 2 are addressed.

[velo]

Friendly ping: the items from the 09-04 review are still open:

1. Drop the redundant supportsContext override.
2. Null-check feignTarget() in targetUri(). The parent DefaultFeignObservationConvention.getLowCardinalityKeyValues already calls requestTemplate.feignTarget().type() without a guard, so a null target throws in super first. Please handle that too if the goal is "never breaks the observation".
3. Add a test for the URISyntaxException branch, and fix "urless" → "url-less" in the README.

Also: @since 13.15 should be 13.16. And a Prometheus registry expects the same tag keys for every meter with the same name, so clients without an explicit port (they emit no net.peer.port) may conflict with clients that have one. Please emit a placeholder value such as UNKNOWN, or document this.

[henriquejsza]

Thanks for the review, and sorry for the delay. All points are addressed in the new commit:

1. Dropped the redundant supportsContext override.
2. targetUri() now null-checks feignTarget(). I also guarded the parent: DefaultFeignObservationConvention derived net.peer.client_name from feignTarget().type() with no check, so it now falls back to UNKNOWN when there is no target.
3. Added optInConventionIsSafeWhenTargetUrlIsNotAValidUri for the URISyntaxException branch, plus optInConventionIsSafeWhenFeignTargetIsNull.
4. README: "urless" -> "url-less", and @since is now 13.16.
5. net.peer.port is always emitted next to the host, with UNKNOWN when the url has no explicit port, so every meter from this convention has the same tag keys. README and tests updated.