opentelemetry-http: fix url extraction #3274
Conversation
Motivation: Our url extraction is wrong in that it doesn't consider possible absolute-form request targets. We also don't allow users to set a host and port in the common case that it's using origin-form request targets and we can't extract a host name or port. Modifications: Fix the extraction and make it possible to provide a host and port.
bryce-anderson
changed the title
| */ | ||
| public OpenTelemetryHttpRequesterFilter(final String componentName, | ||
| final OpenTelemetryOptions opentelemetryOptions, | ||
| final HostAndPort hostAndPort) { |
There was a problem hiding this comment.
WDYT if instead of requiring HostAndPort at the filter constructor (append) time, we simply move HostHeaderHttpRequesterFilter from being the last filter in the chain to being the first filter in the chain before users or providers can append anything else? That way we won't need any special logic inside ServiceTalkHttpAttributesGetter, and users won't need to worry about how the filter is appended (with or without HostAndPort).
IIRC, the rational for adding HostHeaderHttpRequesterFilter at the end was that users may add their own host header in their filters. Thinking about it now, it should not break their use-case, because they can always override what we set or even disable that filter via SingleAddressHttpClientBuilder#hostHeaderFallback(false) method if they always set it when preparing request. Worst case, they may have an overhead of setting a header and then overriding it, but it's a drop in the ocean. Still worth a highlight in release notes about behavior change if we go that route.
There was a problem hiding this comment.
I think that actually makes things a bit easier we also provide a way to convert the abstract address to a char sequence. We may need to try and parse out the host and port from that, but I think that's probably fine.
There was a problem hiding this comment.
After it ends up being a host header, our regular methods like HttpRequestMetaData.effectiveHostAndPort() should work as expected.
It's a good point about an abstract address, we won't be tight to HostAndPort type at all
Motivation:
Our url extraction is wrong in that it doesn't consider possible absolute-form request targets. We also don't allow users to set a host and port in the common case that it's using origin-form request targets and we can't extract a host name or port.
Modifications:
Fix the extraction and make it possible to provide a host and port.