Consider returning noop/non-recording span instead of undefined in the case of no active span

[dyladan]

Original discussion here: https://github.com/open-telemetry/opentelemetry-js-api/pull/175 Related otep discussion: https://github.com/open-telemetry/oteps/issues/216

The user who opened this PR wanted us to return a valid span object any time there is no active span. The related otep discussion shows that other languages are doing this:

Go returns a noop span: open-telemetry/opentelemetry-go@d616df6/trace/context.go#L48 Ruby returns an invalid span: open-telemetry/opentelemetry-ruby@main/api/lib/opentelemetry/trace.rb#L52 DotNet returns an invalid span: open-telemetry/opentelemetry-dotnet@6b7f2dd/src/OpenTelemetry.Api/Trace/Tracer.cs#L48 PHP returns an invalid span: open-telemetry/opentelemetry-php@2c00772/src/API/Trace/AbstractSpan.php#L21 JS returns undefined: open-telemetry/opentelemetry-js-api@main/src/trace/context-utils.ts#L35

I'm opening this issue so that we can discuss the merits and decide what to do. I don't have a strong opinion either way here, but I do think it might be considered a breaking change in behavior.

[dmathieu]

For additional context on what prompted me to open the original otep, the current behavior means whenever we retrieve a span from the context, we need to check if we're not getting nil. So instead of being able to do trace.getSpan(context.active()).setAttributes({'hello': 'world'}), we need to do:

const span = trace.getSpan(context.active())
if (span) {
  span.setAttributes({'hello': 'world'})
}

We are migrating a pretty big codebase from opentracing to opentelemetry, and we are being hit quite frequently by context loss. Because of that behavior, we need to add extra checks every time we want to augment the span, which is very verbose, and does seem to go against the specification's saying on error handling:

We assume that users would prefer to lose telemetry data rather than have the library significantly change the behavior of the instrumented application.

[Flarna]

You could use a simple wrapper or optional chaining (trace.getSpan(context.active())?.setAttributes()) to avoid the repeated verbose code.

I wonder also a bit about your usecase. To my understanding spans should usually describe some operation but it seems you use it as general purpose container to add attributes without knowing who/where/why a span was created and what it describes.

If you use child spans to describe the inner operations above verbose code is not needed as tracer.startSpan() uses a parent if present on context.

[dmathieu]

Our spans have arbitrarily wide events. And the more data/context they have, they more we're able to figure things out from them. So yes, we add lots of attributes on them on middlewares (which are expected not to create child spans, so we always write on the main HTTP span, but that's another story), so we have the entire context as we know about it.

[dyladan]

Because of that behavior, we need to add extra checks every time we want to augment the span, which is very verbose, and does seem to go against the specification's saying on error handling:

I'm not sure the error handling spec is relevant as it is not an error for there not to be an active span. The method signature clearly states there could be an undefined return. Null checks are also not what I think they refer to when they say "significant change" of behavior.

[dyladan]

IMO a stronger argument is to unify the behavior with other SDKs, but I worry this may be considered a breaking change. Currently, you can check if there is an active span by null checking. If we return a valid spanlike object, then that assumption is broken. There are ways we could do this in a backwards compatible way (introduce new method, default-false config, optional argument, or similar mechanism), but we have to decide if that is worth the maintenance cost.

[github-actions[bot]]

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.

[github-actions[bot]]

This issue was closed because it has been stale for 14 days with no activity.

[svrnm]

Just as a datapoint: I ran into an issue with that in the demo application while trying to disable instrumentation:

https://github.com/open-telemetry/opentelemetry-demo/issues/650

[legendecas]

Seems like this issue is still valuable to be tracked. Reopening.

[Flarna]

There are several instrumentations which use the existence of a span to decide if they should create a new span (option requireParentSpan).

Refs: https://github.com/open-telemetry/opentelemetry-js-contrib/pull/1343

[github-actions[bot]]

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.

[github-actions[bot]]

This issue was closed because it has been stale for 14 days with no activity.