Server instrumentations should look for parent spans in current context before extracting context from carriers

[owais]

Today server instrumentations such as Django always extract the parent span from the incoming request headers. This may not always be ideal. For example, there may be other instrumented components that wrap an instrumented web framework such as WSGI. In such cases WSGI would already have generated a server span and used the remote span as parent. If Django did the same, traces would not make a lot of sense. Depending on whether a remote span is present in the incoming request's carrier, Django and WSGI spans would be completely different traces or be siblings instead of parent and child.

To avoid situations like this, all server instrumentations should first check if an active span is present in the current context by calling opentelemetry.api.trace.get_current_span() and use it as the parent when present. When no active span is found in the current context, the instrumentation should try to extract remote span context from the incoming request and use that as the parent.

This needs to be done for all instrumentations that generate server spans.

Changes that need to be made:

• If a span is found in the current context, then:
  • create a new span with kind set to INTERNAL
  • use the span found in current context as parent
• If no span is in the current context, then:
  • create a new span with kind set to SERVER
  • extract remote span context from incoming request and use it as parent

[srikanthccv]

Just the other day I have seen somebody using both WSGI middleware and Django instrumentation. This makes sense in such scenarios but I am wondering if there are any cases where active span is not from remote context that we end up using it instead of actual remote context? I don't know any but just adding here for visibility.

[srikanthccv]

It could also be possible that there is a wrapper which has some instrumentation but not necessary that Django instrumentation spans should be child spans of them. Some raw thought; I don't know if this makes sense.

[owais]

I think it is possible but even if it happens, I think it should be considered an error on part of the instrumentation/user who activates such a span in Django's request/response cycle context.

For added safety, Django span can check if parent span is a SERVER span for additional safety but I'm not sure if it is worth it and it is possible WSGI can create another internal child span in future.

FWIW, both Node and Java follow this pattern today in order to enhance parent spans. Some Node/Java grab parent spans and add attributes or update names. This would be akin Django instrumentation not generating a new span and instead adding additional information to WSGI span. I don't think we can or should do that given how Python ecosystem is different from Node/Java but it does at least set the precedent for server instrumentations looking at parent spans.

(Using django as a placeholder for any python web framework)

[lzchen]

Couple of questions to clarify. First let's call the situation of wsgi wrapping other frameworks like django as situation X.

If a span is found in the current context, then:

If a span is found in the current context, that means the parent span was created in the same process and can be assumed to be of situation X correct? Which will then result in the trace looking like SERVER (from wsgi) -> INTERNAL (from django) -> .... ONLY if user is instrumented with BOTH wsgi instrumentation and django instrumentation. What about the case when the user is only instrumented with django instrumentation? Does this behavior cover ALL uses and does it make sense? Is there ever a case where, there is a current span in the current context (so there is a parent span in the same process) but it is NOT situation X?

[andresbeckruiz]

I'd like to take this on as my first issue! Would appreciate any guidance on getting started.

cc: @alolita

[owais]

If a span is found in the current context, that means the parent span was created in the same process and can be assumed to be of situation X correct? Which will then result in the trace looking like SERVER (from wsgi) -> INTERNAL (from django) -> .... ONLY if user is instrumented with BOTH wsgi instrumentation and django instrumentation.

Right.

What about the case when the user is only instrumented with django instrumentation?

If no local parent span is present, Django span will become the SERVER span.

Does this behavior cover ALL uses and does it make sense? Is there ever a case where, there is a current span in the current context (so there is a parent span in the same process) but it is NOT situation X?

I don't know of any cases like this but it could be possible. However, I think if the source of the parent span does not intent to represent the current HTTP request, it probably should not set itself into the current context. If we wanted to be safer, we could require that each server instrumentation also confirms that their parent span has kind set to SERVER and only in that case do what is proposed here. It will solve this specific case (wsgi(django)) but it is not hard to imagine wsgi or a similar system generating more than one spans in future.

[alolita]

@codeboten @lzchen can you please assign @andresbeckruiz this issue to work on. Thx.

[codeboten]

@alolita @andresbeckruiz done 👍

[andresbeckruiz]

Hello, I'm making some progress on this issue but I am confused as to how I would extract the remote span context from an incoming request. Is there a span context attribute for flask.request or a method available that I missed in the documentation?

[owais]

@andresbeckruiz All instrumentations extract remote context already using the configured global propagator. We just need to update the logic where the context is used and add a decision about whether to use the extracted context or a locally present span in active scope. Look for global propagator usage in instrumentation packages.

@lzchen does this address your concerns? #445 (comment)

[srikanthccv]

Relevant issue on spec repo open-telemetry/opentelemetry-specification#1767 Probably not.

[ashu658]

I would like to pick this up as my first issue! Please let me know if its okay. Would appreciate any guidance on getting started with this.
cc @owais

[lzchen]

@ashu658
We have split this issue up into different issues pertaining to individual instrumentations. Feel free to comment on the ones you want to work on and we will assign them to you :D

[ashu658]

sure @lzchen

[lzchen]

@owais
Resurfacing this due to a recent discussion in 3/17 SIG.
Recently there were a couple of PRs that required the checking of SpanKind in the instrumentations themselves. Besides from the regeression issue that was found, this brought up the way in we are checking for whether or not a user has instrumented with multiple instrumentations that have the same code path in this feature. @srikanthccv also brought up a good point here.

So the question is, should we be relying on the implementation of detail of SpanKind being internal to assume that there are multiple instrumentations? We already have a similar mechanism for checking http spans like in urllib. Maybe we need to have a consistent mechanism for this?