Check context prior to delaying retry in OTLP exporters#7678
Check context prior to delaying retry in OTLP exporters#7678MrAlias merged 3 commits intoopen-telemetry:mainfrom
Conversation
Fix open-telemetry#7673 Do not rely on non-deterministic `select` statement to catch ended context prior to waiting for a retry delay. Explicitly check the context prior to entering the wait.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #7678 +/- ##
=======================================
- Coverage 86.1% 86.0% -0.2%
=======================================
Files 298 298
Lines 21709 21727 +18
=======================================
- Hits 18707 18696 -11
- Misses 2625 2630 +5
- Partials 377 401 +24
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR adds explicit context cancellation checks to OTLP exporters' retry logic to eliminate reliance on non-deterministic select statement behavior. The change ensures requests with canceled contexts fail immediately without waiting for retry delays, fixing a flaky test issue.
- Adds
ctx.Err()check immediately after determining an error is retryable but before attempting any retry delay - Uses consistent error wrapping pattern
fmt.Errorf("%w: %w", ctx.Err(), err)to preserve both the context error and original error - Applied uniformly across all OTLP exporter variants (trace/metric/log over HTTP/gRPC)
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| internal/shared/otlp/retry/retry.go.tmpl | Template source adding context check before retry wait logic |
| exporters/otlp/otlptrace/otlptracehttp/internal/retry/retry.go | Generated file for HTTP trace exporter with context check |
| exporters/otlp/otlptrace/otlptracegrpc/internal/retry/retry.go | Generated file for gRPC trace exporter with context check |
| exporters/otlp/otlpmetric/otlpmetrichttp/internal/retry/retry.go | Generated file for HTTP metric exporter with context check |
| exporters/otlp/otlpmetric/otlpmetricgrpc/internal/retry/retry.go | Generated file for gRPC metric exporter with context check |
| exporters/otlp/otlplog/otlploghttp/internal/retry/retry.go | Generated file for HTTP log exporter with context check |
| exporters/otlp/otlplog/otlploggrpc/internal/retry/retry.go | Generated file for gRPC log exporter with context check |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| // Check if context is canceled before attempting to wait and retry. | ||
| if ctx.Err() != nil { | ||
| return fmt.Errorf("%w: %w", ctx.Err(), err) |
There was a problem hiding this comment.
I'd like to suggest using context.Cause() here, which could make the error more useful.
It is already used in this file, from #6898.
I made a PR: #7811
Fix #7673
Issue being addressed:
Do not rely on non-deterministic
selectstatement to catch ended context prior to waiting for a retry delay. Explicitly check the context prior to entering the wait.This resolves the flaky test and ensure in normal operation that requests with canceled context are ended without having to wait for any additional delays.