Suppress instrumentation upon calling ActivityExporter.ExportAsync #977

alanwest · 2020-08-03T03:50:50Z

Reopening this PR for visibility now that #960 is merged. I had originally opened this as #967

Keeping it in draft state for now because tomorrow I'll try to get a sense of performance overhead. If things look ok, I'll get some tests added, too.

CodeBlanch · 2020-08-03T16:46:55Z

src/OpenTelemetry/Trace/ActivityProcessor.cs

@@ -28,13 +28,29 @@ public abstract class ActivityProcessor
        /// Activity start hook.
        /// </summary>
        /// <param name="activity">Instance of activity to process.</param>
-        public abstract void OnStart(Activity activity);
+        public void OnStart(Activity activity)


Are you open to renaming these? I think the more typical .NET-y style for this would be:

public void Start(Activity activity); protected abstract void OnStart(Activity activity);

If others are open, I'm open to this. It does mean a changing the public API and would affect anyone out there that has created a custom processor.

Both ways are breaking, no?

Today:

public abstract void OnStart(Activity activity);

First version:

public void OnStart(Activity activity); protected abstract void OnStartInternal(Activity activity);

Proposed version:

public void Start(Activity activity); protected abstract void OnStart(Activity activity);

Duh. 🤦 Yes, you're right.

We could move this logic up into ActivityListener to avoid the break. Like what's I'm doing on #969. But I think this is a better long-term design. We don't want a lot of stuff in ActivityListener IMO. I was thinking once this is available, I would re-do how that is done to be in the Start or Stop implementation of base ActivityProcessor.

Normally I would struggle with naming things. This time it is easier for me as the spec said we should use OnStart and OnEnd 😄.

Ah ha! Yes I wondered about that right after submitting this question. Great! We won't touch it.

@alanwest Separate PR works for me 👍

@CodeBlanch no PR necessary 😄. As Reiley pointed out OnStart and OnEnd are the names specified in the spec for the public API.

Got it. Sorry I didn't realize you changed them to Start/OnStart already. That works for me. I think if there wasn't a spec involved something like StartActivity/OnActivityStarted would be better (what you were getting at), but good to stay as close to the spec as possible.

CodeBlanch · 2020-08-03T16:50:00Z

src/OpenTelemetry/Trace/ActivityProcessor.cs

-        public abstract void OnStart(Activity activity);
+        public void OnStart(Activity activity)
+        {
+            if (Sdk.SuppressInstrumentation)


Just wondering out loud here... should the SuppressInstrumentation check go in ActivityProcessor or should it be part of the ActivitySampler? If SuppressInstrumentation == true forced a lower sampling decision (None or PropagationOnly) that would be better for perf I think.

reyang · 2020-08-03T17:20:41Z

src/OpenTelemetry/Trace/BatchingActivityProcessor.cs

                var result = await this.exporter.ExportAsync(this.batch, cancellationToken).ConfigureAwait(false);
+                Sdk.SuppressInstrumentation = previous;


Please consider a try-finally block (or adding the using syntactical sugar) to avoid accidental context leakage.

Yep, good call. I do like the using approach and had planned on going that route.

…umentation

…umentation # Conflicts: # examples/Console/TestConsoleExporter.cs # src/OpenTelemetry/Trace/ActivityProcessor.cs # src/OpenTelemetry/Trace/BatchingActivityProcessor.cs # src/OpenTelemetry/Trace/Internal/NoopActivityProcessor.cs # src/OpenTelemetry/Trace/SimpleActivityProcessor.cs # test/OpenTelemetry.Tests/Implementation/Trace/Config/ActivityProcessorPipelineTests.cs

CodeBlanch · 2020-08-11T02:19:02Z

src/OpenTelemetry/Trace/ActivityProcessor.cs

@@ -114,8 +114,12 @@ protected virtual void Dispose(bool disposing)
            this.disposed = true;
        }

-        protected abstract void OnStartInternal(Activity activity);
+        protected virtual void OnStartInternal(Activity activity)


@alanwest The "Internal" in the name is kind of non-standard. I think we were talking about this before but I lost track. Could we not do?

public void Start(Activity activity) protected virtual void OnStart(Activity activity)

I haven't looked through the changes yet, imagine we have a chain of processors, checking SuppressInstrumentation on every single processor seems to be a big perf hit.

@alanwest The "Internal" in the name is kind of non-standard. I think we were talking about this before but I lost track. Could we not do?

Agreed. I actually have this change staged up, just haven't pushed yet. Mostly just picked things up on the PR today and resolved the various conflicts.

I haven't looked through the changes yet, imagine we have a chain of processors, checking SuppressInstrumentation on every single processor seems to be a big perf hit.

Yes, I haven't spent enough time analyzing this yet. Earlier there was an idea from @CodeBlanch to see if this fits better in the sampler. Will consider this soon.

Given SuppressInstrumentation is used for folks to express whether they want the telemetry to be suppressed or not, how about the following approach:

If the code (app/lib) is using new ActivitySource.StartActivity, it will be responsible to checking the SuppressInstrumentation flag and not creating the activity if the flag is set.

In this way the sampler/processor/exporter won't receive the activity at all since it is not even created.

If the code is not performing the check, the code is faulty and we are noting doing another round of check in the processor - due to perf reason.

If the code is using legacy Activity, the adapter will be responsible to drop the activity before sending it to the sampler/processor.

In this way we gain perf since the adapter is only taking legacy activities.

If the code (app/lib) is using new ActivitySource.StartActivity, it will be responsible to checking the SuppressInstrumentation flag and not creating the activity if the flag is set.

This would require taking an OTel dependency. We want to avoid this need for library authors, no?

If the code is using legacy Activity, the adapter will be responsible to drop the activity before sending it to the sampler/processor.

Yes, this makes sense for the legacy route.

I would worry a bit relying on others to do it perfectly. I'm envisioning a bunch of difficult-to-triage issues being opened when applications go into infinite loops. We could move the check into the ActivityListener? If SuppressInstrumentation is triggered, we return ActivityDataRequest.None from GetRequestedDataUsingContext (or whatever it ends up being called). If we do that, Activity is never created.

We could move the check into the ActivityListener?

If perf looks good, putting the logic in the ActivityListener will work.

This would require taking an OTel dependency. We want to avoid this need for library authors, no?

Definitely yes. I think most library authors don't need to care about this (infinite loops). Only library authors who build libraries that can be used for exporting task will have to worry about it. Probably on for HTTP / TCP / UDP / Unix domain socket.

More data points:

In OpenTelemetry Python it seems to be fine to have only HTTP instrumentation doing the loop detection using SuppressInstrumentation, check this.

If only 5% of the scenario would use it, making it too generic and ask everyone to pay the perf tax could be a hard-sell (if the perf tax is low, for example 0.1%, I guess it'll be fine).

I'll try on this on for size tomorrow:

Check in ActivityListener for the new ActivitySource way

And check in the adapter for the legacy

Then try to get some perf numbers. If we're not happy, then I'm definitely ok entertaining scoping this only to certain instrumentation HttpClient/HttpWebRequest.

@alanwest FYI - I've shared a mini benchmark project which I used for refactor the provider/processor/pipeline #1039.

…umentation

alanwest · 2020-08-12T15:00:55Z

src/OpenTelemetry/Trace/BatchingActivityProcessor.cs

@@ -320,14 +320,18 @@ private async Task<int> ExportBatchAsync(CancellationToken cancellationToken)
                    this.batch.Add(nextActivity);
                }

-                var result = await this.exporter.ExportAsync(this.batch, cancellationToken).ConfigureAwait(false);
-                if (result != ExportResult.Success)
+                using (Sdk.SuppressInstrumentation)


@CodeBlanch @reyang

Uh oh this is a terrible bug waiting to happen. This should be:

using (Sdk.SuppressInstrumentation.Begin())

This bug would result in the static instance getting disposed. I think we need to rethink things a bit to prevent this from being possible.

alanwest · 2020-08-14T02:56:15Z

Closing and reopening this work in #1079

Suppress instrumentation upon calling ActivityExporter.ExportAsync

a76e069

alanwest requested a review from a team August 3, 2020 03:50

alanwest closed this Aug 3, 2020

alanwest reopened this Aug 3, 2020

alanwest marked this pull request as draft August 3, 2020 03:52

CodeBlanch reviewed Aug 3, 2020

View reviewed changes

reyang reviewed Aug 3, 2020

View reviewed changes

alanwest added 3 commits August 3, 2020 13:06

Merge branch 'master' into alanwest/activity-processor-suppress-instr…

f5dae7b

…umentation

Make ActivityProcessor.OnStart/OnEnd virtual instead of abstract

6c289d4

CodeBlanch reviewed Aug 11, 2020

View reviewed changes

alanwest added 4 commits August 10, 2020 19:38

Rename Start/End methods on ActivityProcessor

b7921b3

Merge branch 'master' into alanwest/activity-processor-suppress-instr…

824c14b

…umentation

Merge branch 'master' into alanwest/activity-processor-suppress-instr…

d31bd24

…umentation

Merge branch 'master' into alanwest/activity-processor-suppress-instr…

f7acf67

…umentation

alanwest commented Aug 12, 2020

View reviewed changes

alanwest mentioned this pull request Aug 14, 2020

SuppressInstrumentation from ActivityListener and DiagnosticSourceListener #1079

Merged

alanwest closed this Aug 14, 2020

This was referenced Aug 19, 2020

Check SuppressInstrumentation from HTTP instrumentation #1098

Closed

Clean up exporter and processor #1140

Merged

alanwest deleted the alanwest/activity-processor-suppress-instrumentation branch August 24, 2020 17:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Suppress instrumentation upon calling ActivityExporter.ExportAsync #977

Suppress instrumentation upon calling ActivityExporter.ExportAsync #977

alanwest commented Aug 3, 2020

CodeBlanch Aug 3, 2020

alanwest Aug 3, 2020

CodeBlanch Aug 3, 2020

alanwest Aug 3, 2020

CodeBlanch Aug 3, 2020

reyang Aug 12, 2020

alanwest Aug 12, 2020 •

edited

Loading

CodeBlanch Aug 12, 2020

alanwest Aug 12, 2020

CodeBlanch Aug 12, 2020

CodeBlanch Aug 3, 2020

reyang Aug 3, 2020

alanwest Aug 3, 2020

CodeBlanch Aug 11, 2020

reyang Aug 11, 2020

alanwest Aug 11, 2020 •

edited

Loading

reyang Aug 11, 2020 •

edited

Loading

alanwest Aug 11, 2020 •

edited

Loading

CodeBlanch Aug 11, 2020

reyang Aug 11, 2020

reyang Aug 11, 2020

alanwest Aug 11, 2020

reyang Aug 11, 2020

alanwest Aug 12, 2020 •

edited

Loading

reyang Aug 12, 2020

alanwest commented Aug 14, 2020

		var result = await this.exporter.ExportAsync(this.batch, cancellationToken).ConfigureAwait(false);
		Sdk.SuppressInstrumentation = previous;

Suppress instrumentation upon calling ActivityExporter.ExportAsync #977

Suppress instrumentation upon calling ActivityExporter.ExportAsync #977

Conversation

alanwest commented Aug 3, 2020

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 Aug 12, 2020 • edited Loading

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 Aug 11, 2020 • edited Loading

Choose a reason for hiding this comment

reyang Aug 11, 2020 • edited Loading

Choose a reason for hiding this comment

alanwest Aug 11, 2020 • edited Loading

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 Aug 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alanwest commented Aug 14, 2020

alanwest Aug 12, 2020 •

edited

Loading

alanwest Aug 11, 2020 •

edited

Loading

reyang Aug 11, 2020 •

edited

Loading

alanwest Aug 11, 2020 •

edited

Loading

alanwest Aug 12, 2020 •

edited

Loading