Skip to content
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

Propagators must not return an invalid span if nothing could be extracted from the carrier #1765

Closed
mariojonke opened this issue Apr 13, 2021 · 5 comments · Fixed by #1811 or open-telemetry/opentelemetry-python-contrib#488
Assignees
Labels
bug Something isn't working triaged

Comments

@mariojonke
Copy link
Contributor

Currently most propagators set an INVALID_SPAN into the context on extract in case a value cannot be parsed from the carrier (see tracecontext, b3, jaeger).
Spec however states that a new value must not be set in such a case:

If a value can not be parsed from the carrier, for a cross-cutting concern, the implementation MUST NOT throw an exception and MUST NOT store a new value in the Context, in order to preserve any previously existing valid value.

Propagators should return the passed in context (or the current one if none was given) if nothing could be extracted. Also in other language SIGs (go, java, dotnet, ...) just the unmodified context is returned.

@mariojonke mariojonke added the bug Something isn't working label Apr 13, 2021
@marcinzaremba
Copy link
Contributor

marcinzaremba commented Apr 13, 2021

These issues/PR might be of your interest:

The discussion was already started, however I also believe problem is not fully solved yet.

@mariojonke
Copy link
Contributor Author

Looks like the outcome of the discussion in #1732 was to return a context with an INVALID_SPAN in case the passed in context is None and the propagator could not extract a value.
I think this behavior is incorrect. All the APIs which accept a context (set_value, get_value, inject, extract, ...) treat None implcitly as the current context. See also the doc string on the propagator's extract method. So on a successfull extract the propagator would update the current context with whatever got extracted. But on the other hand this would also mean for an unsuccessful extract the propagator has to return the unmodified current context to not contradict with the spec.

@srikanthccv
Copy link
Member

It could come as very surprising for me if current context is used as parent span context when there is some deserialization error extracting remote span context.

But on the other hand this would also mean for an unsuccessful extract the propagator has to return the unmodified current context to not contradict with the spec.

Spec doesn't say that unsuccessful extract the propagator has to return the unmodified current context. It only says if there is a passed in context and If a value can not be parsed from the carrier, in order to preserve any previously existing valid value we shouldn't modify it (which we are doing now for b3 and should be updated for others).

I would be fine with seeing two or more traces which ideally should be under one trace because I messed up serialization but if I find spans under some different trace it is incorrect and strange. From what I can see in linked the other languages they make it mandatory to pass context unlike python. If there is issue with doc string or it conveys wrong message then we should update it or reword it but I don't think we shouldn't return unmodified current context.

@mariojonke
Copy link
Contributor Author

I can see the problem of the propagator continuing the current active trace if there is a deserialization problem and returning the current context will exactly lead to this behavior.
The acutal problem i guess lies with the optionality of the context argument. For most of the context API an absent context refers to the current context. I agree that this does not make much sense for the case of extract but leads to the question to what an absent/default context should actually refer to in this case. As pointed out by @lonewolf3739 other SIGs do not have this problem since the context argument always has to be passed explicitly and it is for the user of the API to decide whether to pass the current, root or some other context. So for python it should be clearly defined what a missing context means for the extract operation.

Looking at the b3 propagator it returns a mix of current and "root" context by appling the INVALID_SPAN to the current context. This looks somewhat strange to me. Wouldn't it make more sense to just use the root context as default if no context argument was provided (i.e. define root context for extract if none was provided)?
Something like:

if context is None:
    context = Context()
...
if could_not_extract:
    return context
...
return set_span_in_context(NonRecordingSpan(...), context)

A call like extract(carrier, getter) in python would then translate to something like extract(ROOT_CONTEXT, carrier, getter) in another language.
I'm not sure but i don't think it is possible in other SIGs to have the propagator return a constellation like in python (current context + INVALID_SPAN) regaldless of what arguments are passed to extract.
So i think for extract the default context should actually refer to the root context (i could be wrong though).

@lzchen
Copy link
Contributor

lzchen commented Apr 27, 2021

@mariojonke
Could you bring this up in the SIG meeting on Thursday?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working triaged
Projects
None yet
5 participants