From 4ef16a995dc731fec60c10ab0ec984c12d6f457f Mon Sep 17 00:00:00 2001 From: Cijo Thomas Date: Tue, 4 Aug 2020 23:42:06 -0700 Subject: [PATCH 1/7] Leverage ActivityListener.AutoGenerateRootContextTraceId --- src/OpenTelemetry/Sdk.cs | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/src/OpenTelemetry/Sdk.cs b/src/OpenTelemetry/Sdk.cs index 3f68ff21b6c..ebae001da73 100644 --- a/src/OpenTelemetry/Sdk.cs +++ b/src/OpenTelemetry/Sdk.cs @@ -144,9 +144,12 @@ public static TracerProvider CreateTracerProvider(Action ActivityStopped = activityProcessor.OnEnd, // Function which takes ActivitySource and returns true/false to indicate if it should be subscribed to - // or not + // or not. ShouldListenTo = (activitySource) => tracerProviderBuilder.ActivitySourceNames?.Contains(activitySource.Name.ToUpperInvariant()) ?? false, + // Setting this to true means TraceId will be generated for RootActivities. + AutoGenerateRootContextTraceId = true, + // The following parameter is not used now. GetRequestedDataUsingParentId = (ref ActivityCreationOptions options) => ActivityDataRequest.AllData, @@ -163,14 +166,12 @@ internal static ActivityDataRequest ComputeActivityDataRequest( in ActivityCreationOptions options, Sampler sampler) { - var isRootSpan = options.Parent.TraceId == default; - - // This is not going to be the final traceId of the Activity (if one is created), however, it is - // needed in order for the sampling to work. This differs from other OTel SDKs in which it is - // the Sampler always receives the actual traceId of a root span/activity. - ActivityTraceId traceId = !isRootSpan - ? options.Parent.TraceId - : ActivityTraceId.CreateRandom(); + var isRootSpan = options.Parent.SpanId == default; + + // As we set ActivityListener.AutoGenerateRootContextTraceId = true, + // Parent.TraceId will always be the TraceId of the to-be-created Activity, + // if it get created. + ActivityTraceId traceId = options.Parent.TraceId; var samplingParameters = new SamplingParameters( options.Parent, From a78dbf0b1d74b2e48b7dd71c2285bf778c677a38 Mon Sep 17 00:00:00 2001 From: Cijo Thomas Date: Tue, 4 Aug 2020 23:44:18 -0700 Subject: [PATCH 2/7] test --- src/OpenTelemetry/Sdk.cs | 4 +- .../Implementation/Trace/TraceSdkTest.cs | 132 ++++++++++++++++++ 2 files changed, 135 insertions(+), 1 deletion(-) create mode 100644 test/OpenTelemetry.Tests/Implementation/Trace/TraceSdkTest.cs diff --git a/src/OpenTelemetry/Sdk.cs b/src/OpenTelemetry/Sdk.cs index ebae001da73..5bfecc1fe69 100644 --- a/src/OpenTelemetry/Sdk.cs +++ b/src/OpenTelemetry/Sdk.cs @@ -147,7 +147,9 @@ public static TracerProvider CreateTracerProvider(Action // or not. ShouldListenTo = (activitySource) => tracerProviderBuilder.ActivitySourceNames?.Contains(activitySource.Name.ToUpperInvariant()) ?? false, - // Setting this to true means TraceId will be generated for RootActivities. + // Setting this to true means TraceId will be always + // available in sampling callbacks and will be the actual + // traceid used, if activity ends up getting created. AutoGenerateRootContextTraceId = true, // The following parameter is not used now. diff --git a/test/OpenTelemetry.Tests/Implementation/Trace/TraceSdkTest.cs b/test/OpenTelemetry.Tests/Implementation/Trace/TraceSdkTest.cs new file mode 100644 index 00000000000..a12a172cf8b --- /dev/null +++ b/test/OpenTelemetry.Tests/Implementation/Trace/TraceSdkTest.cs @@ -0,0 +1,132 @@ +// +// Copyright The OpenTelemetry Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// + +using System.Diagnostics; +using OpenTelemetry; +using OpenTelemetry.Trace; +using Xunit; + +namespace OpenTelemetry.Tests.Implementation.Trace +{ + public class TraceSdkTest + { + private const string ActivitySourceName = "TraceSdkTest"; + + [Fact] + public void TracerSdkInvokesSamplingWithCorrectParameters() + { + var testSampler = new TestSampler(); + using var activitySource = new ActivitySource(ActivitySourceName); + using var sdk = Sdk.CreateTracerProvider( + (tpbuilder) => + { + tpbuilder.AddActivitySource(ActivitySourceName); + tpbuilder.SetSampler(testSampler); + }); + + // OpenTelemetry Sdk is expected to set default to W3C. + Assert.True(Activity.DefaultIdFormat == ActivityIdFormat.W3C); + + using (var rootActivity = activitySource.StartActivity("root")) + { + Assert.NotNull(rootActivity); + + // TODO: Follow up with .NET on why ParentSpanId is != default here. + // Assert.True(rootActivity.ParentSpanId == default); + + // Validate that the TraceId seen by Sampler is same as the + // Activity when it got created. + Assert.Equal(rootActivity.TraceId, testSampler.LatestSamplingParameters.TraceId); + } + + using (var parent = activitySource.StartActivity("parent", ActivityKind.Client)) + { + Assert.Equal(parent.TraceId, testSampler.LatestSamplingParameters.TraceId); + using (var child = activitySource.StartActivity("child")) + { + Assert.Equal(child.TraceId, testSampler.LatestSamplingParameters.TraceId); + Assert.Equal(parent.TraceId, child.TraceId); + Assert.Equal(parent.SpanId, child.ParentSpanId); + } + } + + var customContext = new ActivityContext( + ActivityTraceId.CreateRandom(), + ActivitySpanId.CreateRandom(), + ActivityTraceFlags.None); + + using (var fromCustomContext = + activitySource.StartActivity("customContext", ActivityKind.Client, customContext)) + { + Assert.Equal(fromCustomContext.TraceId, testSampler.LatestSamplingParameters.TraceId); + Assert.Equal(customContext.TraceId, fromCustomContext.TraceId); + Assert.Equal(customContext.SpanId, fromCustomContext.ParentSpanId); + Assert.NotEqual(customContext.SpanId, fromCustomContext.SpanId); + } + } + + [Fact] + public void TracerSdkSetsActivityDataRequestBasedOnSamplingDecision() + { + var testSampler = new TestSampler(); + using var activitySource = new ActivitySource(ActivitySourceName); + using var sdk = Sdk.CreateTracerProvider( + (tpbuilder) => + { + tpbuilder.AddActivitySource(ActivitySourceName); + tpbuilder.SetSampler(testSampler); + }); + + testSampler.DesiredSamplingResult = new SamplingResult(true); + using (var activity = activitySource.StartActivity("root")) + { + Assert.NotNull(activity); + Assert.True(activity.IsAllDataRequested); + Assert.True(activity.Recorded); + } + + testSampler.DesiredSamplingResult = new SamplingResult(false); + using (var activity = activitySource.StartActivity("root")) + { + // Even if sampling returns false, for root activities, + // activity is still created with PropagationOnly. + Assert.NotNull(activity); + Assert.False(activity.IsAllDataRequested); + Assert.False(activity.Recorded); + + using (var innerActivity = activitySource.StartActivity("inner")) + { + // This is not a root activity. + // If sampling returns false, no activity is created at all. + Assert.Null(innerActivity); + } + } + } + + private class TestSampler : Sampler + { + public SamplingResult DesiredSamplingResult { get; set; } = new SamplingResult(true); + + public SamplingParameters LatestSamplingParameters { get; private set; } + + public override SamplingResult ShouldSample(in SamplingParameters samplingParameters) + { + this.LatestSamplingParameters = samplingParameters; + return this.DesiredSamplingResult; + } + } + } +} From 65ffaed4848632a5324b5542caf410e875345fd9 Mon Sep 17 00:00:00 2001 From: Cijo Thomas Date: Tue, 4 Aug 2020 23:56:03 -0700 Subject: [PATCH 3/7] More test --- src/OpenTelemetry/Sdk.cs | 3 --- .../Implementation/Trace/TraceSdkTest.cs | 25 +++++++++++++++++++ 2 files changed, 25 insertions(+), 3 deletions(-) diff --git a/src/OpenTelemetry/Sdk.cs b/src/OpenTelemetry/Sdk.cs index 5bfecc1fe69..2ac5d4f1414 100644 --- a/src/OpenTelemetry/Sdk.cs +++ b/src/OpenTelemetry/Sdk.cs @@ -152,9 +152,6 @@ public static TracerProvider CreateTracerProvider(Action // traceid used, if activity ends up getting created. AutoGenerateRootContextTraceId = true, - // The following parameter is not used now. - GetRequestedDataUsingParentId = (ref ActivityCreationOptions options) => ActivityDataRequest.AllData, - // This delegate informs ActivitySource about sampling decision when the parent context is an ActivityContext. GetRequestedDataUsingContext = (ref ActivityCreationOptions options) => ComputeActivityDataRequest(options, sampler), }; diff --git a/test/OpenTelemetry.Tests/Implementation/Trace/TraceSdkTest.cs b/test/OpenTelemetry.Tests/Implementation/Trace/TraceSdkTest.cs index a12a172cf8b..3170717d8a9 100644 --- a/test/OpenTelemetry.Tests/Implementation/Trace/TraceSdkTest.cs +++ b/test/OpenTelemetry.Tests/Implementation/Trace/TraceSdkTest.cs @@ -76,6 +76,31 @@ public void TracerSdkInvokesSamplingWithCorrectParameters() Assert.Equal(customContext.SpanId, fromCustomContext.ParentSpanId); Assert.NotEqual(customContext.SpanId, fromCustomContext.SpanId); } + + // Validate that when StartActivity is called using Parent as string, + // Sampling is called correctly. + var act = new Activity("anything").Start(); + act.Stop(); + var customContextAsString = act.Id; + var expectedTraceId = act.TraceId; + var expectedParentSpanId = act.SpanId; + + using (var fromCustomContextAsString = + activitySource.StartActivity("customContext", ActivityKind.Client, customContextAsString)) + { + Assert.Equal(fromCustomContextAsString.TraceId, testSampler.LatestSamplingParameters.TraceId); + Assert.Equal(expectedTraceId, fromCustomContextAsString.TraceId); + Assert.Equal(expectedParentSpanId, fromCustomContextAsString.ParentSpanId); + } + + using (var fromInvalidW3CIdParent = + activitySource.StartActivity("customContext", ActivityKind.Client, "InvalidW3CIdParent")) + { + // OpenTelemetry ActivityContext does not support + // non W3C Ids. Starting activity with non W3C Ids + // will result in no activity being created. + Assert.Null(fromInvalidW3CIdParent); + } } [Fact] From bd2b67cf68de4cf2444c2f0e264e6dbfc362fa13 Mon Sep 17 00:00:00 2001 From: Cijo Thomas Date: Tue, 4 Aug 2020 23:58:45 -0700 Subject: [PATCH 4/7] changelog --- src/OpenTelemetry/CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/OpenTelemetry/CHANGELOG.md b/src/OpenTelemetry/CHANGELOG.md index 0bb78d5730f..fb6361264e6 100644 --- a/src/OpenTelemetry/CHANGELOG.md +++ b/src/OpenTelemetry/CHANGELOG.md @@ -10,7 +10,7 @@ * `BatchingActivityProcessor`/`SimpleActivityProcessor` is disposable and it disposes the containing exporter. * `BroadcastActivityProcessor`is disposable and it disposes the processors. - +* Samplers now get the actual TraceId of the Activity to be created. ## 0.3.0-beta Released 2020-07-23 From b486437508f6ccbae24acad06d970052ce03cf3c Mon Sep 17 00:00:00 2001 From: Cijo Thomas Date: Wed, 5 Aug 2020 00:04:25 -0700 Subject: [PATCH 5/7] remove duplicated test logic --- .../Trace/ActivityListenerSdkTest.cs | 110 ------------------ 1 file changed, 110 deletions(-) delete mode 100644 test/OpenTelemetry.Tests/Implementation/Trace/ActivityListenerSdkTest.cs diff --git a/test/OpenTelemetry.Tests/Implementation/Trace/ActivityListenerSdkTest.cs b/test/OpenTelemetry.Tests/Implementation/Trace/ActivityListenerSdkTest.cs deleted file mode 100644 index 10fd51538da..00000000000 --- a/test/OpenTelemetry.Tests/Implementation/Trace/ActivityListenerSdkTest.cs +++ /dev/null @@ -1,110 +0,0 @@ -// -// Copyright The OpenTelemetry Authors -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. -// - -using System.Diagnostics; -using OpenTelemetry.Trace; -using Xunit; - -namespace OpenTelemetry.Tests.Implementation.Trace -{ - public class ActivityListenerSdkTest - { - static ActivityListenerSdkTest() - { - Activity.DefaultIdFormat = ActivityIdFormat.W3C; - Activity.ForceDefaultIdFormat = true; - } - - [Fact] - public void BuildSamplingParametersHandlesCurrentActivity() - { - using var activitySource = new ActivitySource(nameof(this.BuildSamplingParametersHandlesCurrentActivity)); - - var testSampler = new TestSampler { DesiredSamplingResult = new SamplingResult(true) }; - - using var listener = new ActivityListener - { - ShouldListenTo = _ => true, - GetRequestedDataUsingContext = (ref ActivityCreationOptions options) => - Sdk.ComputeActivityDataRequest(options, testSampler), - }; - - ActivitySource.AddActivityListener(listener); - - using (var root = activitySource.StartActivity("root")) - { - Assert.Equal(default(ActivitySpanId), root.ParentSpanId); - - // This enforces the current behavior that the traceId passed to the sampler for the - // root span/activity is not the traceId actually used. - Assert.NotEqual(root.TraceId, testSampler.LatestSamplingParameters.TraceId); - } - - using (var parent = activitySource.StartActivity("parent", ActivityKind.Client)) - { - // This enforces the current behavior that the traceId passed to the sampler for the - // root span/activity is not the traceId actually used. - Assert.NotEqual(parent.TraceId, testSampler.LatestSamplingParameters.TraceId); - using (var child = activitySource.StartActivity("child")) - { - Assert.Equal(parent.TraceId, testSampler.LatestSamplingParameters.TraceId); - Assert.Equal(parent.TraceId, child.TraceId); - Assert.Equal(parent.SpanId, child.ParentSpanId); - } - } - - var customContext = new ActivityContext( - ActivityTraceId.CreateRandom(), - ActivitySpanId.CreateRandom(), - ActivityTraceFlags.None); - using (var fromCustomContext = - activitySource.StartActivity("customContext", ActivityKind.Client, customContext)) - { - Assert.Equal(customContext.TraceId, fromCustomContext.TraceId); - Assert.Equal(customContext.SpanId, fromCustomContext.ParentSpanId); - Assert.NotEqual(customContext.SpanId, fromCustomContext.SpanId); - } - - // Preserve traceId in case span is propagated but not recorded (sampled per OpenTelemetry parlance) and - // no data is requested for children spans. - testSampler.DesiredSamplingResult = new SamplingResult(false); - using (var root = activitySource.StartActivity("root")) - { - Assert.Equal(default(ActivitySpanId), root.ParentSpanId); - - using (var child = activitySource.StartActivity("child")) - { - Assert.Null(child); - Assert.Equal(root.TraceId, testSampler.LatestSamplingParameters.TraceId); - Assert.Same(Activity.Current, root); - } - } - } - - private class TestSampler : Sampler - { - public SamplingResult DesiredSamplingResult { get; set; } - - public SamplingParameters LatestSamplingParameters { get; private set; } - - public override SamplingResult ShouldSample(in SamplingParameters samplingParameters) - { - this.LatestSamplingParameters = samplingParameters; - return this.DesiredSamplingResult; - } - } - } -} From 3319a83299c18ba4f3086045cfbd38ad851e2cf6 Mon Sep 17 00:00:00 2001 From: Cijo Thomas Date: Wed, 5 Aug 2020 00:05:38 -0700 Subject: [PATCH 6/7] markdown fix --- src/OpenTelemetry/CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/src/OpenTelemetry/CHANGELOG.md b/src/OpenTelemetry/CHANGELOG.md index fb6361264e6..a7d0997be28 100644 --- a/src/OpenTelemetry/CHANGELOG.md +++ b/src/OpenTelemetry/CHANGELOG.md @@ -11,6 +11,7 @@ disposes the containing exporter. * `BroadcastActivityProcessor`is disposable and it disposes the processors. * Samplers now get the actual TraceId of the Activity to be created. + ## 0.3.0-beta Released 2020-07-23 From 8234059d1fbebeb047ccf1a4472fea6f6350b4fb Mon Sep 17 00:00:00 2001 From: Cijo Thomas Date: Wed, 5 Aug 2020 00:09:03 -0700 Subject: [PATCH 7/7] space --- src/OpenTelemetry/CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/OpenTelemetry/CHANGELOG.md b/src/OpenTelemetry/CHANGELOG.md index a7d0997be28..d780fc4d852 100644 --- a/src/OpenTelemetry/CHANGELOG.md +++ b/src/OpenTelemetry/CHANGELOG.md @@ -11,7 +11,7 @@ disposes the containing exporter. * `BroadcastActivityProcessor`is disposable and it disposes the processors. * Samplers now get the actual TraceId of the Activity to be created. - + ## 0.3.0-beta Released 2020-07-23