SuppressInstrumentation from ActivityListener and DiagnosticSourceListener

[alanwest]

This PR applies Sdk.SuppressInstrumentation originally introduced in #960.

Changes

• Check Sdk.SuppressInstrumentation in ActivityListener covering the new ActivitySource sourced activities.
• Check Sdk.SuppressInstrumentation in DiagnosticSourceListener covering the legacy activities.

Some performance analysis was done on this implementation in #1049

Alternative to this PR

Given that this PR suppresses instrumentation in a general way, any perf hit from checking Sdk.SuppressInstrumentation is incurred for every Activity. Alternative would be to narrow what we suppress as suggested here #977 (comment). I implemented this alternative in #1098.

Based on discussion here: #1098 (comment), I think we're going to go with this PR and close #1098.

Follow up after this PR

• Set SuppressInstrumentation = true in Zipkin/Jaeger exporters.
• Remove hard-coded values in IsInternalUrl from HTTP instrumentation

[codecov]

Codecov Report

Merging #1079 into master will increase coverage by 0.12%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1079      +/-   ##
==========================================
+ Coverage   74.63%   74.76%   +0.12%     
==========================================
  Files         223      223              
  Lines        6360     6365       +5     
==========================================
+ Hits         4747     4759      +12     
+ Misses       1613     1606       -7     

Impacted Files	Coverage Δ
...emetry/Instrumentation/DiagnosticSourceListener.cs	90.90% <100.00%> (+0.90%)	⬆️
src/OpenTelemetry/Trace/TracerProviderSdk.cs	94.64% <100.00%> (+0.14%)	⬆️
src/OpenTelemetry.Api/Context/RuntimeContext.cs	80.00% <0.00%> (+4.00%)	⬆️
...elemetry.Api/Context/RemotingRuntimeContextSlot.cs	54.54% <0.00%> (+54.54%)	⬆️

[CodeBlanch]

Probably a good optimization to have this, but I don't think it's strictly required. For "legacy" Activity flows we pass them through an ActivitySource for sampling. So the logic below should also catch these?

[alanwest]

@CodeBlanch Hmm 🤔, maybe I need you to point me to some code... because without this check here, this test I wrote would fail:

https://github.com/open-telemetry/opentelemetry-dotnet/pull/1079/files#diff-a266e12581b9c9ae7bd148bbf91dabb8R37-R59

So, it would seem that for legacy activities, we'll need a check somewhere in addition to the check I have in the ActivityListener for the new activities.

[CodeBlanch]

You're right, my bad. That was a test and you passed, great job!

It looks like ActivitySourceAdapter has its own sampler, so we'll need to do similar in here:

opentelemetry-dotnet/src/OpenTelemetry/Trace/ActivitySourceAdapter.cs

Lines 85 to 131 in 44b55d4

	private void RunGetRequestedData(Activity activity)
	{
	ActivityContext parentContext;
	if (string.IsNullOrEmpty(activity.ParentId))
	{
	parentContext = default;
	}
	else if (activity.Parent != null)
	{
	parentContext = activity.Parent.Context;
	}
	else
	{
	parentContext = new ActivityContext(
	activity.TraceId,
	activity.ParentSpanId,
	activity.ActivityTraceFlags,
	activity.TraceStateString,
	isRemote: true);
	}
	var samplingParameters = new SamplingParameters(
	parentContext,
	activity.TraceId,
	activity.DisplayName,
	activity.Kind,
	activity.TagObjects,
	activity.Links);
	var samplingResult = this.sampler.ShouldSample(samplingParameters);
	switch (samplingResult.Decision)
	{
	case SamplingDecision.NotRecord:
	activity.IsAllDataRequested = false;
	break;
	case SamplingDecision.Record:
	activity.IsAllDataRequested = true;
	break;
	case SamplingDecision.RecordAndSampled:
	activity.IsAllDataRequested = true;
	activity.ActivityTraceFlags |= ActivityTraceFlags.Recorded;
	break;
	}
	}
	}
	}

Or maybe we can refactor so more is shared?

[alanwest]

That was a test and you passed, great job!

Ha! Phew!

It looks like ActivitySourceAdapter has its own sampler, so we'll need to do similar in here

I don't think so because this is actually downstream of the check I have in place.

So, DiagnosticSourceListener.OnNext --- invokes ---> ListenerHandler.On[Start|End|Exception|Custom]

It is our ListenerHandlers that hold an instance of the ActivitySourceAdapter which is then the thing that simulates Start/Stop/etc and deals with sampling like the standard ActivitySource/ActivityListener.

[CodeBlanch]

Are you saying it wouldn't work, or just that this spot is sooner? If the later, I suppose that is compelling enough a reason to keep it here and I'm good with it 👍

[alanwest]

It would work for preventing this.activityProcessor.OnStart(activity);, but the reason I put the check where I did in DiagnosticSourceListener.OnNext is that the check there prevents all instrumentation callbacks defined on the ListenerHandler from being invoked. For example, if some library had:

myDiagnosticListener.Write("MyCustomEvent", payload);

If we had a ListenerHander wired up to this library then this Write would result in invoking the OnCustom callback. I figured it would be good to suppress all aspects of the library's instrumentation.

Though I guess in most (if not all) of these callbacks we check activity.IsAllDataRequested, so they'd result in noops anyways...

[CodeBlanch]

Makes sense good call!

[CodeBlanch]

LGTM