Span creation in tracer SDK #69
Conversation
| traceoptions: 'TraceOptions' = None, | ||
| tracestate: 'TraceState' = None | ||
| ) -> None: | ||
| if traceoptions is None: |
There was a problem hiding this comment.
According to the current specification, we need to have traceOptions.
There was a problem hiding this comment.
Or trace_options, either way looks weird though.
There was a problem hiding this comment.
I don't think the spec is meant to dictate internal names like this, especially since it's clear what this field represents. In any case we'll follow the W3C spec when the class gets serialized.
There was a problem hiding this comment.
I think it's more about "do we treat traceoptions as a single word or not" rather than "do we take the name strictly from the spec".
The W3C spec is using two words, having traceoptions is against Python naming convention.
There was a problem hiding this comment.
That's fair. I prefer traceoptions/state but agree it's more consistent to split the words up. Changed in 0f52732.
c24t
force-pushed
the
span-creation-sdk
branch
3 times, most recently
from
9dc8118 to
b64e873
Compare
| Returns: | ||
| A random 64-bit int for use as a span ID | ||
| """ | ||
| return random.getrandbits(64) |
There was a problem hiding this comment.
We might need to move this into API if we plan to support the "default behavior" in https://github.com/w3c/trace-context/blob/master/spec/20-http_header_format.md#mutating-the-traceparent-field.
We can explore this later, no need to block this PR.
There was a problem hiding this comment.
Besides what @reyang commented: I remember we kept this private in Java, to not have use this as a defacto standard for creating span ids. Something to consider?
There was a problem hiding this comment.
I don't have a strong opinion here, and think the bar for making things private is generally a lot lower in java than python. But since we don't have a convention for marking the SDK's "API", hiding everything we don't expect users to call may be a good idea.
That said, if someone is calling into the SDK package instead of just using the API they're already voiding the warranty...
|
LGTM! |
| trace_options: 'TraceOptions' = None, | ||
| trace_state: 'TraceState' = None | ||
| ) -> None: | ||
| if trace_options is None: |
|
|
||
| import typing | ||
|
|
||
| AttributeValue = typing.Union[str, bool, float] |
| Event = namedtuple('Event', ('name', 'attributes')) | ||
|
|
||
| Link = namedtuple('Link', ('context', 'attributes')) | ||
|
|
||
|
|
||
| class Span(trace_api.Span): | ||
| """See `opentelemetry.trace.Span`. | ||
|
|
||
| Users should generally create `Span`s via the `Tracer` instead of this |
There was a problem hiding this comment.
I think we should highlight this in a stronger tone - I'd really disliked users creating Spans directly (unless you guys have a specific reason/case in mind).
There was a problem hiding this comment.
No specific use case, how about removing "generally"?
| return random.getrandbits(64) | ||
|
|
||
|
|
||
| def generate_trace_id(): |
There was a problem hiding this comment.
(Same as generate_span_id)
| """ | ||
|
|
||
| def __init__(self, | ||
| cv: 'contextvars.ContextVar' = _CURRENT_SPAN_CV |
There was a problem hiding this comment.
What about using the context-propagation layer? Will that happen in the future?
There was a problem hiding this comment.
Yeah, I'll do that in another PR.
|
Hey @c24t ! Left a few comments - minor feedback, but overall it looks very neat! |
This PR adds span creation methods to the
TracerSDK. Seetest_start_span_implicitandtest_start_span_explicitfor examples of how this should work.This uses a module-level context variable for now, I'll replace it with our own context API in a separate PR. It also sets a global module-level
tracerwhich doesn't square with the existing loader logic, also to be replaced in a follow-up PR.