Create an activity even when SamplingDecision is Drop

[utpilla]

Fixes #1709.

Changes

• Return ActivitySamplingResult.Propagation when SamplingDecision is Drop and the activity is not root
• This is to comply with the spec requirement of always creating a span regardless of the Sampling Decision

[codecov]

Codecov Report

Merging #1715 (0f5491e) into master (02adc4c) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1715   +/-   ##
=======================================
  Coverage   82.18%   82.18%           
=======================================
  Files         250      250           
  Lines        6763     6758    -5     
=======================================
- Hits         5558     5554    -4     
+ Misses       1205     1204    -1     

Impacted Files	Coverage Δ
src/OpenTelemetry/Trace/TracerProviderSdk.cs	89.76% <100.00%> (-0.39%)	⬇️
...Telemetry/Internal/SelfDiagnosticsEventListener.cs	94.28% <0.00%> (+1.42%)	⬆️

[cijothomas]

While this gets us closer to the spec, it creates an additional issue of broken traces.
If A->B->C, and A,C sampling decision is Record&Sampler, but B is Drop. If we create new Activity for B, and C considers B as its parent, then we'll not be able to stitch together A->C in backends, as C's parent is B, which is not stored anywhere.

This can be alleviated, but it requires us to device a strategy. Most likely this requires help from .net runtime as well.
https://github.com/open-telemetry/opentelemetry-specification/pull/998/files#r517616726

[CodeBlanch]

Something seems off about this. Looks like the current behavior is we create the root but not children. That's great for perf. Could the problem be in the instrumentation?

• Broken example - .NET Core HttpClient instrumentation only propagates if a new activity is created (OnStartActivity):
  

  opentelemetry-dotnet/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs

  Line 114 in 02adc4c

  textMapPropagator.Inject(new PropagationContext(activity.Context, Baggage.Current), request, HttpRequestMessageContextPropagation.HeaderValueSetter);

• Working example - .NET Framework HttpWebRequest will propagate always:
  

  opentelemetry-dotnet/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs

  Line 228 in 02adc4c

  InstrumentRequest(request, activityContext);

[cijothomas]

Something seems off about this. Looks like the current behavior is we create the root but not children. That's great for perf. Could the problem be in the instrumentation?

• Broken example - .NET Core HttpClient instrumentation only propagates if a new activity is created (OnStartActivity):
  

  opentelemetry-dotnet/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs

  Line 114 in 02adc4c

  textMapPropagator.Inject(new PropagationContext(activity.Context, Baggage.Current), request, HttpRequestMessageContextPropagation.HeaderValueSetter);

• Working example - .NET Framework HttpWebRequest will propagate always:
  

  opentelemetry-dotnet/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs

  Line 228 in 02adc4c

  InstrumentRequest(request, activityContext);

The current issue is - we are not in compliance with the spec, as spec says "a new Span with new SpanId must be created irrespective of Sampling outcome.". To make us compliant with that part of the spec, we need to create an Activity always with PropagationData only. This ensure new SpanId is generated. But this causes broken trace. Some of these were discussed in https://github.com/open-telemetry/opentelemetry-specification/pull/998/files#r517616726. But I am yet to find out if any language has this implemented correctly. If .NET needs to fix that issue, we'll likely need changes from .NET runtime itself.

[cijothomas]

The issue with instrumentations not propagating - is tracked here : #1561 Its closely related issue.

[utpilla]

Closing this PR as we need to decide on the best approach to ensure that there are no broken traces due to differential sampling of spans within a single trace.