Skip to content
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

Merged
merged 10 commits into from
Mar 29, 2021

Conversation

MrAlias
Copy link
Contributor

@MrAlias MrAlias commented Mar 17, 2021

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 the End and IsRecorded 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:

you should not synchronously do I/O e.g. to export the span.

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:

  • Do not preform blocking I/O on calling thread
  • Try and provide a lock-free algorithm and require lock usage be minimized
  • Scope to not include "non-production" SpanExporters and SpanProcessors

@yurishkuro

This comment has been minimized.

@jmacd

This comment has been minimized.

@MrAlias

This comment has been minimized.

Oberon00
Oberon00 previously approved these changes Mar 17, 2021
@iNikem

This comment has been minimized.

@iNikem

This comment has been minimized.

@Oberon00

This comment has been minimized.

@iNikem

This comment has been minimized.

@MrAlias

This comment has been minimized.

@MrAlias

This comment has been minimized.

anuraaga
anuraaga previously approved these changes Mar 18, 2021
Copy link
Contributor

@anuraaga anuraaga left a 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.

Copy link
Member

@yurishkuro yurishkuro left a 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.

@MrAlias

This comment has been minimized.

@yurishkuro

This comment has been minimized.

@Oberon00

This comment has been minimized.

@iNikem

This comment has been minimized.

@MrAlias MrAlias dismissed stale reviews from yurishkuro, anuraaga, and Oberon00 March 22, 2021 18:04

Feedback has moved PR to a different solution

@MrAlias MrAlias changed the title Remove non-blocking requirement from span API End Clarify non-blocking requirement from span API End Mar 22, 2021
@MrAlias
Copy link
Contributor Author

MrAlias commented Mar 22, 2021

@anuraaga @Oberon00 @yurishkuro @iNikem I've updated based on the feedback. PTAL

@MrAlias MrAlias added the spec:trace Related to the specification/trace directory label Mar 22, 2021
Add missing word.
MrAlias and others added 2 commits March 23, 2021 16:28
Co-authored-by: Christian Neumüller <christian+github@neumueller.me>
@carlosalberto carlosalberto merged commit 161f516 into open-telemetry:main Mar 29, 2021
@MrAlias MrAlias deleted the fix-1522 branch March 29, 2021 15:36
ThomsonTan pushed a commit to ThomsonTan/opentelemetry-specification that referenced this pull request Mar 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
spec:trace Related to the specification/trace directory
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Non-Blocking clarification for Span End method
9 participants