Log cancelled and failed requests #919
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #919 +/- ##
========================================
+ Coverage 7.08% 7.09% +0.01%
========================================
Files 276 276
Lines 64376 64381 +5
========================================
+ Hits 4563 4570 +7
Misses 59502 59502
+ Partials 311 309 -2 ☔ View full report in Codecov by Sentry. |
| assert.Equal(t, `[DEBUG] non-retriable error: Get "/a": request timed out after 30s of inactivity | ||
| [DEBUG] GET /a | ||
| < Error: Get "/a": request timed out after 30s of inactivity | ||
| `, bufLogger.String()) |
There was a problem hiding this comment.
We could eliminate the first line to avoid duplication, perhaps?
There was a problem hiding this comment.
I think we should keep it since there are two distinct things being logged: the raw request/response pair, and then the decision to not retry the error. I've reordered them so that there is a logical flow in the log output.
| // client failed to read the response body. | ||
| // Otherwise the response body will include details about the error. | ||
| if len(r.ResponseBody) == 0 && r.Err != nil { | ||
| sb.WriteString(fmt.Sprintf(" (Error: %s)", r.Err)) |
There was a problem hiding this comment.
a question, when length of response body is > 0 (line 86), is it always true that the Err would be nil?
There was a problem hiding this comment.
Not necessarily. We call the ErrorMapper function that parses the response into one of the SDK errors depending on the result. If the API responds with any of our standard error codes, r.Err will be set to that.
This is just refactored from above, so there is no behavior change here.
* Fixed codecov for repository ([#909](#909)). * Add traceparent header to enable distributed tracing. ([#914](#914)). * Log cancelled and failed requests ([#919](#919)). Dependency updates: * Bump golang.org/x/net from 0.22.0 to 0.24.0 ([#884](#884)). * Bump golang.org/x/net from 0.17.0 to 0.23.0 in /examples/zerolog ([#896](#896)). * Bump golang.org/x/net from 0.21.0 to 0.23.0 in /examples/slog ([#897](#897)).
* Fixed codecov for repository ([#909](#909)). * Add traceparent header to enable distributed tracing. ([#914](#914)). * Log cancelled and failed requests ([#919](#919)). Dependency updates: * Bump golang.org/x/net from 0.22.0 to 0.24.0 ([#884](#884)). * Bump golang.org/x/net from 0.17.0 to 0.23.0 in /examples/zerolog ([#896](#896)). * Bump golang.org/x/net from 0.21.0 to 0.23.0 in /examples/slog ([#897](#897)).
Changes
Currently, we do not log requests that failed to complete or that failed when the SDK started processing the response. This means that requests that timeout due to a client-side timeout or canceled context are not logged, meaning that there is no visibility in the log of these kinds of requests.
This PR changes the request processing and error handling to ensure that all requests are logged, even those that are cancelled or fail due to other errors. The general approach is to process as much of the response as possible until encountering an error, log what we've retrieved, and then do any post-processing.
Some changes are needed in the roundtrip stringer to handle the case of a nil response.
Tests
Unit tests verify that canceled requests or nil responses are logged.
make testpassingmake fmtapplied