sdk: Implement force_flush for span processors#389
Merged
toumorokoshi merged 3 commits intoopen-telemetry:masterfrom Feb 4, 2020
Merged
sdk: Implement force_flush for span processors#389toumorokoshi merged 3 commits intoopen-telemetry:masterfrom
toumorokoshi merged 3 commits intoopen-telemetry:masterfrom
Conversation
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.
Codecov Report
@@ Coverage Diff @@
## master #389 +/- ##
==========================================
- Coverage 85.35% 85.14% -0.21%
==========================================
Files 38 38
Lines 1925 1912 -13
Branches 226 226
==========================================
- Hits 1643 1628 -15
- Misses 218 219 +1
- Partials 64 65 +1
Continue to review full report at Codecov.
|
ocelotl
reviewed
Jan 29, 2020
c24t
approved these changes
Jan 29, 2020
Member
c24t
left a comment
c24t
left a comment
There was a problem hiding this comment.
LGTM after updating the tests for https://github.com/open-telemetry/opentelemetry-python/pull/389/files#r372660672.
It sounds like it'd be sufficient to check that calling shutdown results in a flush even without calling force_flush first.
codeboten
approved these changes
Jan 30, 2020
Contributor
codeboten
left a comment
codeboten
left a comment
There was a problem hiding this comment.
Thanks for updating the code to match the spec!
Test that: - processor works after call to force_flush() - shutdown() flushes the processor
Oberon00
approved these changes
Feb 4, 2020
mauriciovasquezbernal
force-pushed
the
mauricio/implement-force-flush
branch
from
dba213d to
7612e6e
Compare
mauriciovasquezbernal
added a commit
to kinvolk/opentelemetry-python
that referenced
this pull request
Feb 6, 2020
open-telemetry#389 implemented force_flush() for the span processor. For BatchSpanProcessor it was implemented by exposing an already existing _flush() method, it created a race condition because the _flush() method was intended to be called only from the context of the worker thread, this because it uses the export() method that is not thread safe. The result after that PR is that some tests were failing randomly because export() was being executed in two different threads, the worker thread and the user thread calling force_flush(). This commit fixes it by implementing a more sophisticated flush mechanism. When a flush is requested, a special span token is inserted in the spans queue, a flag indicating a flush operation is on progress is set and the worker thread is waken up, after it a condition variable is monitored waiting for the worker thread to indicate that the token has been processed. The worker thread has a new logic to avoid sleeping (waiting on the condition variable) when there is a flush operation going on, it also notifies the caller (using another condition variable) when the token has been processed.
c24t
pushed a commit
that referenced
this pull request
Feb 7, 2020
#389 implemented force_flush() for the span processor. For BatchSpanProcessor it was implemented by exposing an already existing _flush() method, it created a race condition because the _flush() method was intended to be called only from the context of the worker thread, this because it uses the export() method that is not thread safe. The result after that PR is that some tests were failing randomly because export() was being executed in two different threads, the worker thread and the user thread calling force_flush(). This commit fixes it by implementing a more sophisticated flush mechanism. When a flush is requested, a special span token is inserted in the spans queue, a flag indicating a flush operation is on progress is set and the worker thread is waken up, after it a condition variable is monitored waiting for the worker thread to indicate that the token has been processed. The worker thread has a new logic to avoid sleeping (waiting on the condition variable) when there is a flush operation going on, it also notifies the caller (using another condition variable) when the token has been processed.
toumorokoshi
pushed a commit
to toumorokoshi/opentelemetry-python
that referenced
this pull request
Feb 17, 2020
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.
toumorokoshi
pushed a commit
to toumorokoshi/opentelemetry-python
that referenced
this pull request
Feb 17, 2020
open-telemetry#389 implemented force_flush() for the span processor. For BatchSpanProcessor it was implemented by exposing an already existing _flush() method, it created a race condition because the _flush() method was intended to be called only from the context of the worker thread, this because it uses the export() method that is not thread safe. The result after that PR is that some tests were failing randomly because export() was being executed in two different threads, the worker thread and the user thread calling force_flush(). This commit fixes it by implementing a more sophisticated flush mechanism. When a flush is requested, a special span token is inserted in the spans queue, a flag indicating a flush operation is on progress is set and the worker thread is waken up, after it a condition variable is monitored waiting for the worker thread to indicate that the token has been processed. The worker thread has a new logic to avoid sleeping (waiting on the condition variable) when there is a flush operation going on, it also notifies the caller (using another condition variable) when the token has been processed.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
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.
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.