Add SpanExporter.ForceFlush. #1467
Conversation
Oberon00
added
area:sdk
Co-authored-by: Armin Ruech <armin.ruech@dynatrace.com>
|
Overall LGTM, although this is precisely one of the interfaces that are exposed to the users and needs special care and some thought on whether some kind of 'default method' can be offered in the different SIGs. cc @bogdandrutu |
| @@ -582,6 +588,24 @@ return a `Failure` result. | |||
| and the destination is unavailable). OpenTelemetry client authors can decide if they | |||
| want to make the shutdown timeout configurable. | |||
|
|
|||
| #### `ForceFlush()` | |||
There was a problem hiding this comment.
I think we can say, in case of stable releases a "no-op" implementation can be offered as a default. I think that is the best we can do :)
I agree with @reyang here, we should should return when the deadline happens and not try to over-engineer the logic, if too short you get done what was possible during the amount of time. |
| `ForceFlush` SHOULD complete or abort within some timeout. `ForceFlush` can be | ||
| implemented as a blocking API or an asynchronous API which notifies the caller | ||
| via a callback or an event. OpenTelemetry client authors can decide if they want to | ||
| make the flush timeout configurable. |
There was a problem hiding this comment.
For blocking API, I think it is very hard to abort within some timeout. Like the implementation complies to the spec if it just does abort its entry point without doing any work. Should we say ForceFlush should return (complete or abort) as soon as possible if the given timeout is reached?
There was a problem hiding this comment.
This is copy & past from shutdown and the other ForceFlush. Please open an issue if you want tor reword this (should be consistent everywhere).
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
|
@Oberon00 please rebase :) that will also remove the stale |
|
Please rebase |
|
@arminru can you rebase & review & and merge if you are ok with this change :) |
| 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. | ||
| The [built-in SpanProcessors](#built-in-span-processors) MUST do so. |
There was a problem hiding this comment.
@Oberon00 Please add an entry in the compliance matrix for that, otherwise it might be overlooked easily.
Add "SpanProcessor implements ForceFlushes spec" row
Co-authored-by: Armin Ruech <armin.ruech@dynatrace.com>
Fixes #1454.
Changes
Adds
ForceFlush(which already existed in SdkTracerProvider and SpanProcessor) to SpanExporter, and adds wording that SpanProcessor.ForceFlush must call that new SpanExporter.ForceFlush.