-
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 the order of mutually exclusive params when creating spans. #136
Clarify the order of mutually exclusive params when creating spans. #136
Conversation
In languages that need to take all the three parameters at the same time when creating a `Span`, | ||
parent `Span` should take precedence, then remote parent `SpanContext`, and `root` comes last. | ||
For example: | ||
3. `tracer.start_span(name='span', parent_span=span1, parent_span_context=ctx, root=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.
This seems to be language specific. Do we intend to document language specific behavior here? For Python, probably we could take a different approach, do we want to put it in the spec as well (I guess no)?
_default = object()
def start_span(tracer, name, parent=_default):
if parent is _default:
parent = tracer.get_current_span()
...
tracer.start_span('foobar') # create a span as a child of the current span (or root span if there is no current span)
tracer.start_span('foobar', parent=parent_span) # create a child span under span 'parent_span'
tracer.start_span('foobar', parent=None) # create a root span
tracer.start_span('foobar', parent=ctx) # create a span as a child of parent context
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.
Yes this PR is a bit language-specific but it's meant to reduce ambiguity in behavior. I think the approach you proposed is still consistent with the current spec, since we mentioned local parent span and remote parent span context are optional parameters so you're free to implement it with default values.
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.
I agree that this is language specific, but I think it's better to keep it this time like this for clarity purposes, while we figure out what other languages do.
We can refine it later ;)
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.
I agree that this is oddly implementation-specific for the spec; it sounds more like a description of a specific language API. It's not clear to me that the spec should require that the start_span
method should even handle all three cases.
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.
LGTM, though it seems to be a bit language specific.
3e29166
to
d68572f
Compare
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.
Many (most?) languages allow for patterns that would let us avoid the problem solved by both this and #131 (closed too quickly to discuss there), e.g., the Python approach suggested by @reyang or an interface.
I propose that rather than focusing on workarounds, we should encourage patterns that let us avoid the problems altogether.
766f4b0
to
9f8e809
Compare
Is this comment blocking the PR? Do you have a suggestion on how to rephrase this better? |
Oh, we can do that, but we need a few people from other languages to chime in and make sure one way or another sounds valid ;) (Also, we can always change stuff later on, of course) |
I suggest to merge this as it addresses the immediate problem of describing API surface and clarifying the intent and ordering of fields. Let's iterate on this doc over time |
Please file an issue for that. Though I'm afraid if we go with this approach we may end up enumerating the behavior for each language in the spec. |
New name for docs team.
Fixes #133.
The current order is local parent span -> remote parent span context -> no parent, if more than one out of the three parameters are set. Feel free to propose an alternative.