otelhttptrace: fix false positive errors in otelhttptrace for http.receive span #8299
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dmathieu
added
the
blocked: CLA
mitioshi
force-pushed
the
silence-http-pool-errors
branch
from
492a3b1 to
8320979
Compare
Author
|
@dmathieu Since this PR has a pending CLA label, here's a FYI that I signed it. This PR can now be reviewed |
dmathieu
removed
the
blocked: CLA
dmathieu
reviewed
Dec 18, 2025
| // this error is passed to the PutIdleConn callback when http.Transport's HTTP 1.1 connection pool is full | ||
| // It's not an error per se, but rather a control flow error used for connection pooling logic in net/http internals | ||
| // corresponds to errTooManyIdleHost in net/http/internal/transport.go | ||
| const poolReturnErr = "too many idle connections" |
Member
There was a problem hiding this comment.
Could you move those constants at the package level?
| // non-nil err in this callback is one of the following net/http errors: errTooManyIdleHost, errBrokenConn, errCloseIdle | ||
| // All of those are internal. No reason to mark the span as error if we encounter them | ||
| // net/http doesn't export said errors, so we can only match against their strings | ||
| if strings.Contains(err.Error(), poolReturnErr) || |
Member
There was a problem hiding this comment.
Doing ct.end twice is a bit misleading.
How about checking for err == nil in this condition as well?
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8299 +/- ##
=====================================
Coverage 82.3% 82.3%
=====================================
Files 193 193
Lines 13769 13779 +10
=====================================
+ Hits 11334 11344 +10
Misses 2030 2030
Partials 405 405
🚀 New features to boost your workflow:
|
Member
|
This will need a changelog entry. |
Author
Added one in the recent commit |
mitioshi
force-pushed
the
silence-http-pool-errors
branch
2 times, most recently
from
9931da9 to
b18867d
Compare
mitioshi
force-pushed
the
silence-http-pool-errors
branch
from
b18867d to
6b277bc
Compare
Author
|
@dmathieu FYI: I've implemented the changes proposed by code review |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Related issue: #8216
Problem
We use httptrace for our http client. Very frequently we observe traces being marked as "error" due to "http: putIdleConn: too many idle connections for host" in the http.receive span.
This error is a part of an internal mechanism in net/http to pool http connections.
Transport keeps a per-host pool of idle keep-alive sockets.
In tryPutIdleConn, after a request finishes, the connection is returned to that pool. If the pool already holds MaxIdleConnsPerHost entries (default 2 if you don’t override it), the function returns errTooManyIdleHost, and the connection is immediately closed instead of being cached.
This is not a client issue and not even an error(just a part of the pooling flow) and shouldn't mark traces as erroring
Implementation
In this PR, we explicitly ignore the errors that can be passed by http.Transport to the PutIdleConn callback(which is used by ClientTrace). Since the errors are unexported, we have to match the error strings.
I've also added tests for two of three possible erorrs. The third one, errCloseIdle cannot be reliably reproduced in tests without introduing a flaky test, so I decided to omit the test for it.