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
Store parent context in span builder #399
Conversation
The span builder currently holds a parent `SpanContext`, and `Context` information is passed separately when starting a span directly or via a builder. This poses a few problems, first you can assign a context which contains an active span _as well as_ a builder with a parent span context, which can lead to confusion, and secondly we have to construct a wrapper context when sampling even in cases when the passed in context already contains an active span. This patch resolves this by changing the span builder's parent context to instead store a `Context` directly. This allows the builder methods to have a sigle context to look at when searchig for parent span contexts, and allows the wrapper context to onlly be created for sampling when it is facilitating a remote parent.
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 feel a little confusing that parent_span_context
is kind of part of the parent_ctx
, and it feels like we pass the duplicate element into the make_sample_decision
function. But overall I think it looks good.
Codecov Report
@@ Coverage Diff @@
## master #399 +/- ##
==========================================
- Coverage 49.12% 49.11% -0.02%
==========================================
Files 66 66
Lines 5394 5400 +6
==========================================
+ Hits 2650 2652 +2
- Misses 2744 2748 +4
Continue to review full report at Codecov.
|
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 👍
The span builder currently holds a parent
SpanContext
, andContext
information is passed separately when starting a span directly or via a builder. This poses a few problems, first you can assign a context which contains an active span as well as a builder with a parent span context, which can lead to confusion, and secondly we have to construct a wrapper context when sampling even in cases when the passed in context already contains an active span.This patch resolves this by changing the span builder's parent context to instead store a
Context
directly. This allows the builder methods to have a single context to look at when searching for parent span contexts, and allows the wrapper context to only be created for sampling when it is facilitating a remote parent.