open-telemetry / opentelemetry-js

OpenTelemetry JavaScript Client
https://opentelemetry.io
Apache License 2.0
2.76k stars 810 forks source link

Reuse of parentSpanId for NoopSpans #2027

Open Flarna opened 3 years ago

Flarna commented 3 years ago

In case a span is dropped because of sampling decision the Tracer returns a NoopSpan with a new, unique SpanId as described in spec.

But if the Tracer returns a NoopSpan because of disabled instrumentation the SpanId from parent is reused (see here).

Should we create a new SpanId also in that case? Or is this anyway something not relevant in real world?

vmarchaud commented 3 years ago

If i'm not mistaken this is mostly to propagate the correct info across process boundary, even though the span shouldn't noop doesn't mean it isn't part of a bigger trace

Flarna commented 3 years ago

The difference of the two cases is that non sampled spans get an unique ID and backend will show that a span is missing (assuming a non parent based sampler here). The unique spanId ensures also that users can use the spanId for logging, as a key in a map,.. without the risk of having duplicates.

On the other hand a span suppressed via disable instrumentation has the same spanId as it's parent. It's a NoopSpan so it will be never exported but the case above with a user doing logging use spanId as key in a map would be broken. a propagator would inject this span so for the backend it would look like the suppressed span doesn't exist at all.

suppress is used by HTTP based exporters. My gut feeling is that such spans should either have a unique spanId as others or an invalid spanId. And we should suppress inject of invalid spanIds.

github-actions[bot] commented 2 years ago

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.