Generic mechanism for preventing multiple Server/Client spans

[iNikem]

Otel spec says the following about SERVER and CLIENT spans:

• SERVER [...] span is the child of a remote CLIENT
• CLIENT [...] span is the parent of a remote SERVER

Together with the remaining section this brings me to the conclusion, that spec demands having at most 1 span of kind SERVER while a trace passes single JVM. The same for CLIENT. Partially this requirement is already encoded into instrumentations. E.g. incoming HTTP instrumentations attach entry-point span to a request and avoid creating new spans if they find one already attached to a request. But I propose to make this requirement more explicit.

Let us do the following:

1. Whenever SERVER or CLIENT span is created, associate it with the current Context under the corresponding key
2. Before attempting to create new SERVER or CLIENT span instrumentations should check if one is already present in the current context. If yes, then no new span should be created.
3. Until we have semantic tracers, these methods can leave in BaseDecorator.
4. Explicitly document that instrumentation may augment already existing SERVER or CLIENT span with additional information. E.g. to change span name or to add transport level attributes.

This will directly address #440 as well.

[trask]

Yes! This sounds great to me. Thanks @anuraaga for bringing attention to this, and thanks @iNikem for this proposal. I very much like the idea of using the context to track both the current SERVER and CLIENT spans.

Tracking the SERVER span like this will allow us to finally resolve the last item in #50:

JAX-RS instrumentation was not commented out, but there was potentially some loss during the transition, as it previously used getLocalRootSpan() and now just uses the parent span, which makes all the same tests pass, but may have some impact in the real world.

[iNikem]

@tylerbenson I would love to hear your opinion here :)

[trask]

I think #507 is likely a prerequisite for this issue

[tylerbenson]

@iNikem it sounds sensible to me, but do we have generic access to the context?

[iNikem]

wdym? Context.current is always available, no?

[tylerbenson]

I thought we were trying to avoid direct access to the grpc context like that... isn't it more of an implementation detail?

[trask]

context used to be an implementation detail, but got promoted in OTEP 66

[trask]

I think this applies to PRODUCER/CONSUMER spans too (so all except INTERNAL)

[trask]

marking p2 because another p2 depends on this

[trask]

I closed this too soon, we haven't implemented duplicate check generically for SERVER spans yet at HttpServerTracer level

[pavolloffay]

When does this problem happen?

For instance does it happen in a situation where mutiple multiple server side instrumentations create spans e.g. servlet filter and then jax-rs? Or was this just a proposal to enforce single client, server span situation?

[trask]

hey @pavolloffay!

I don't think any of the instrumentation currently creates duplicate SERVER spans, so this proposal is mostly preventative for SERVER spans, and will replace some existing logic that already existed to prevent duplicate SERVER spans.

This mechanism was a bit more needed for CLIENT spans, e.g. #440

[pavolloffay]

thanks for the pointer @trask.

We had similar issues OT for server spans when people used multiple layer instrumentation for incomming requests. We ended up checking for parent span when the context was extracted. If there was active parent span it was used and just added lables e.g. @Path to enrich the data.

For the client spans the situation might be similar the lower layers can check if the parent is client and act accordingly e.g. add labels that are unique or event if the timing for that layer is important (it sometimes is).

[trask]

Just adding a note to remember about database CLIENT spans that make http (CLIENT) calls

[jkwatson]

It seems like there are 3 options, really.

• suppress entirely (what we do today)
• allow willy-nillly wild west span creation
• suppress span creation, but enable amending attributes in the suppressed child.

Should we add a config option for these 3 options, defaulting to the current behavior?

[trask]

We may only need one of option 3 / option 1 (I think either one of those would likely be acceptable for vendors that expect/prefer no nested client spans).

I'm not sure what to do in option 3 if a database client span makes 2 http requests?

[jkwatson]

urgh. good point.
Maybe option 4:

• sub spans get turned into events, with their attributes intact. We only would lose the duration of the sub-span, in that case (or maybe we introduce a semantic convention for the duration of a span event?)

[iNikem]

Make it two events: starts and finish. But I quite like the idea of event duration as well

[trask]

adding a note here that there are now generic tests for suppression of nested http client spans (#2646)

[iNikem]

@open-telemetry/java-approvers Did https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/docs/suppressing-instrumentation.md#enable-instrumentation-suppression-by-type solve this issue? Can we close it now?

[trask]

👍 additional proposals/discussions can be opened as new issues