-
Notifications
You must be signed in to change notification settings - Fork 192
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
Prevent concurrent ::export()
calls in span processor
#788
Prevent concurrent ::export()
calls in span processor
#788
Conversation
Codecov Report
@@ Coverage Diff @@
## main #788 +/- ##
============================================
+ Coverage 80.40% 80.66% +0.25%
- Complexity 1765 1786 +21
============================================
Files 222 222
Lines 4516 4592 +76
============================================
+ Hits 3631 3704 +73
- Misses 885 888 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
77e02fc
to
af668ad
Compare
int $scheduledDelayMillis = self::DEFAULT_SCHEDULE_DELAY, | ||
int $exportTimeoutMillis = self::DEFAULT_EXPORT_TIMEOUT, | ||
int $maxExportBatchSize = self::DEFAULT_MAX_EXPORT_BATCH_SIZE, | ||
bool $autoFlush = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be changed to default false
before beta to be consistent with async implementations that will only export once the event loop gains control. Using default true
for now to keep the scope of the PR small / to not break examples etc.
de53b81
to
4a2b869
Compare
88e0a69
to
d309f83
Compare
This PR calls
::export()
in a synchronous loop to prevent concurrent calls to::export()
.Note that this might block the
Span::end()
call that triggered the export longer. The blocking autoflush behavior ofBatchSpanProcessor::onEnd()
can be disabled, spans will then only be flushed on explicit::forceFlush()
/::shutdown()
calls.Disallowing concurrent export calls means that we have to reintroduce support for concurrent exports by changing the return type of
::export()
to a promise/future-like value - I plan on working on that after this PR is merged.Alternative to #787, see also #787 (comment).