Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Suppress Tracing context key #1653

Closed
wants to merge 11 commits into from
+18 −0
Closed

Add Suppress Tracing context key #1653

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
53ef6f2
Add Suppress Instrumentation context key
dyladan Apr 27, 2021
5b47357
Merge branch 'main' into suppress-instrumentation
dyladan Apr 27, 2021
bc99c8b
Only suppress tracing
dyladan Apr 28, 2021
5855f06
Merge branch 'suppress-instrumentation' of github.com:dynatrace-oss-c…
dyladan Apr 28, 2021
2b5e273
Review comment
dyladan Apr 28, 2021
f29ae83
Update changelog
dyladan Apr 28, 2021
babbb89
update ToC
dyladan Apr 28, 2021
552abce
remove inject suppression from suppress tracing
dyladan Apr 28, 2021
60d3e4c
Move SuppressTracing to tracing and add to compliance matrix
dyladan Apr 28, 2021
21b1fa7
Merge branch 'main' into suppress-instrumentation
dyladan Apr 29, 2021
8306351
Merge branch 'main' into suppress-instrumentation
dyladan Apr 30, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ release.

- Added `OTEL_EXPORTER_JAEGER_TIMEOUT` environment variable. ([#1612](https://github.com/open-telemetry/opentelemetry-specification/pull/1612))
- Added `OTEL_EXPORTER_ZIPKIN_TIMEOUT` environment variable. ([#1636](https://github.com/open-telemetry/opentelemetry-specification/pull/1636))
- Added Suppress Tracing context key. ([#1653](https://github.com/open-telemetry/opentelemetry-specification/pull/1653))

### Traces

Expand Down
1 change: 1 addition & 0 deletions spec-compliance-matrix.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ formats is required. Implementing more than one format is optional.
| events collection size limit | | + | + | + | + | + | - | | + | + | - | + |
| attribute collection size limit | | + | + | + | + | + | - | | + | + | - | + |
| links collection size limit | | + | + | + | + | + | - | | + | + | - | + |
| `SuppressTracing` context flag | | | | | | | | | | | | |
| [Span attributes](specification/trace/api.md#set-attributes) | | | | | | | | | | | | |
| SetAttribute | | + | + | + | + | + | + | + | + | + | + | + |
| Set order preserved | X | + | - | + | + | + | + | + | + | + | + | + |
Expand Down
14 changes: 14 additions & 0 deletions specification/trace/api.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ Table of Contents
* [TracerProvider](#tracerprovider)
* [TracerProvider operations](#tracerprovider-operations)
* [Context Interaction](#context-interaction)
* [Suppress Tracing](#suppress-tracing)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want individual flags for different signals (e.g. SuppressTracing, SuppressMetrics), or it should be one single flag (e.g. SuppressInstrumentation)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

haha the original implementation suppressed everything. It was changed to only suppress tracing based on feedback.

* [Tracer](#tracer)
* [Tracer operations](#tracer-operations)
* [SpanContext](#spancontext)
Expand Down Expand Up @@ -145,6 +146,8 @@ instance:

- Extract the `Span` from a `Context` instance
- Insert the `Span` to a `Context` instance
- Set a `SuppressTracing` flag on a `Context` instance
- Unset the `SuppressTracing` flag on a `Context` instance

The functionality listed above is necessary because API users SHOULD NOT have
access to the [Context Key](../context/context.md#create-a-key) used by the Tracing API implementation.
Expand All @@ -160,6 +163,14 @@ All the above functionalities operate solely on the context API, and they MAY be
exposed as either static methods on the trace module, or as static methods on a class
inside the trace module. This functionality SHOULD be fully implemented in the API when possible.

### Suppress Tracing

In some cases it may be useful to temporarily suppress tracing.
For example, this may be used to prevent span exports from being traced and exported,
or by an instrumentation which wraps a lower-level package which may also be
instrumented in order to prevent duplicate spans. To see how tracing is suppressed,
see [SDK Span creation](./sdk.md#sdk-span-creation).

## Tracer

The tracer is responsible for creating `Span`s.
Expand Down Expand Up @@ -344,6 +355,9 @@ The API MUST accept the following parameters:

The semantic parent of the Span MUST be determined according to the rules
described in [Determining the Parent Span from a Context](#determining-the-parent-span-from-a-context).

If the `Context` key described in [Suppress Tracing](#suppress-tracing) is set to true,
the newly created `Span` should be a non-recording `Span` with an invalid `SpanContext`.
- [`SpanKind`](#spankind), default to `SpanKind.Internal` if not specified.
- [`Attributes`](../common/common.md#attributes). Additionally,
these attributes may be used to make a sampling decision as noted in [sampling
Expand Down
2 changes: 2 additions & 0 deletions specification/trace/sdk.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,8 @@ When asked to create a Span, the SDK MUST act as if doing the following in order
A non-recording span MAY be implemented using the same mechanism as when a
`Span` is created without an SDK installed or as described in
[wrapping a SpanContext in a Span](api.md#wrapping-a-spancontext-in-a-span).
If the [`SuppressTracing`](./api.md#suppress-tracing) `Context` flag is set,
the newly created `Span` should be a non-recording `Span`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can there be a situation where it might be desired to suppress tracing but then re-enable it later in the same trace? For example a chain of 5 middlewares (all instrumented by a common instrumentation) where a user suppresses tracing in middleware 2 and then enable it again in middleware 4. If we generate non-recording spans, the spans from middleware 4 and 5 will not will not be children of the span generated by middleware 2. Not recording any spans when tracing is suppressed instead would allow this case to generate correct traces.

This could also cause issues with context propagation. If tracing is suppressed on a code path that end up making a network call and there is user/instrumentation code that inject trace context into the outgoing call, it'll end up either not injecting context or injecting non-recording span's span ID depending on the implementation of non-recording span.

May be there are other such cases.

If we were to actually suppress tracing, i.e, not record any spans at all instead of non-recording spans, both of these cases would work. Neither solution is perfect but not recording anything would probably result in traces that are "less broken".

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can there be a situation where it might be desired to suppress tracing but then re-enable it later in the same trace?

If we generate non-recording spans, the spans from middleware 4 and 5 will not will not be children of the span generated by middleware 2. Not recording any spans when tracing is suppressed instead would allow this case to generate correct traces

Sure I think that's a valid usecase. It's definitely not solved by this though for the reasons you stated. It's not easy to solve either because this puts the burden on the caller to not create spans. If an instrumentation doesn't check the context key before calling startSpan, then it can't be suppressed.

This could also cause issues with context propagation. If tracing is suppressed on a code path that end up making a network call and there is user/instrumentation code that inject trace context into the outgoing call, it'll end up either not injecting context or injecting non-recording span's span ID depending on the implementation of non-recording span.

Yes, it was my intention to suppress injection. For the usecases described, I think this is the desired behavior.

If we were to actually suppress tracing, i.e, not record any spans at all instead of non-recording spans, both of these cases would work. Neither solution is perfect but not recording anything would probably result in traces that are "less broken".

The spec states startSpan MUST return something implementing the Span interface, so I don't really see any option other than returning a non-recording span. Open to ideas if you have them.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if context API could act like a stack in this case. Suppressing instrumentation could store a reference to the active span and undoing it later would re-activate that span again. That'd solve both the cases but it probably bring up more issues and complicates implementation too much.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

context API already works like a stack since context is immutable, you need a new copy for nested scope

Copy link
Contributor

@owais owais Apr 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure I think that's a valid usecase. It's definitely not solved by this though for the reasons you stated. It's not easy to solve either because this puts the burden on the caller to not create spans. If an instrumentation doesn't check the context key before calling startSpan, then it can't be suppressed.

True but if we make it part of the spec or semantic conventions then it's very likely they that they will.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True but if we make it part of the spec or semantic conventions then it's very likely they that they will.

In my experience this is not true. Instrumentations created by SIGs will, but the community is very unlikely to read the spec that closely.


### Sampler

Expand Down