From 5efa47869b6099d8cb45a0beee4966726a9976bf Mon Sep 17 00:00:00 2001 From: Timothy Mothra Lee Date: Wed, 5 Jul 2023 17:44:07 -0700 Subject: [PATCH 1/5] fix for tags --- .../Internal/ActivityHelperExtensions.cs | 18 ++++++++++++++ .../CHANGELOG.md | 4 ++++ .../Implementation/HttpInListener.cs | 4 ++++ .../Trace/ActivityExtensionsTest.cs | 24 +++++++++++++++++++ .../BasicTests.cs | 12 +++++++--- 5 files changed, 59 insertions(+), 3 deletions(-) diff --git a/src/OpenTelemetry.Api/Internal/ActivityHelperExtensions.cs b/src/OpenTelemetry.Api/Internal/ActivityHelperExtensions.cs index ec844f8c5ad..27de1ff35f2 100644 --- a/src/OpenTelemetry.Api/Internal/ActivityHelperExtensions.cs +++ b/src/OpenTelemetry.Api/Internal/ActivityHelperExtensions.cs @@ -95,6 +95,24 @@ public static object GetTagValue(this Activity activity, string tagName) return null; } + [MethodImpl(MethodImplOptions.AggressiveInlining)] + public static bool TryGetTagValue(this Activity activity, string tagName, out object tagValue) + { + Debug.Assert(activity != null, "Activity should not be null"); + + foreach (ref readonly var tag in activity.EnumerateTagObjects()) + { + if (tag.Key == tagName) + { + tagValue = tag.Value; + return true; + } + } + + tagValue = null; + return false; + } + /// /// Checks if the user provided tag name is the first tag of the and retrieves the tag value. /// diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md b/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md index 3ebe52252f3..37e85adc935 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md @@ -7,6 +7,10 @@ which attributes are emitted by setting the environment variable `OTEL_SEMCONV_STABILITY_OPT_IN`. +* Fixed an issue affecting NET 7.0+. If custom propagation is being used + and a custom sampler adds tags to an Activity that Activity would be dropped. + ([]()) + ## 1.5.0-beta.1 Released 2023-Jun-05 diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs index 809473ad3aa..86ade538a5c 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs @@ -320,7 +320,11 @@ public void OnStopActivity(Activity activity, object payload) } } +#if NET7_0_OR_GREATER + if (activity.TryGetTagValue("IsCreatedByInstrumentation", out var tagValue) && ReferenceEquals(tagValue, bool.TrueString)) +#else if (activity.TryCheckFirstTag("IsCreatedByInstrumentation", out var tagValue) && ReferenceEquals(tagValue, bool.TrueString)) +#endif { // If instrumentation started a new Activity, it must // be stopped here. diff --git a/test/OpenTelemetry.Api.Tests/Trace/ActivityExtensionsTest.cs b/test/OpenTelemetry.Api.Tests/Trace/ActivityExtensionsTest.cs index bb261512020..702d26d4d4c 100644 --- a/test/OpenTelemetry.Api.Tests/Trace/ActivityExtensionsTest.cs +++ b/test/OpenTelemetry.Api.Tests/Trace/ActivityExtensionsTest.cs @@ -212,6 +212,30 @@ public void GetTagValue() Assert.Null(activity.GetTagValue("Tag2")); } + [Fact] + public void TryGetTagValue_Empty() + { + using var activity = new Activity("Test"); + + Assert.False(activity.TryGetTagValue("Tag1", out var value)); + Assert.Null(value); + } + + [Fact] + public void TryGetTagValue() + { + using var activity = new Activity("Test"); + activity.SetTag("Tag1", "Value1"); + + Assert.True(activity.TryGetTagValue("Tag1", out var value)); + Assert.Equal("Value1", value); + + Assert.False(activity.TryGetTagValue("tag1", out value)); + Assert.Null(value); + Assert.False(activity.TryGetTagValue("Tag2", out value)); + Assert.Null(value); + } + [Theory] [InlineData("Key", "Value", true)] [InlineData("CustomTag", null, false)] diff --git a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/BasicTests.cs b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/BasicTests.cs index d8b3737ead0..87524f90bc4 100644 --- a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/BasicTests.cs +++ b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/BasicTests.cs @@ -219,7 +219,11 @@ public async Task CustomPropagator() builder.ConfigureTestServices(services => { Sdk.SetDefaultTextMapPropagator(propagator.Object); - this.tracerProvider = Sdk.CreateTracerProviderBuilder().AddAspNetCoreInstrumentation().AddInMemoryExporter(exportedItems).Build(); + this.tracerProvider = Sdk.CreateTracerProviderBuilder() + .SetSampler(new TestSampler(SamplingDecision.RecordAndSample, new Dictionary { { "SomeTag", "SomeKey" }, })) + .AddAspNetCoreInstrumentation() + .AddInMemoryExporter(exportedItems) + .Build(); }); builder.ConfigureLogging(loggingBuilder => loggingBuilder.ClearProviders()); })) @@ -1134,15 +1138,17 @@ public override void Inject(PropagationContext context, T carrier, Action> attributes; - public TestSampler(SamplingDecision samplingDecision) + public TestSampler(SamplingDecision samplingDecision, IEnumerable> attributes = null) { this.samplingDecision = samplingDecision; + this.attributes = attributes; } public override SamplingResult ShouldSample(in SamplingParameters samplingParameters) { - return new SamplingResult(this.samplingDecision); + return new SamplingResult(this.samplingDecision, this.attributes); } } From e6a149f2bb37be4d4ccc03dd62ca76731a808484 Mon Sep 17 00:00:00 2001 From: Timothy Mothra Lee Date: Wed, 5 Jul 2023 17:52:01 -0700 Subject: [PATCH 2/5] changelog --- src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md b/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md index 37e85adc935..77070c0ca74 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md @@ -9,7 +9,7 @@ * Fixed an issue affecting NET 7.0+. If custom propagation is being used and a custom sampler adds tags to an Activity that Activity would be dropped. - ([]()) + ([#4637](https://github.com/open-telemetry/opentelemetry-dotnet/pull/4637)) ## 1.5.0-beta.1 From 71e1974b891d0fbb22673b3eefe82315b88aff11 Mon Sep 17 00:00:00 2001 From: Timothy Mothra Date: Fri, 7 Jul 2023 10:09:01 -0700 Subject: [PATCH 3/5] Update src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md Co-authored-by: Vishwesh Bankwar --- src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md b/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md index 77070c0ca74..42f97a41eb6 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md @@ -8,7 +8,7 @@ `OTEL_SEMCONV_STABILITY_OPT_IN`. * Fixed an issue affecting NET 7.0+. If custom propagation is being used - and a custom sampler adds tags to an Activity that Activity would be dropped. + and tags are added to an Activity during sampling then that Activity would be dropped. ([#4637](https://github.com/open-telemetry/opentelemetry-dotnet/pull/4637)) ## 1.5.0-beta.1 From 0f43d10d1356ddd3d51eeeb17789316218ea4ae3 Mon Sep 17 00:00:00 2001 From: Timothy Mothra Lee Date: Mon, 10 Jul 2023 10:02:30 -0700 Subject: [PATCH 4/5] remove tryget --- .../Internal/ActivityHelperExtensions.cs | 18 -------------- .../Implementation/HttpInListener.cs | 3 ++- .../Trace/ActivityExtensionsTest.cs | 24 ------------------- 3 files changed, 2 insertions(+), 43 deletions(-) diff --git a/src/OpenTelemetry.Api/Internal/ActivityHelperExtensions.cs b/src/OpenTelemetry.Api/Internal/ActivityHelperExtensions.cs index 27de1ff35f2..ec844f8c5ad 100644 --- a/src/OpenTelemetry.Api/Internal/ActivityHelperExtensions.cs +++ b/src/OpenTelemetry.Api/Internal/ActivityHelperExtensions.cs @@ -95,24 +95,6 @@ public static object GetTagValue(this Activity activity, string tagName) return null; } - [MethodImpl(MethodImplOptions.AggressiveInlining)] - public static bool TryGetTagValue(this Activity activity, string tagName, out object tagValue) - { - Debug.Assert(activity != null, "Activity should not be null"); - - foreach (ref readonly var tag in activity.EnumerateTagObjects()) - { - if (tag.Key == tagName) - { - tagValue = tag.Value; - return true; - } - } - - tagValue = null; - return false; - } - /// /// Checks if the user provided tag name is the first tag of the and retrieves the tag value. /// diff --git a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs index 86ade538a5c..1963659b167 100644 --- a/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs +++ b/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs @@ -321,7 +321,8 @@ public void OnStopActivity(Activity activity, object payload) } #if NET7_0_OR_GREATER - if (activity.TryGetTagValue("IsCreatedByInstrumentation", out var tagValue) && ReferenceEquals(tagValue, bool.TrueString)) + var tagValue = activity.GetTagValue("IsCreatedByInstrumentation"); + if (ReferenceEquals(tagValue, bool.TrueString)) #else if (activity.TryCheckFirstTag("IsCreatedByInstrumentation", out var tagValue) && ReferenceEquals(tagValue, bool.TrueString)) #endif diff --git a/test/OpenTelemetry.Api.Tests/Trace/ActivityExtensionsTest.cs b/test/OpenTelemetry.Api.Tests/Trace/ActivityExtensionsTest.cs index 702d26d4d4c..bb261512020 100644 --- a/test/OpenTelemetry.Api.Tests/Trace/ActivityExtensionsTest.cs +++ b/test/OpenTelemetry.Api.Tests/Trace/ActivityExtensionsTest.cs @@ -212,30 +212,6 @@ public void GetTagValue() Assert.Null(activity.GetTagValue("Tag2")); } - [Fact] - public void TryGetTagValue_Empty() - { - using var activity = new Activity("Test"); - - Assert.False(activity.TryGetTagValue("Tag1", out var value)); - Assert.Null(value); - } - - [Fact] - public void TryGetTagValue() - { - using var activity = new Activity("Test"); - activity.SetTag("Tag1", "Value1"); - - Assert.True(activity.TryGetTagValue("Tag1", out var value)); - Assert.Equal("Value1", value); - - Assert.False(activity.TryGetTagValue("tag1", out value)); - Assert.Null(value); - Assert.False(activity.TryGetTagValue("Tag2", out value)); - Assert.Null(value); - } - [Theory] [InlineData("Key", "Value", true)] [InlineData("CustomTag", null, false)] From 5cae4610ceda84841e6a2fafa0f75bb92e52bf77 Mon Sep 17 00:00:00 2001 From: Timothy Mothra Lee Date: Mon, 10 Jul 2023 15:07:20 -0700 Subject: [PATCH 5/5] change test to run w/ and w/o sampler --- .../BasicTests.cs | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/BasicTests.cs b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/BasicTests.cs index 87524f90bc4..326cf4d5705 100644 --- a/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/BasicTests.cs +++ b/test/OpenTelemetry.Instrumentation.AspNetCore.Tests/BasicTests.cs @@ -194,8 +194,10 @@ public async Task SuccessfulTemplateControllerCallUsesParentContext() ValidateAspNetCoreActivity(activity, "/api/values/2"); } - [Fact] - public async Task CustomPropagator() + [Theory] + [InlineData(true)] + [InlineData(false)] + public async Task CustomPropagator(bool addSampler) { try { @@ -219,8 +221,15 @@ public async Task CustomPropagator() builder.ConfigureTestServices(services => { Sdk.SetDefaultTextMapPropagator(propagator.Object); - this.tracerProvider = Sdk.CreateTracerProviderBuilder() - .SetSampler(new TestSampler(SamplingDecision.RecordAndSample, new Dictionary { { "SomeTag", "SomeKey" }, })) + var tracerProviderBuilder = Sdk.CreateTracerProviderBuilder(); + + if (addSampler) + { + tracerProviderBuilder + .SetSampler(new TestSampler(SamplingDecision.RecordAndSample, new Dictionary { { "SomeTag", "SomeKey" }, })); + } + + this.tracerProvider = tracerProviderBuilder .AddAspNetCoreInstrumentation() .AddInMemoryExporter(exportedItems) .Build();