Repository navigation
[EXPORTER] Fix incorrect doc comment about SessionState::TimedOut in the Elasticsearch exporter - #4647
Open
om7057 wants to merge 6 commits into
Open
[EXPORTER] Fix incorrect doc comment about SessionState::TimedOut in the Elasticsearch exporter#4647om7057 wants to merge 6 commits into
om7057 wants to merge 6 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4647 +/- ##
==========================================
+ Coverage 86.69% 86.70% +0.01%
==========================================
Files 527 527
Lines 20902 20902
==========================================
+ Hits 18119 18120 +1
+ Misses 2783 2782 -1
🚀 New features to boost your workflow:
|
Contributor
|
Thanks for the investigation — the diagnosis is right, but I don't think we can remove the enumerator. SessionState is in an installed public header and is the event contract for any HttpClient implementation, including the out-of-tree ones we explicitly support. Removing it breaks their build, so it would need a DEPRECATED entry and a deprecation cycle per docs/deprecation-process.md. Also, #4604 is still |
No HttpClient implementation in this repo ever dispatched TimedOut. The curl client maps CURLE_OPERATION_TIMEDOUT to SendFailed like any other transport error, so the four branches handling TimedOut in the Elasticsearch and OTLP HTTP exporters were unreachable, and the doc comment claiming a timeout arrives as a TimedOut event was incorrect. Removed the dead branches and fixed the comment instead of making TimedOut reachable, since that would change behavior for any code already handling a timeout via SendFailed today. Fixes open-telemetry#4604
Removing SessionState::TimedOut requires going through the project's formal deprecation process (docs/deprecation-process.md), since it is part of an installed public header and the event contract for any out-of-tree HttpClient implementation. Issue open-telemetry#4604 is also still needs-triage, not yet accepted for a specific direction. Reverted the enum removal and the four now-restored dead branches in the Elasticsearch and OTLP HTTP exporters. Kept only the doc comment fix on ResponseHandler::waitForResponse(), which was simply incorrect about a timeout arriving as a TimedOut event when it actually arrives as SendFailed, and needs no process to correct. Addresses review feedback from nikhilbhatia08.
om7057
force-pushed
the
fix/remove-dead-timedout-session-state
branch
2 times, most recently
from
September 29, 2026 15:11
11b61f0 to
b3151b9
Compare
om7057
changed the title
Contributor
Author
|
@nikhilbhatia08 Understood and thanks. I have scoped this down to just the doc comment fix, which was simply wrong about the event and needs no process to correct. |
…out-session-state # Conflicts: # CHANGELOG.md
…out-session-state # Conflicts: # CHANGELOG.md
Contributor
Author
|
@nikhilbhatia08 could you please check this PR once? |
om7057
added a commit
to om7057/opentelemetry-cpp
that referenced
this pull request
Oct 18, 2026
waitForResponse()'s deadline was always derived as now() + seconds(response_timeout_), so response_timeout_ = 0 produced a deadline equal to now(), which the next instant is already behind. Every synchronous export then timed out immediately, even against a client that was about to answer successfully, since cv_::wait_until checks its predicate before checking the deadline only when the predicate is already satisfied at entry. response_timeout = 0 is documented as meaning no timeout, matching curl's own CURLOPT_TIMEOUT_MS 0 semantics, so a non-positive timeout now maps to steady_clock::time_point::max() instead. Added ZeroResponseTimeoutMeansNoTimeout with a DelayedSession that answers on a background thread after a genuine short delay, so the deadline comparison is actually exercised; confirmed it fails on the prior behavior and passes on this fix. Also corrected the waitForResponse() doc comment, which claimed a timeout surfaces as a TimedOut session event. It surfaces as SendFailed, matching the wording open-telemetry#4647 already uses for the same comment. Addresses review feedback from mateenali66.
…out-session-state # Conflicts: # CHANGELOG.md
Contributor
Author
|
cc: @marcalff |
…out-session-state # Conflicts: # CHANGELOG.md
This branch has not been deployed
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fixes #4604 (doc comment only; the enum question needs SIG discussion, see below)
Changes
ResponseHandler::waitForResponse()'s doc comment in the Elasticsearch log exporter claims a request timeout (set viaSetTimeoutMs()) "arrives here as a TimedOut session event." That's incorrect: noHttpClientimplementation in this repo dispatchesSessionState::TimedOut. The curl client mapsCURLE_OPERATION_TIMEDOUTtoSendFailed, indistinguishable from any other transport failure, which the comment now says.This PR originally also removed the unreachable
TimedOutenumerator and its four dead branches (Elasticsearch and OTLP HTTP exporters). A reviewer correctly pointed out that's not something to do directly:SessionStateis part of an installed public header and the event contract for any out-of-treeHttpClientimplementation, so removing an enumerator needs to go throughdocs/deprecation-process.md(SIG discussion, a[DEPRECATION]issue, aDEPRECATEDentry, a full deprecation cycle before removal). Issue #4604 is also stillneeds-triage, so it hasn't been accepted for a specific direction yet.Scoped this PR down to just the doc comment fix, which needs no process to correct. Left the actual enum question (dispatch
TimedOutfor real vs. deprecate-and-remove it) for the SIG to decide on the issue.Ran the full test suite locally, all passing.
CHANGELOG.mdupdated for non-trivial changes