Wait for exports on the simple span processor's ForceFlush #1030
Conversation
This file contains 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
|
|
ForceFlush seems to have been left behind in open-telemetry#502. With those changes, the processing is not really synchronous anymore, i.e. OnEnd now only sends the span down the pipe to be processed in the separate thread as soon as possible. https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/sdk.md#forceflush-1 says: > In particular, if any SpanProcessor has any associated exporter, it SHOULD try to call the exporter's Export with all spans for which this was not already done and then invoke ForceFlush on it. As the comment states, all spans previously got exported synchronounsly right away, so that no such spans existed, but now they might be anywhere between the channel and (the end of) the export call. Doin g nothing in ForceFlush even violates the specification as... > The built-in SpanProcessors MUST do so. Awaiting all open tasks from the channel on ForceFlush fixes this. Previous discussions regarding parts of the specification that this does not tackle in line with Shutdown: > ForceFlush SHOULD provide a way to let the caller know whether it succeeded, failed or timed out. open-telemetry#358 (comment) > ForceFlush SHOULD complete or abort within some timeout. https://github.com/open-telemetry/opentelemetry-rust/pull/502/files#r603722431 This brings the simple processor a step closer to the batch processor with the obvious main difference of batches and the (not so obvious, also see open-telemetry#502 (comment)) difference that it works without a presumed async runtime.
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #1030 +/- ##
=======================================
- Coverage 55.1% 55.1% -0.1%
=======================================
Files 149 149
Lines 18143 18159 +16
=======================================
+ Hits 10006 10010 +4
- Misses 8137 8149 +12
... and 1 file with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
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.
ForceFlush seems to have been left behind in #502. With those changes, the processing is not really synchronous anymore, i.e. OnEnd now only sends the span down the pipe to be processed in the separate thread as soon as possible.
https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/sdk.md#forceflush-1 says:
As the comment states, all spans previously got exported synchronounsly right away, so that no such spans existed, but now they might be anywhere between the channel and (the end of) the export call. Doing nothing in ForceFlush even violates the specification as...
Awaiting all open tasks from the channel on ForceFlush fixes this.
Previous discussions regarding parts of the specification that this does not tackle in line with Shutdown:
#358 (comment)
https://github.com/open-telemetry/opentelemetry-rust/pull/502/files#r603722431
This brings the simple processor a step closer to the batch processor with the obvious main difference of batches and the (not so obvious, also see #502 (comment)) difference that it works without a presumed async runtime.