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.

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 isRemote check for context propagation #3329

Merged
merged 13 commits into from
Jun 6, 2022
5 changes: 5 additions & 0 deletions src/OpenTelemetry/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,11 @@

## Unreleased

* `TracerProviderSDK` modified for spans with remote parent. For such spans
activity will be created irrespective of SamplingResult, to maintain context
propagation.
([#3329](https://github.com/open-telemetry/opentelemetry-dotnet/pull/3329))

## 1.3.0

Released 2022-Jun-03
Expand Down
10 changes: 5 additions & 5 deletions src/OpenTelemetry/Trace/TracerProviderSdk.cs
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,7 @@ internal sealed class TracerProviderSdk : TracerProvider
else if (sampler is AlwaysOffSampler)
{
listener.Sample = (ref ActivityCreationOptions<ActivityContext> options) =>
!Sdk.SuppressInstrumentation ? PropagateOrIgnoreData(options.Parent.TraceId) : ActivitySamplingResult.None;
!Sdk.SuppressInstrumentation ? PropagateOrIgnoreData(options.Parent.TraceId, options.Parent.IsRemote) : ActivitySamplingResult.None;
this.getRequestedDataAction = this.RunGetRequestedDataAlwaysOffSampler;
}
else
Expand Down Expand Up @@ -393,17 +393,17 @@ protected override void Dispose(bool disposing)
return activitySamplingResult;
}

return PropagateOrIgnoreData(options.Parent.TraceId);
return PropagateOrIgnoreData(options.Parent.TraceId, options.Parent.IsRemote);
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
private static ActivitySamplingResult PropagateOrIgnoreData(ActivityTraceId traceId)
private static ActivitySamplingResult PropagateOrIgnoreData(ActivityTraceId traceId, bool isParentRemote)
Copy link
Contributor

Choose a reason for hiding this comment

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

We could probably explore updating the method signature here to something like this:
private static ActivitySamplingResult PropagateOrIgnoreData(in ActivityContext parentContext) and pass in options.Parent to this method. ActivityContext is a readonly struct so using the in modifier might give some minor perf gains.

Copy link
Member Author

Choose a reason for hiding this comment

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

Minor:

Method Mean Error StdDev
WithoutInParam 716.8 ns 12.69 ns 11.87 ns
WithInparam 676.5 ns 7.27 ns 6.44 ns

I have updated the method signature

{
var isRootSpan = traceId == default;

// If it is the root span select PropagationData so the trace ID is preserved
// If it is the root span or the parent is remote select PropagationData so the trace ID is preserved
// even if no activity of the trace is recorded (sampled per OpenTelemetry parlance).
return isRootSpan
return (isRootSpan || isParentRemote)
? ActivitySamplingResult.PropagationData
: ActivitySamplingResult.None;
}
Expand Down
2 changes: 1 addition & 1 deletion test/OpenTelemetry.Tests/Trace/TracerProviderSdkTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -222,7 +222,7 @@ public void TracerProviderSdkSamplerAttributesAreAppliedToActivity(SamplingDecis
}
}

[Fact(Skip = "https://github.com/open-telemetry/opentelemetry-dotnet/issues/3315")]
[Fact]
public void TracerSdkSetsActivitySamplingResultAsPropagationWhenParentIsRemote()
{
var testSampler = new TestSampler();
Expand Down