SDK Tracer treats invalid span parent like null (fixes #233) #235
Conversation
toumorokoshi
requested review from
a-feld,
c24t,
carlosalberto,
lzchen,
Oberon00 and
reyang
as code owners
| @@ -364,7 +364,7 @@ def create_span( | |||
| span_id = generate_span_id() | |||
| if parent is Tracer.CURRENT_SPAN: | |||
| parent = self.get_current_span() | |||
| if parent is None: | |||
| if parent in {None, trace_api.INVALID_SPAN_CONTEXT}: | |||
There was a problem hiding this comment.
You create a set each time the method is called here, which is very inefficient. in (None, trace_api.INVALID_SPAN_CONTEXT) would already be better but since create_span is a hot path, I'd even suggest plain old parent is None or parent == trace_api.INVALID_SPAN_CONTEXT.
But since SpanContext(0x0, 0xbadcoffee) != trace_api.INVALID_SPAN_CONTEXT, I guess we should even check parent is None or not parent.trace_id (parent.is_valid() might be overly strict since it also checks parent.span_id != 0 but maybe that's ok)
EDIT: Actually, since we don't override hash/eq for SpanContext, we effectively check parent is trace_api.INVALID_SPAN_CONTEXT, meaning that SpanContext(0, 0) would not cause us to generate a new ID.
EDIT2: Or was it actually the intent to treat trace_api.INVALID_SPAN_CONTEXT as special object similar to Tracer.CURRENT_SPAN ?
There was a problem hiding this comment.
Good questions.
My understanding is that INVALID_SPAN_CONTEXT is a special object, in fact I have some concerns because the B3 propagator returns SpanContext(0,0) and not INVALID_SPAN_CONTEXT: #233 (comment).
I think parent is None or not parent.is_valid() would work just fine. Maybe a little bit slower but more flexible as propagators don't have to return INVALID_SPAN_CONTEXT.
There was a problem hiding this comment.
Shouldn't parent be assigned to None when it is INVALID_SPAN_CONTEXT (is not valid in general)`?
If we don't do that other components like exporters will have to check also if parent is valid or not to understand if the span is a root one.
...
if not parent.is_valid():
parent = None
if parent is None:
....
Does it make sense?
There was a problem hiding this comment.
I think is_valid is a good approach. I figure INVALID_SPAN_CONTEXT should be something that people can check against, but at the same time there's nothing stopping people from generating span contexts using invalid span / trace ids.
I guess a bigger risk there is that one can load invalid span and trace ids from header values, and those would generate an invalid span context that is not the same object as INVALID_SPAN_CONTEXT.
Shouldn't
parentbe assigned toNonewhen it isINVALID_SPAN_CONTEXT(is not valid in general)`?
If we don't do that other components like exporters will have to check also if parent is valid or not to understand if the span is a root one.... if not parent.is_valid(): parent = None if parent is None: ....Does it make sense?
Agreed, will do that.
| new_span = tracer.create_span( | ||
| "root", parent=trace_api.INVALID_SPAN_CONTEXT | ||
| ) | ||
| self.assertNotEqual( |
There was a problem hiding this comment.
Wouldn't this check have succeeded even before the fix (since we generate a new span ID but used to keep the zero trace ID)?
I suggest the more strict self.assertTrue(new_span.context.is_valid()).
Fixes open-telemetry#233. The SDK tracer will now create spans with invalid parents as brand new spans, similar to not having a parent at all. Adding this behavior to the Tracer ensures that integrations do not have to handle invalid span contexts in their own code, and ensures that behavior is consistent with w3c tracecontext (which specifies invalid results should be handled by creating new spans).
Tracer.create_span now checks is_valid for validity of the SpanContext. This is more comprehensive than a simple identity check for INVALID_SPAN_CONTEXT. Setting the parent to none if the parent context is invalid, reducing logic to handle that situation in downstream processing like exporters.
toumorokoshi
force-pushed
the
feature/handle-invalid-spancontext
branch
from
2c87e44
to
4ee81ab
Compare
moving the generate span / trace id methods back to API. no longer needed due to open-telemetry#235 moving test service to it's own module. modifying shell script to use bourne shell, using posix standard location
moving the generate span / trace id methods back to API. no longer needed due to open-telemetry#235 moving test service to it's own module. modifying shell script to use bourne shell, using posix standard location
* fix: set attributes from SpanOptions * fix: add more tests
Fixes #233. The SDK tracer will now create spans with invalid parents
as brand new spans, similar to not having a parent at all.
Adding this behavior to the Tracer ensures that integrations do not have
to handle invalid span contexts in their own code, and ensures that behavior
is consistent with w3c tracecontext (which specifies invalid results should
be handled by creating new spans).