Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Span creation in tracer SDK #69
Span creation in tracer SDK #69
Changes from 10 commits
23a7a93
dbfdc60
9479b2d
3d73c1f
c3e0544
6aed32b
af40573
c9f293c
48f2fe4
89f1a0c
7a796e1
f3d9ed3
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
According to the current specification, we need to have
traceOptions
.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.
Or
trace_options
, either way looks weird though.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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fair. I prefer
traceoptions/state
but agree it's more consistent to split the words up. Changed in 0f52732.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.
+1
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 think we should highlight this in a stronger tone - I'd really disliked users creating
Span
s directly (unless you guys have a specific reason/case in mind).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.
No specific use case, how about removing "generally"?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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...
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.
(Same as
generate_span_id
)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.
What about using the context-propagation layer? Will that happen in the future?
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.
Yeah, I'll do that in another PR.