Improve BatchExportSpanProcessor #1062
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* it was possible for force flush calls to miss the flush finished notifications by the worker thread. in case a flush token got added in the main thread and the worker thread processed and notified the flush condition before the main thread called wait on the flush condition, the wakup is missed and the main thread has to wait the full flush timeout * calls to force flush were not really thread safe since the state if a flush operation is in progress was indictated by a single boolean flag which gets reset when the first force flush call finishes. * instead of having a boolean flag to indicate a flush request use an Event. When a call to force flush is made it is looked up if a flush request event is currently pending or a new one is created. The worker thread will check if a flush request event exists, unset it and use a local reference for signaling once the export operation finished. Force flush calls will wait in the meantime on the flush request event until they are signaled by the worker thread. This also makes calls to force flush thread safe since multiple threads can/might wait on one event.
lzchen
reviewed
Sep 1, 2020
|
I believe this PR also solves the issue of the previous implementation of |
lzchen
reviewed
Sep 1, 2020
lzchen
reviewed
Sep 1, 2020
lzchen
approved these changes
Sep 2, 2020
|
Does this PR implement enough tests to cover what PR #947 was trying to cover? |
codeboten
approved these changes
Sep 3, 2020
| if num_spans <= 0: | ||
| break | ||
|
|
||
| def export_batch(self) -> int: |
There was a problem hiding this comment.
Would it make sense to make this method internal to the processor?
Suggested change
| def export_batch(self) -> int: | |
| def _export_batch(self) -> int: |
There was a problem hiding this comment.
sounds good since it should only be called by the worker thread. same goes for the export method.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Improve flushing in the
BatchExportSpanProcessorby using athreading.Eventfor requesting a flush operation and signaling that a flush operation finished.The idea is to have the thread which calls
force_flushto create a flush request (threading.Event+ num spans to flush) and wait on this request until it gets signaled by the worker thread or the flush timeout expires.The worker thread checks in each export interval if a flush request exists, unsets it and uses the local reference to signal the end of a flush/export operation.
By using a
threading.Eventfor signaling, wakeup notifications are not missed since the wait call will return immediately once being signaled.This also addresses the issue that calls to
force_flushwere not thread safe since the indiciation if a flush operation was in progress relied on a boolean flag on the span processors instance which gets reset once the firstforce_flushcall finishes.Addresses #949
Type of change
Checklist: