Add flush interface to span processor #370
Conversation
dyladan
requested review from
AloisReitbauer,
bogdandrutu,
c24t,
carlosalberto,
iredelmeier,
jmacd,
reyang,
SergeyKanzhelev,
songy23,
tedsuo,
tigrannajaryan and
yurishkuro
as code owners
There was a problem hiding this comment.
Hi @dyladan. This does seem simple on the surface, but please review my requests here: #351 (comment)
|
@tedsuo This is a SDK-facing API, and I believe the problems you allude to stem from letting this API face the user in OpenTracing APIs. There is always a question of what-to-do when Flush calls fail. The additional complication here is that it's not a very good idea to use OpenTelemetry APIs to report the failure, but since this is a SDK-facing API the SDK or the main function that installed it should have a good idea of what to do (it is, after all, a special circumstance). |
|
This is not a property of the SpanProcessor interface. You probably need this just for the processors that talk to the exporter pipeline. |
@bogdandrutu right now export is the only use-case I can think of, but the specification is specific that batching should be done at the span processor and not at the exporter. For that reason, that's where the flush needs to be. For span processors that don't need to flush, it will be a simple no-op. |
| @@ -328,7 +336,7 @@ return FailedNotRetryable error. | |||
|
|
|||
| `Shutdown` should not block indefinitely (e.g. if it attempts to flush the data | |||
There was a problem hiding this comment.
maybe make flush a link to the newly added flush method?
|
Discussed in triage meeting: Can we close this? |
|
My understanding was that we just need a better "dangerously sounding" name and then we can merge this? |
|
My understanding of the most recent conversations was that we decided to call it |
|
I am OK with calling it |
|
It sounds like actually "ForceFlush" was the chosen name, no? Other than that, I'm approving. |
|
It looks like we cannot merge this PR as @tedsuo didn't resolve his request. Wondering if there's something we can do to get that out of the way ;) |
jmacd
dismissed
tedsuo’s stale review
Dismissed. The presenter has given good justification, and this is not a function on the a user-facing API.
|
Ok, we got @tedsuo changes request dismissed, merging ;) |
open-telemetry/opentelemetry-specification#370 added the requirement to have a "force_flush" method in the span processors. This commit exposes an already existing internal method on the batch span processor that does exactly the same, it also adds it to the span processor interface and as a no-op to the simple span processor.
open-telemetry/opentelemetry-specification#370 added the requirement to have a "force_flush" method in the span processors. This commit exposes an already existing internal method on the batch span processor that does exactly the same, it also adds it to the span processor interface and as a no-op to the simple span processor.
open-telemetry/opentelemetry-specification#370 added the requirement to have a "force_flush" method in the span processors. This commit exposes an already existing internal method on the batch span processor that does exactly the same, it also adds it to the span processor interface and as a no-op to the simple span processor.
Fixes #351
Some cases like AWS lambda can suspend a process before span processors export the completed spans. In these cases, it is desirable to flush the span processor to the exporter before the process suspends so that no spans are lost.
Alternatives considered
call shutdown
use simple span processor
The simple span processor does not batch, so will export spans as soon as they are completed. While this solves the flushing problem, it can be inefficient in many cases. It is a common use case for a lambda function to make many async calls and return as soon as they are all complete. With a simple span processor, the function may call the export function many times just as the function completes. Since it is specified that
Exportmay not be called concurrently, these exports will happen serially. In these cases it is preferable to batch spans and export them in a single call.