-
Notifications
You must be signed in to change notification settings - Fork 888
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
Clarify non-blocking requirement from span API End #1555
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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.
This language seems more useful for the BatchSpanProcessor than the API - there is really nothing Span.end()
can do about the possibility of blocking in span processors / exporters. So it needs to be just as non-blocking as other methods like setAttribute. A text at the top of the doc applying to all methods could make sense to me but not on only the one Span.end
.
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.
Blocking this against accidental merge. There's no consensus from what I can see, and this is a critical piece of the API which if we relax it we cannot make it more restrictive again.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Feedback has moved PR to a different solution
@anuraaga @Oberon00 @yurishkuro @iNikem I've updated based on the feedback. PTAL |
Add missing word.
Co-authored-by: Christian Neumüller <christian+github@neumueller.me>
Fixes #1522
Changes
The specification states states a "Span becomes non-recording by being ended". The specification reiterates this in the
IsRecording
section by saying "[a]fter a Span is ended, it usually becomes non-recording and thus IsRecording SHOULD consequently return false for ended Spans." This means that theEnd
andIsRecorded
must share some state about the ended nature of a Span.How this shared state is manage in implementations as it can lead to a race condition if it is accessed simultaneously. To specify how this should be handled the specification state that "[a]ll methods of Span are safe to be called concurrently", but it also states the
End
method of a Span "MUST be non-blocking."This introduces a logical inconsistency.
The term lock-free is also interpreted as:
And as the comment goes on to state, "that's exactly what the built-in SimpleSpanProcessor does."
This change updates the specification to clarify what "non-blocking" means: