Fixes #1858: Allow specifying a custom HTTP transport for the otlphttp driver #1859
Conversation
…or the otlphttp driver
arnogeurts-sqills
requested review from
Aneurysm9,
dashpole,
evantorrie,
jmacd,
MrAlias,
paivagustavo and
XSAM
as code owners
|
|
| // Keep it in sync with golang's DefaultTransport from net/http! We | ||
| // have our own copy to avoid handling a situation where the | ||
| // DefaultTransport is overwritten with some different implementation | ||
| // of http.RoundTripper or it's modified by other package. |
There was a problem hiding this comment.
Is it necessarily problematic if the DefaultTransport has been modified elsewhere? I'm not sure I'm keen on the idea of maintaining our own default configuration that is supposed to stay in sync with the stdlib.
There was a problem hiding this comment.
This code has been moved from the otlphttp/driver.go:
https://github.com/open-telemetry/opentelemetry-go/pull/1859/files#diff-25cd91630c3b0471c12667c1138ba7715cd6269e539dde179b3b420fe455e2adL52
And I suggest to keep it in to ensure backwards compatability.
There was a problem hiding this comment.
Yeah, that got added in #1420 with 2cdd01c based on a comment I made about a check that the http.DefaultTransport was still a *http.Transport that would panic() if it wasn't. I didn't then, and I'm not sure I do now, understand why it would be problematic if the http.DefaultTransport had been changed. Wouldn't that be indicative that the user really wanted all HTTP clients to use their new default?
I can see it being potentially problematic if the user has replaced http.DefaultTransport with otelhttp.Transport which will create spans for every outgoing request. Tracing the requests to ship your tracing data will result in never-ending trace data. But, that's a misconfiguration the user could create by using the mechanisms provided in this PR as well. Other SIGs have proposed a suppression mechanism that could be used to indicate that no telemetry should be generated from that context, which seems to me a potentially better solution to that problem.
There was a problem hiding this comment.
That makes sense. So how can I move forward with this? Should I remove the DefaultTransport and fall back to the one from stdlib? And should I implement the suppression mechanism, to prevent the never ending trace scenario? Or would that be out of scope for this PR?
There was a problem hiding this comment.
Adding a suppression mechanism is definitely out of scope for this PR. It was discussed briefly at the recent spec SIG meeting but there's not yet consensus on how to handle it. I think it is something we should be able to add post-1.0 if it becomes required.
For this PR I think we can remove the DefaultTransport and simply use the one from stdlib. @MrAlias thoughts?
MrAlias
added
the
blocked:CLA
|
I have to close and reopen the pull request according to the EasyCLA documentation, to force a recheck by the EasyCLA system. Do you prefer me to do that now, or after all review has been done and all conversations are resolved? @Aneurysm9 |
Codecov Report
@@ Coverage Diff @@
## main #1859 +/- ##
=====================================
Coverage 78.6% 78.6%
=====================================
Files 137 137
Lines 7298 7315 +17
=====================================
+ Hits 5737 5755 +18
- Misses 1316 1317 +1
+ Partials 245 243 -2
|
Hmm, my closing and re-opening didn't seem to do it. Does it require that you re-submit a new PR? That's awful :( In any event, best to get it out of the way as soon as possible. |
|
What problem is this PR trying to solve? It's not clear to me what adding this will provide and the linked issue only states it would be nice to have. Can you update the description or the linked issue with situations where having this will solve a problem? |
|
I have updated the referenced issue description to include my use case. |
I have opened a new PR with the proposed changes and rebased onto the main branch: |
Resolves #1858