Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[span processor] Add force_flush method. #358

Merged
merged 4 commits into from
Nov 11, 2020

Conversation

TommyCpp
Copy link
Contributor

@TommyCpp TommyCpp requested a review from a team November 11, 2020 02:12
@codecov
Copy link

codecov bot commented Nov 11, 2020

Codecov Report

Merging #358 (aacec5d) into master (9c4f310) will increase coverage by 0.36%.
The diff coverage is 79.16%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #358      +/-   ##
==========================================
+ Coverage   54.19%   54.56%   +0.36%     
==========================================
  Files          70       70              
  Lines        5836     5914      +78     
==========================================
+ Hits         3163     3227      +64     
- Misses       2673     2687      +14     
Impacted Files Coverage Δ
opentelemetry/src/testing/trace.rs 71.21% <68.42%> (-1.13%) ⬇️
opentelemetry/src/sdk/trace/span_processor.rs 81.04% <86.20%> (+3.38%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9c4f310...aacec5d. Read the comment docs.

Copy link
Member

@frigus02 frigus02 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks nice. I'm unsure about this part of the spec:

ForceFlush SHOULD provide a way to let the caller know whether it succeeded, failed or timed out.

This sounds like, similar to shutdown, the function should wait until the spans have been exported and only return then.

I don't immediately know how to implement this, though. And it's only a "should". So I think we're spec compliant with the way you did it.

Copy link

@flycash flycash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my understanding,

ForceFlush SHOULD provide a way to let the caller know whether it succeeded, failed or timed out.
...
ForceFlush SHOULD complete or abort within some timeout. 

I think force_flush need to add the timeout parameter, and use enum or status code as return value.

Implementations can use something like future to flush spans.

@TommyCpp
Copy link
Contributor Author

ForceFlush SHOULD provide a way to let the caller know whether it succeeded, failed or timed out.

@frigus02 Yeah I also noticed that, but I find the shutdown method also doesn't return any result. So feel like we should have a dedicated PR to discuss how to implement this.

I think force_flush need to add the timeout parameter, and use enum or status code as return value.

@flycash I agree. But I think we might as well add timeout for both shutdown and force_flush in SimpleSpanProcessor and BatchSpanProcessor. So I was going to create another PR for that.

Copy link
Member

@frigus02 frigus02 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me 👍

opentelemetry/src/sdk/trace/span_processor.rs Outdated Show resolved Hide resolved
@jtescher jtescher merged commit 68f041a into open-telemetry:master Nov 11, 2020
cschramm added a commit to cschramm/opentelemetry-rust that referenced this pull request Apr 17, 2023
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.
cschramm added a commit to cschramm/opentelemetry-rust that referenced this pull request Apr 24, 2023
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.
jtescher pushed a commit that referenced this pull request Apr 24, 2023
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:

> 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.

#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 #502 (comment)) difference that it works without a presumed async runtime.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement SpanProcessor::force_flush
4 participants