Add Sdk.SuppressInstrumentation #960

reyang · 2020-07-31T17:28:17Z

This is a follow up PR on #948. It helps us to move closer to the resolution for #893 (comment) and the 2nd point of #809 (comment).

Changes

With this change, SDK consumers (e.g. exporters) can use:

using OpenTelemetry;

System.Console.WriteLine($"{Sdk.SuppressInstrumentation}"); // this gives you false by default
Sdk.SuppressInstrumentation = true;
System.Console.WriteLine($"{Sdk.SuppressInstrumentation}");

var previous = Sdk.SuppressInstrumentation; // do we want some syntactic sugar such like using/dispose?
Sdk.SuppressInstrumentation = true;
// export via HTTP
Sdk.SuppressInstrumentation = previous;

For API consumers, I guess we don't want to expose this as a public API, so what folks would do (e.g. in instrumentation):

using OpenTelemetry.Context;

class FoobarInstrumentation
{
    internal static readonly RuntimeContextSlot<bool> SuppressInstrumentationSlot;
    FoobarInstrumentation()
    {
        try {
            SuppressInstrumentationSlot = RuntimeContext.GetSlot("otel.suppress_instrumentation");
        } catch (Exception) {
            // instrumentation initialization failed, swallow the exception to prevent type load error.
        }
    }

    // no set as we don't expect instrumentation to change this flag
    public static bool SuppressInstrumentation
    {
        [MethodImpl(MethodImplOptions.AggressiveInlining)]
        get
        {
            return SuppressInstrumentationSlot == null ? false : SuppressInstrumentationSlot.Get();
        }
    }
}


if (FoobarInstrumentation.SuppressInstrumentation)
{
    // skip
}

Please provide a brief description of the changes here. Update the
CHANGELOG.md for non-trivial changes.

For significant contributions please make sure you have completed the following items:

Design discussion issue #
Changes in public API reviewed

reyang · 2020-07-31T17:32:21Z

src/OpenTelemetry/Sdk.cs

-        private static TimeSpan defaultPushInterval = TimeSpan.FromSeconds(60);
+        private static readonly TimeSpan DefaultPushInterval = TimeSpan.FromSeconds(60);
+
+        private static readonly RuntimeContextSlot<bool> SuppressInstrumentationRuntimeContextSlot = RuntimeContext.RegisterSlot<bool>("otel.suppress_instrumentation");


This would help the exporters/processors to quickly look up the suppress_instrumentation flag without a hash lookup.

codecov · 2020-07-31T17:35:02Z

Codecov Report

Merging #960 into master will increase coverage by 0.21%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           master     #960      +/-   ##
==========================================
+ Coverage   68.24%   68.46%   +0.21%     
==========================================
  Files         218      218              
  Lines        5955     5961       +6     
  Branches      969      969              
==========================================
+ Hits         4064     4081      +17     
+ Misses       1623     1611      -12     
- Partials      268      269       +1

Impacted Files	Coverage Δ
...emetry.Api/Context/AsyncLocalRuntimeContextSlot.cs	`50.00% <ø> (+50.00%)`	⬆️
...rc/OpenTelemetry.Api/Context/DistributedContext.cs	`76.92% <ø> (ø)`
...Telemetry.Api/Context/DistributedContextBuilder.cs	`69.23% <ø> (ø)`
...elemetry.Api/Context/RemotingRuntimeContextSlot.cs	`0.00% <ø> (ø)`
...metry.Api/Context/ThreadLocalRuntimeContextSlot.cs	`0.00% <ø> (ø)`
...lemetryProtocol/Implementation/TimestampHelpers.cs	`100.00% <ø> (ø)`
...OpenTelemetry/Metrics/BoundCounterMetricSdkBase.cs	`100.00% <ø> (ø)`
src/OpenTelemetry/Metrics/CounterMetricSdkBase.cs	`93.33% <ø> (ø)`
src/OpenTelemetry/Sdk.cs	`85.05% <50.00%> (-2.90%)`	⬇️
src/OpenTelemetry.Api/Context/RuntimeContext.cs	`44.44% <75.00%> (+44.44%)`	⬆️
... and 6 more

alanwest · 2020-07-31T17:36:46Z

src/OpenTelemetry.Api/Context/RuntimeContext.cs

+        {
+            object slot;
+            Slots.TryGetValue(name, out slot);
+            return (RuntimeContextSlot<T>)slot;


Will this cause a cast exception if the the TryGet fails?

Yes it will. I expect consumers to call this once, and cache the slot for subsequent use (similar like what I did for the Sdk).

Perhaps I should just use (RuntimeContextSlot<T>)Slots[name]?

I think that will throw a key not found. I'm just noting your comment
<returns>The slot previously registered, or null if not found.</returns>

Seems like the TryGetValue is a good way to go and just return null if that fails. Was that your intent?

I've updated the code and also the PR description to show my current approach/thinking.

👍 ok yea I get it, thanks.

Also, the using idea is interesting - maybe we try on for size what you have here, and refactor if we like it?

Yeah, I think we will have better understanding of how syntactical sugar will help exporters/processors/instrumentation when we start to add the code.

CodeBlanch · 2020-07-31T22:56:26Z

src/OpenTelemetry/Sdk.cs

+
+        private static readonly RuntimeContextSlot<bool> SuppressInstrumentationRuntimeContextSlot = RuntimeContext.RegisterSlot<bool>("otel.suppress_instrumentation");
+
+        public static bool SuppressInstrumentation


We should have XML comments here. Important spot :) Not sure exactly how to word it though?

/// <summary> /// Gets or sets a value indicating whether or not <see cref="Activity"/>Activity</see> object collection in the current context should be suppressed (dsiabled). Default value: False. /// </summary> /// <remarks> /// Set <see cref="SuppressInstrumentation"/> to <see langword="true"/> when you want to turn off automatic <see cref="Activity"/> collection. /// This is typically done to prevent infinte loops created by collection of internal operations, such as exporting spans over HTTP. /// <code> /// public override async Task<ExportResult> ExportAsync(IEnumerable<Activity> batchActivity, CancellationToken cancellationToken) /// { /// Sdk.SuppressInstrumentation = true; /// try /// { /// await this.SendBatchActivityAsync(batchActivity, cancellationToken).ConfigureAwait(false); /// return ExportResult.Success; /// } /// finally /// { /// Sdk.SuppressInstrumentation = false; /// } /// } /// </code> /// </remarks>

Should context be in the prop name? SuppressInstrumentationOnCurrentContext? Trying to hint to users what it is doing.

We should have XML comments here. Important spot :) Not sure exactly how to word it though?

Good catch, will add it.

Should context be in the prop name? SuppressInstrumentationOnCurrentContext? Trying to hint to users what it is doing.

Not sure, the proposed name seems too long 😅
Perhaps the XML comment should address this?

What if it wasn't exposed as a bool? Something more like log scope: public IDisposable SuppressInstrumentation()
Which is to say, return an object that sets the bool to true on ctor, sets it to fault on dispose?

I believe this is what @reyang was proposing in the PR description. I think I do like the disposable idea. I'm currently playing with this implementation and I thing this would make things read nicer.

Yeah, @alanwest is helping on that part in a separate PR ❤️

alanwest · 2020-08-01T01:17:12Z

@reyang In your sample code in the PR description you have

// no set as we don't expect instrumentation to change this flag

Though, it seems like this will be desirable. If some DB library uses HttpClient under the covers then it seems that it will need to be up to the DB library to enable suppression if that's the desired effect.

reyang · 2020-08-01T01:29:24Z

@reyang In your sample code in the PR description you have
// no set as we don't expect instrumentation to change this flag
Though, it seems like this will be desirable. If some DB library uses HttpClient under the covers then it seems that it will need to be up to the DB library to enable suppression if that's the desired effect.

Definitely. Another example is a library that keep polling the backend (e.g. in Microsoft there is something called SignalR), where folks only want high level logical operation rather than millions of HTTP 304.

add Sdk.SuppressInstrumentation

7a29abf

reyang requested a review from a team as a code owner July 31, 2020 17:28

reyang commented Jul 31, 2020

View reviewed changes

alanwest reviewed Jul 31, 2020

View reviewed changes

reyang added 2 commits July 31, 2020 10:42

simplify the code

a686324

fix comments

3911dc5

alanwest approved these changes Jul 31, 2020

View reviewed changes

reyang requested a review from rajkumar-rangaraj July 31, 2020 18:38

rajkumar-rangaraj approved these changes Jul 31, 2020

View reviewed changes

CodeBlanch reviewed Jul 31, 2020

View reviewed changes

add XML comment

1f83935

alanwest mentioned this pull request Aug 1, 2020

Suppress instrumentation upon calling ActivityExporter.ExportAsync #967

Closed

CodeBlanch approved these changes Aug 1, 2020

View reviewed changes

CodeBlanch and others added 3 commits July 31, 2020 18:37

Merge branch 'master' into reyang/suppress-instrumentation

af61349

Merge branch 'master' into reyang/suppress-instrumentation

ce02006

Merge branch 'master' into reyang/suppress-instrumentation

0594fdd

CodeBlanch merged commit aa86dfe into master Aug 1, 2020

CodeBlanch deleted the reyang/suppress-instrumentation branch August 1, 2020 03:42

This was referenced Aug 3, 2020

Suppress instrumentation upon calling ActivityExporter.ExportAsync #977

Closed

Make SuppressInstrumentation an IDisposable #988

Merged

alanwest mentioned this pull request Aug 20, 2020

SuppressInstrumentation from ActivityListener and DiagnosticSourceListener #1079

Merged

reyang mentioned this pull request Feb 26, 2021

Remove usage of InternalsVisibleTo in the internal instrumentations. #1856

Merged

1 task

reyang mentioned this pull request Jan 17, 2023

add SuppressTracing flag open-telemetry/opentelemetry-specification#3103

Closed

This was referenced Feb 9, 2024

[GrpcNetClient] Clarify impact to SuppressDownstreamInstrumentation #5340

Merged

SuppressDownstreamInstrumentation option in OpenTelemetry.Instrumentation.GrpcNetClient instrumentation open-telemetry/opentelemetry-dotnet-contrib#1727

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Sdk.SuppressInstrumentation #960

Add Sdk.SuppressInstrumentation #960

reyang commented Jul 31, 2020 •

edited

reyang Jul 31, 2020 •

edited

codecov bot commented Jul 31, 2020 •

edited

alanwest Jul 31, 2020

reyang Jul 31, 2020 •

edited

reyang Jul 31, 2020

alanwest Jul 31, 2020

reyang Jul 31, 2020

alanwest Jul 31, 2020

alanwest Jul 31, 2020

reyang Jul 31, 2020

CodeBlanch Jul 31, 2020

reyang Jul 31, 2020

CodeBlanch Jul 31, 2020

alanwest Jul 31, 2020

reyang Aug 1, 2020

alanwest commented Aug 1, 2020 •

edited

reyang commented Aug 1, 2020


		private static readonly RuntimeContextSlot<bool> SuppressInstrumentationRuntimeContextSlot = RuntimeContext.RegisterSlot<bool>("otel.suppress_instrumentation");

		public static bool SuppressInstrumentation

Add Sdk.SuppressInstrumentation #960

Add Sdk.SuppressInstrumentation #960

Conversation

reyang commented Jul 31, 2020 • edited

Changes

reyang Jul 31, 2020 • edited

Choose a reason for hiding this comment

codecov bot commented Jul 31, 2020 • edited

Codecov Report

Choose a reason for hiding this comment

reyang Jul 31, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alanwest commented Aug 1, 2020 • edited

reyang commented Aug 1, 2020

reyang commented Jul 31, 2020 •

edited

reyang Jul 31, 2020 •

edited

codecov bot commented Jul 31, 2020 •

edited

reyang Jul 31, 2020 •

edited

alanwest commented Aug 1, 2020 •

edited