add SuppressTracing flag

[tsloughter]

Fixes #530

Changes

This PR proposes a new Context key to be used at Span creation time to disable creation of any child Spans. This feature is useful in cases like disabling tracing within an exporter when it calls the HTTP or grpc client.

The feature exists already in multiple implementations.

Prior art:

• This PR is based on the PR from @dyladan but with the difference in how
  StartSpan operates when suppressed. Add Suppress Tracing context key #1653

• Ruby: untraced fix: untraced only works with parent-based sampler opentelemetry-ruby#1412

• Javascript: suppressTracing / unsuppressTracing https://github.com/open-telemetry/opentelemetry-js/blob/main/packages/opentelemetry-core/src/trace/suppress-tracing.ts

Why return the parent

My thinking here is that Unsuppress picks up where it left off. The user does
not end up with an invalid parent or, if valid spans were used in this PR
instead, with a parent that is not recorded.

I'm torn on the usefulness of this because usually the user would be instead
returning to the previous Context (Contexts being immutable) and have no need to
either Unsuppress or be tracking the parent.

Why not suppress all signals

The plan I have is to add suppression for all signals, which can be wrapped in a
SuppressSignals or similar function. The reason is I think it would be very
common to want to suppress tracing and possibly metrics but would not want
suppression logging. In the example of not tracing an exporter you'd likely
still want any logs from the HTTP/grpc client in case something goes wrong.

What about Propagation

That is an open question not covered by this PR yet. I think I've come around to
suppression not having an effect on propagation and leave it up to libraries to
support overriding the global propagator with a no-op propagator so the user is
able to disable propagation.

[reyang]

Sharing what OpenTelemetry .NET has done:

1. A mechanism called SuppressInstrumentationScope.
2. SuppressInstrumentationScope can be used in exporters and other SDK components to prevent live-loop, e.g. the exporter calling HTTP stack which generates traces. https://github.com/open-telemetry/opentelemetry-dotnet/blob/ea5a25caac8217a71c4491b7a15ac02576b3e2cb/docs/trace/extending-the-sdk/MyExporter.cs#L32-L34
3. SuppressInstrumentationScope can also be used in a higher-level instrumentation library (e.g. gRPC) to suppress signals from a lower-level library (e.g. HTTP) https://github.com/open-telemetry/opentelemetry-dotnet/blob/ea5a25caac8217a71c4491b7a15ac02576b3e2cb/src/OpenTelemetry.Instrumentation.GrpcNetClient/Implementation/GrpcClientDiagnosticListener.cs#L90-L96

Related discussions:

• Filter activites proposals. opentelemetry-dotnet#809 (comment)
• Add Sdk.SuppressInstrumentation opentelemetry-dotnet#960

[reyang]

Can we do this in the SDK instead of the API?
In OpenTelemetry .NET the SDK has provided a mechanism which seems sufficient open-telemetry/opentelemetry-dotnet#960.

[yurishkuro]

@reyang the functionality may need to be available in instrumentation libraries, which are not supposed to be aware of the SDK

[trask]

this is the mechanism that all of the Java Instrumentation uses: open-telemetry/oteps#172

[Flarna]

In JS some (but by far not all) instrumentations check the suppress context key and call startSpan only if it is not set.
SDK checks in startSpan and returns a Noop(InvalidTraceId/SpanId) in case it's set

In python it seems that the concrete instrumentations check if suppress context key is set and if if is set they do not even call startSpan().
SDK seems to have no special handling of the suppressed context key.

I think we should add handling in SDK to avoid the needed that everyone has to check the context before calling startSpan.

We might also want to distinguish between suppressing nested/inner spans (e.g. HTTPS calls public, instrumented HTTP APIs internal) but keep child spans like DNS and propagation to other processes or suppress the complete subtrace which would include the need to propagate the suppress flag to avoid that receiver starts a new trace.

[reyang]

@reyang the functionality may need to be available in instrumentation libraries, which are not supposed to be aware of the SDK

Yep. I think there are two parts:

1. From the PR description "This feature is useful in cases like disabling tracing within an exporter when it calls the HTTP or grpc client." - I feel this part can be done purely in the SDK, without having to touch the API.
2. For instrumentation libraries - agree that it should not take dependency on the SDK. Based on the practice in OpenTelemetry .NET, it seems a combination of context API and some known "named slots" would also work. In addition, such approach seems to be addressing the problem for instrumentation authors while avoiding exposing an API and encouraging application developers from calling it.

[yurishkuro]

@reyang a publicly accessible (via API) Context key to suppress tracing is a possible alternative, but the "obscurity" argument doesn't sit well with me (and it's also less type-safe than the dedicated methods). If someone has a valid need to suppress tracing this way, it seems strange to make the feature harder to find (i.e. it won't come up in auto-complete). It also bifurcates the way API users are asked to interact with the API, without actually reducing the mental model of the API surface.

[lmolkova]

Based on the research I've done in #1767, each language SIG has (or had at the time I did it) has some version of a suppression flag

(links are 1.5 years old)

• python
• .net
• js
• java suppresses nested client spans by default - changed to Instrumentation layers and suppressing duplicates oteps#172
• seems ruby does it

Suppression flag works great to suppress the instrumentation of exporter calls, but for application/library instrumentation purposes, it's not expressive enough.
E.g. ORM call instrumentation should only suppress DB spans, but not the protocol level (e.g. HTTP spans) underneath. At least there should be a way to achieve it. Hence the open-telemetry/oteps#172

[github-actions]

This PR was marked stale due to lack of activity. It will be closed in 7 days.

[github-actions]

Closed as inactive. Feel free to reopen if this PR is still being worked on.

[lathspell]

Any chance that this gets re-opened as the problem is unsolved yet?

[dmathieu]

As mentioned before, this is something that several SDKs are already doing. How about reopening the issue?

This would be interesting for Go as well, as we currently don't want to let folks override the http.Transport for OTLP HTTP exporters, to avoid having that exporter generate traces unexpectedly.
Being able to suppress tracing in the context would allow us to suppress tracing within the exporter, and to make the use of a custom exporter safe.

[yurishkuro]

There was a reasonable objection to this approach raised in #3103 (comment) by @lmolkova .