store root span info directly in context instead of request attribute

[pdelewski]

Stop relying on library request attribute and use context instead to store information about root span

[welcome]

Thanks for opening your first pull request! If you haven't yet signed our Contributor License Agreement (CLA), then please do so that we can accept your contribution. A link should appear shortly in this PR if you have not already signed one.

[brettmc]

I will look into why AWS build is failing. It looks like there is at least a linting issue with psr15 though: 1) Psr15/src/Psr15Instrumentation.php (braces). Can you fix that (and run make all to verify), then I'll merge it.

[pdelewski]

I will look into why AWS build is failing. It looks like there is at least a linting issue with psr15 though: 1) Psr15/src/Psr15Instrumentation.php (braces). Can you fix that (and run make all to verify), then I'll merge it.

ok, will do that

[codecov]

Codecov Report

Merging #84 (bbf87b7) into main (e34f242) will decrease coverage by 0.08%.
The diff coverage is 0.00%.

Additional details and impacted files

@@             Coverage Diff              @@
##               main      #84      +/-   ##
============================================
- Coverage     80.60%   80.51%   -0.09%     
  Complexity      330      330              
============================================
  Files            34       34              
  Lines          1000     1001       +1     
============================================
  Hits            806      806              
- Misses          194      195       +1     

Flag	Coverage Δ
7.4	89.66% <ø> (ø)
8.0	80.21% <0.00%> (-0.09%)	⬇️
8.1	80.31% <0.00%> (-0.09%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...Instrumentation/Psr15/src/Psr15Instrumentation.php	0.00% <0.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e34f242...bbf87b7. Read the comment docs.

[brettmc]

This probably also breaks the Slim auto-instrumentation, did you want to look at that in this PR, or in a new one?

[pdelewski]

This probably also breaks the Slim auto-instrumentation, did you want to look at that in this PR, or in a new one?

Don't know exactly what do you mean, this coverage report? (Seems that now all checks are passing)
Anyway, I would prefer small PRs if possible, so if work can be done in new PR then it's better option IMO

[brettmc]

Don't know exactly what do you mean, this coverage report?

No, this: https://github.com/open-telemetry/opentelemetry-php-contrib/blob/main/src/Instrumentation/Slim/src/SlimInstrumentation.php#L44 It is trying to get the root span from a request attribute, which psr-15 doesn't do now.

[Nevay]

It is trying to get the root span from a request attribute, which psr-15 doesn't do now.

This is also the main reason why I dislike this change; it re-adds the direct dependency on the PSR15 package for instrumentation that wants to modify the request span.

This will be problematic if the request is also handled by non-psr implementations.
A while ago I wrote a PoC runtime that runs a symfony kernel inside a amphp/http-server request handler; the runtime converts the amphp request into psr request into symfony request and maintains all request attributes (common concept for server requests), including the associated root span.
Storing the span in an amphp/symfony/etc. specific context key would result in losing the root span association unless every instrumentation checks every context key -> If we want to store the span in the context we should use a context key that is not related to any specific request implementation.

[Nevay]

I think that this won't work with preloading, should be moved into ::register().

[Nevay]

Suggested change

	public static $rootSpan;
	/** @var ContextKeyInterface<SpanInterface> */
	public static ContextKeyInterface $rootSpan;

IMO should also be private and only accessible via getter to prevent modification - or use an enum for this context key.

[pdelewski]

It is trying to get the root span from a request attribute, which psr-15 doesn't do now.

This is also the main reason why I dislike this change; it re-adds the direct dependency on the PSR15 package for instrumentation that wants to modify the request span.

This will be problematic if the request is also handled by non-psr implementations. A while ago I wrote a PoC runtime that runs a symfony kernel inside a amphp/http-server request handler; the runtime converts the amphp request into psr request into symfony request and maintains all request attributes (common concept for server requests), including the associated root span. Storing the span in an amphp/symfony/etc. specific context key would result in losing the root span association unless every instrumentation checks every context key -> If we want to store the span in the context we should use a context key that is not related to any specific request implementation.

Don't understand your last sentence, why we need check every context key. Is context key I introduced somehow related to specific request implementation?. Anyway, still I think that storing span in context is better than relying on something that we are instrumenting (request in this case) and is self-contained. True, that we are depending on context and contextkey, but in current implementation we depend on request. Besides of that, I don't think we need to instrument performRouting just to update name, however if that will not be possible, we need to check one additional contextkey.

[pdelewski]

Don't know exactly what do you mean, this coverage report?

No, this: https://github.com/open-telemetry/opentelemetry-php-contrib/blob/main/src/Instrumentation/Slim/src/SlimInstrumentation.php#L44 It is trying to get the root span from a request attribute, which psr-15 doesn't do now.

My plan is to get rid hooking into performRouting and update root span based on information we have on PSR-15 instrumentation, don't know yet if that's 100% possible due to cardinality.

[pdelewski]

This can be moved out from Psr15Instrumentation then we will not depend on it but on something generic

[pdelewski]

This is also the main reason why I dislike this change; it re-adds the direct dependency on the PSR15 package for instrumentation that wants to modify the request span.

One of the solution to this problem would be base class that will be part of SDK and will contain functionality of setting/getting $rootSpan

[pdelewski]

Don't know exactly what do you mean, this coverage report?

No, this: https://github.com/open-telemetry/opentelemetry-php-contrib/blob/main/src/Instrumentation/Slim/src/SlimInstrumentation.php#L44 It is trying to get the root span from a request attribute, which psr-15 doesn't do now.

Just for now we can solve that in another PR as we need to introduce dependency to opentelemetry-php-contrib, but as I written in other comment, this dependency can be removed with, for instance, base class for Instrumentation that will have functionality for setting/getting $rootSpan and maybe other things in future

[brettmc]

One of the solution to this problem would be base class that will be part of SDK and will contain functionality of setting/getting $rootSpan

I think being able to retrieve the root span is very useful, for precisely this reason. This is actually the very first question I had when trying to use otel for the first time - we don't always have enough information when creating the root span to give it a good name as advised in spec: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/semantic_conventions/http.md?plain=1#L32
Being able to get the root span seems like a really clean way to decouple slim/psr15, and would simplify auto-instrumentation for any framework where routing isn't the very first thing to happen, IMO.

[Nevay]

Is context key I introduced somehow related to specific request implementation?

My comment becomes obsolete if we introduce a global http server span context key; keeping the context key in the PSR15 instrumentation package would likely lead to the introduction of another context key for symfony, laravel etc. -> no longer interoperable.

How do we handle sub-requests if we store the request span in the context?
For example calling $app->handle(new Request('GET', '/example')) from within a slim route callable should create a new span, the routing middleware should update the name of this span instead of the initial request span.

[brettmc]

How do we handle sub-requests if we store the request span in the context?

If we activate a context entry using the same key to represent a http server request and any sub-requests, then I think it can work. I'm assuming here that you'd only update a "http server" span's name when processing that request (ie, we're not trying to update the main request from a sub-request)

Here's a draft PR which adds a context key, and an example of it being used: open-telemetry/opentelemetry-php#864 - thoughts?

[Nevay]

If we activate a context entry using the same key to represent a http server request and any sub-requests

How do we detect that we are handling a sub-request?
We cannot rely on object identity because requests are immutable. IMO the only solution is to store some kind of marker in the request attributes (and if we do this: what is the advantage of doing this over just storing the span as request attribute as we do currently?).

[pdelewski]

If we activate a context entry using the same key to represent a http server request and any sub-requests

How do we detect that we are handling a sub-request? We cannot rely on object identity because requests are immutable. IMO the only solution is to store some kind of marker in the request attributes (and if we do this: what is the advantage of doing this over just storing the span as request attribute as we do currently?).

Storing additional marker in request is not a solution because as you mentioned, it's not better from what we already have. What about storing a counter in context (so in result there would be two properties - httpSpan and counter), root span would have always 0 value and sub-request > 0. Creating span for sub-request would increase counter, end span operation would decrease it. (everything with assumption that @brettmc mentioned, that we are not updating main request from a sub-request, but that's the case that we cannot handle even now IMO)

[pdelewski]

Forget about my last comment, it's wrong

[pdelewski]

@Nevay @brettmc Starting this PR I had in mind a case where we will need update root span, but not scenario mentioned by @Nevay. As of now, I don't see solution to distinguish sub-request that would be better from what we have now. Having said that, this bothers me as I can imagine library or framework where storing state in external object (the one provided by library) will not be possible

[brettmc]

I agree, @pdelewski - I think that using a context key defined in API/SDK can work but it's not better than storing an attribute on the request (plus, it's an extra scope to remember to detach). The other benefit of storing it in the request itself is that there is no doubt about which "root span" belongs to which request when there are multiple.
I think I'm in favour of stopping this PR and continuing to use a request attribute...?

[pdelewski]

Yes, we can close it and continue with current approach