-
Notifications
You must be signed in to change notification settings - Fork 906
Description
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:
- check if there is a span in current context.
- if the span is a CLIENT span, add IP (and any other metadata) to it.
- 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