-
Notifications
You must be signed in to change notification settings - Fork 657
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
Parent is now always passed in via Context, intead of Span or SpanContext #1146
Changes from 13 commits
4e87e13
5180478
9e707e3
3493bce
34b6cc1
3e2ffab
eceae0f
865891c
1acb6db
01502ca
42c574f
f82bd23
76107e5
b388cd8
3114a13
a2dfae9
594f9f8
aa1e329
a451c4f
c6f5289
2585e3f
6a910fa
3b49754
bf512da
77fba4a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -346,7 +346,7 @@ class Span(trace_api.Span): | |
name: The name of the operation this span represents | ||
context: The immutable span context | ||
parent: This span's parent's `opentelemetry.trace.SpanContext`, or | ||
codeboten marked this conversation as resolved.
Show resolved
Hide resolved
|
||
null if this is a root span | ||
None if this is a root span | ||
sampler: The sampler used to create this span | ||
trace_config: TODO | ||
resource: Entity producing telemetry | ||
|
@@ -683,7 +683,7 @@ def __init__( | |
def start_as_current_span( | ||
self, | ||
name: str, | ||
parent: trace_api.ParentSpan = trace_api.Tracer.CURRENT_SPAN, | ||
parent: Optional[context_api.Context] = None, | ||
kind: trace_api.SpanKind = trace_api.SpanKind.INTERNAL, | ||
attributes: types.Attributes = None, | ||
links: Sequence[trace_api.Link] = (), | ||
|
@@ -694,27 +694,23 @@ def start_as_current_span( | |
def start_span( # pylint: disable=too-many-locals | ||
self, | ||
name: str, | ||
parent: trace_api.ParentSpan = trace_api.Tracer.CURRENT_SPAN, | ||
parent: Optional[context_api.Context] = None, | ||
kind: trace_api.SpanKind = trace_api.SpanKind.INTERNAL, | ||
attributes: types.Attributes = None, | ||
links: Sequence[trace_api.Link] = (), | ||
start_time: Optional[int] = None, | ||
set_status_on_exception: bool = True, | ||
) -> trace_api.Span: | ||
if parent is Tracer.CURRENT_SPAN: | ||
parent = trace_api.get_current_span() | ||
|
||
parent_context = parent | ||
if isinstance(parent_context, trace_api.Span): | ||
parent_context = parent.get_context() | ||
parent_context = trace_api.get_current_span(parent).get_context() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Now that there is the concept of a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And maybe even renaming the variable from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would +1 on "get_span_context", although with open-telemetry/opentelemetry-specification#1033 that might not even be necessary. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I renamed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that's fine, I don't know if I have a strong opinion on the attribute name on the span. And I'm completely comfortable with taking these PRs piecemeal too. |
||
|
||
if parent_context is not None and not isinstance( | ||
parent_context, trace_api.SpanContext | ||
): | ||
raise TypeError("parent must be a Span, SpanContext or None.") | ||
raise TypeError("parent_context must be a SpanContext or None.") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess this type validation is still valuable. But practically speaking, this would only be an issue if someone directly manipulates the context, and doesn't use the API functions. Is that a common enough case to even catch here? Alternatively I suppose this is cheap enough. |
||
|
||
if parent_context is None or not parent_context.is_valid: | ||
parent = parent_context = None | ||
parent_context = None | ||
trace_id = self.source.ids_generator.generate_trace_id() | ||
trace_flags = None | ||
trace_state = None | ||
|
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'm wondering if the variable is worth renaming. "parent" was vague to begin with, and if you look at any descriptions of how tracing works, one would immediately think "parent" refers to the span (since you specifically call the span a child of the parent span).
This would also be useful to rename since new API users will get exceptions when their existing code passes an argument as parent. Rather than get a new slightly confusing error ("why is the API now rejecting this previously valid value?"), they will get an argument not supported error, and look up the docs to see the variable changed.
parent_context
is the first thing that comes to mind, but I worry a user would get it confused with the parent SpanContext.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.
@toumorokoshi yeah I had a similar thought around the name. Looking at some of the other implementations, it appears
parent
andparent_context
are the current names used. I'm happy to change it to parent_context as it's at least a bit more meaningful than parent. The alternative i can think of would be to just call itcontext
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.
Either sounds great!
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'd say
context
is the better name. AContext
object passed in this argument may or may not contain a parentSpan
, making the nameparent_context
less accurate andcontext_with_or_without_the_parent_span
is a bit long.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.
Thanks for the feedback, went with
context
.