SpanProcessor incorrect calls to SpanExporter::forceFlush()#787
SpanProcessor incorrect calls to SpanExporter::forceFlush()#787amber0612 wants to merge 29 commits into
Conversation
Codecov Report
@@ Coverage Diff @@
## main #787 +/- ##
============================================
- Coverage 81.03% 80.38% -0.65%
+ Complexity 1825 1768 -57
============================================
Files 225 222 -3
Lines 4687 4523 -164
============================================
- Hits 3798 3636 -162
+ Misses 889 887 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Nevay
left a comment
There was a problem hiding this comment.
(Please rebase your branch to remove the unrelated commits.)
| if ($this->bufferReachedExportLimit() || $this->enoughTimeHasPassed()) { | ||
| $this->forceFlush(); | ||
| $this->exporter->export($this->queue); | ||
| $this->queue = []; |
There was a problem hiding this comment.
Queue has to be cleared before calling ::export(), otherwise we might export the batch multiple times if the exporter suspends execution. We will need a $queueSize property to handle $maxQueueSize after this change.
There was a problem hiding this comment.
I have created a try block inside which export would take place. If somehow exporter suspends execution and fails , finally block would clear the queue. Will this help resolving the issue??
There was a problem hiding this comment.
No, I meant suspend as in Fiber::suspend(). Something like the following would resolve it:
$batch = $this->queue;
$this->queue = [];
$this->stopwatch->reset();
try {
$this->exporter->export($batch);
} finally {
$this->queueSize -= count($batch);
}The spec mentions that "Export() will never be called concurrently for the same exporter instance", but I don't see a way for us to follow the spec unless we queue execution in a single fiber. This would mean even more blocking for the Span::end() call that triggered the initial export (worst case blocking forever) ("Expect this operation to be called in the "hot path" of production applications. It needs to be designed to complete fast, if not immediately.").
#715