discuss opentelemetry-util-http

[owais]

opentelemetry-util-http introduced a change in #661 which caused a bug #744. We've a temporary solution in #745. Opening this issue to discuss the proper long term solution.

Some questions:

1. Do we want requests and other similar instrumentations to explicitly request HttpClientInstrumentor to set IP on their span by setting the span in context?

What if HttpClientInstrumentor always added IP to whatever span was active in the current context? It would be far simpler and wouldn't need any explicit dependencies on opentelemetry-util-http. Can anyone think of any downsides to this? In some cases, it may not be the same span as the one created by requests lib (another instrumentation in the middle) but it is almost guaranteed to be a client span.

I'd even take this a bit further and update HttpClientInstrumentor to do the following:

1. check if there is a span in current context.
2. if the span is a CLIENT span, add IP (and any other metadata) to it.
3. else if no span is found or the span is not a CLIENT span then create a new CLIENT span and add all metadata to that.

This should take care of all cases without requiring explicitly cooperation between different instrumentations.

2. Should HttpClientInstrumentor be it's own package?

Because of how our tooling works with instrumentation packages, we have to move the opentelemetry-util-http package under instrumentations/ directory. We might as well create a new package for httplib instrumentor and leave the shared util code in opentelemetry-util-http. One upside to this is that people can easily discover instrumentation for httplib which will be important especially if we go with proposal in the previous section.
As long as we want to ship an instrument
I can go either

[owais]

//cc @open-telemetry/python-approvers @Oberon00

[Oberon00]

Because of how our tooling works with instrumentation packages, we have to move the opentelemetry-util-http package under instrumentations/ directory.

I initially had created a new, separate package under instrumentation/ on the PR without touching util-http but I got a request to move it to the util-http package: #661 (comment) See commit b02f1d3 on that PR.

Do you want me to submit a PR to revert that commit, i.e. move it to a separate package again?

[Oberon00]

What if HttpClientInstrumentor always added IP to whatever span was active in the current context? It would be far simpler and wouldn't need any explicit dependencies on opentelemetry-util-http. Can anyone think of any downsides to this? In some cases, it may not be the same span as the one created by requests lib (another instrumentation in the middle) but it is almost guaranteed to be a client span.

I did the explicit request because the IP should be set on the HTTP span. It probably doesn't hurt if it is also set on another span, but I wanted to have it on the HTTP span in particular. That is what the semantic conventions recommend.
Also, in theory, the HTTP library might e.g. dispatch to another thread where there is no span active.

[Oberon00]

As far as future directions go, I think one way to go would be to have the HttpClientInstrumentor be a "full" instrumentation that does not set IPs on other spans but creates spans for httplib activity itself (including IP but also http.target, ...). That may allow to get rid of the separate urllib sensor (unless we want to support protocols other than HTTP) and considerably shrink the requests instrumentation.

[owais]

Do you want me to submit a PR to revert that commit, i.e. move it to a separate package again?

I think we should do that. That would allow us to re-enable this instrumentation (add back the entry-point) and later decide if we want to change the HTTP instrumentation somehow.

[owais]

As far as future directions go, I think one way to go would be to have the HttpClientInstrumentor be a "full" instrumentation that does not set IPs on other spans but creates spans for httplib activity itself (including IP but also http.target, ...). That may allow to get rid of the separate urllib sensor (unless we want to support protocols other than HTTP) and considerably shrink the requests instrumentation.

I agree it'd be better but we'll end up with multiple CLIENT spans which is a bigger problem. If the HttpClientInstrumentor could however be a little smart about it, I think we could make it work in most situations. It could first check if the span in current context is already a CLIENT span and add attributes to that. If a span does not exist or the span is of type INTERNAL, then it could create a new CLIENT span. It is very unlikely (impossible?) that a higher level wrapper over httplib would use httplib for non-http spans so we can safely assume if a CLIENT span exists in the context, it would represent an HTTP op.

[Oberon00]

I agree it'd be better but we'll end up with multiple CLIENT spans which is a bigger problem.

Hmm, probably it would be wise to wait for (or participate in) the HTTP semantic convention decisions being made in that regard (open-telemetry/oteps#174, open-telemetry/opentelemetry-specification#1747)

[Oberon00]

One likely outcome will be that every redirect/retry needs to be its own span, so that you do not have multiple server-span siblings under the same client span, see @lmolkova's insightful comment at open-telemetry/opentelemetry-specification#1747 (comment)

[lmolkova]

Thanks @Oberon00 for sharing. @owais we just started discussing HTTP retries on Tue 4 pm PT instrumentation SIG meeing, please come and let's discuss - we need to define a consistent story across languages/instrumentations.