Opentracing shim noop cases (#4668)

pjanotti · web-flow · commit d016bb15cd78 · 2023-07-21T14:30:09.000-07:00
diff --git a/src/OpenTelemetry.Shims.OpenTracing/CHANGELOG.md b/src/OpenTelemetry.Shims.OpenTracing/CHANGELOG.md
@@ -2,6 +2,10 @@
 
 ## Unreleased
 
+* Fix: Do not raise `ArgumentException` if `Activity` behind the shim span
+  has an invalid context.
+  ([#2787](https://github.com/open-telemetry/opentelemetry-dotnet/issues/2787))
+
 ## 1.5.0-beta.1
 
 Released 2023-Jun-05
diff --git a/src/OpenTelemetry.Shims.OpenTracing/SpanContextShim.cs b/src/OpenTelemetry.Shims.OpenTracing/SpanContextShim.cs
@@ -22,11 +22,6 @@ internal sealed class SpanContextShim : ISpanContext
     {
         public SpanContextShim(in Trace.SpanContext spanContext)
         {
-            if (!spanContext.IsValid)
-            {
-                throw new ArgumentException($"Invalid '{nameof(Trace.SpanContext)}'", nameof(spanContext));
-            }
-
             this.SpanContext = spanContext;
         }
 
diff --git a/src/OpenTelemetry.Shims.OpenTracing/SpanShim.cs b/src/OpenTelemetry.Shims.OpenTracing/SpanShim.cs
@@ -45,15 +45,11 @@ public SpanShim(TelemetrySpan span)
         {
             Guard.ThrowIfNull(span);
 
-            if (!span.Context.IsValid)
-            {
-                throw new ArgumentException($"Invalid '{nameof(SpanContext)}'", nameof(span.Context));
-            }
-
             this.Span = span;
             this.spanContextShim = new SpanContextShim(this.Span.Context);
         }
 
+        /// <inheritdoc/>
         public ISpanContext Context => this.spanContextShim;
 
         public TelemetrySpan Span { get; private set; }
diff --git a/test/OpenTelemetry.Shims.OpenTracing.Tests/IntegrationTests.cs b/test/OpenTelemetry.Shims.OpenTracing.Tests/IntegrationTests.cs
@@ -0,0 +1,149 @@
+// <copyright file="IntegrationTests.cs" company="OpenTelemetry Authors">
+// 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.
+// </copyright>
+
+using System.Diagnostics;
+using OpenTelemetry.Context.Propagation;
+using OpenTelemetry.Trace;
+using OpenTracing;
+using Xunit;
+
+namespace OpenTelemetry.Shims.OpenTracing.Tests
+{
+    public class IntegrationTests
+    {
+        private const string ChildActivitySource = "ChildActivitySource";
+        private const string ParentActivitySource = "ParentActivitySource";
+        private const string ShimTracerName = "OpenTracing.Shim";
+
+        [Theory]
+        [InlineData(SamplingDecision.Drop, SamplingDecision.Drop, SamplingDecision.Drop)]
+        [InlineData(SamplingDecision.Drop, SamplingDecision.RecordAndSample, SamplingDecision.Drop)]
+        [InlineData(SamplingDecision.Drop, SamplingDecision.RecordOnly, SamplingDecision.Drop)]
+        [InlineData(SamplingDecision.RecordOnly, SamplingDecision.RecordAndSample, SamplingDecision.RecordOnly)]
+        [InlineData(SamplingDecision.RecordAndSample, SamplingDecision.RecordOnly, SamplingDecision.RecordAndSample)]
+        [InlineData(SamplingDecision.RecordAndSample, SamplingDecision.Drop, SamplingDecision.RecordAndSample)]
+        public void WithActivities(
+            SamplingDecision parentActivitySamplingDecision,
+            SamplingDecision shimSamplingDecision,
+            SamplingDecision childActivitySamplingDecision)
+        {
+            var exportedSpans = new List<Activity>();
+
+            const string ParentActivityName = "ParentActivity";
+            const string ShimActivityName = "ShimActivity";
+            const string ChildActivityName = "ChildActivity";
+
+            var testSampler = new TestSampler((samplingParameters) =>
+                samplingParameters.Name switch
+                {
+                    ParentActivityName => parentActivitySamplingDecision,
+                    ShimActivityName => shimSamplingDecision,
+                    ChildActivityName => childActivitySamplingDecision,
+                    _ => SamplingDecision.Drop,
+                });
+
+            using var tracerProvider = Sdk.CreateTracerProviderBuilder()
+                .AddInMemoryExporter(exportedSpans)
+                .SetSampler(testSampler)
+                .When(
+                    parentActivitySamplingDecision == SamplingDecision.RecordAndSample,
+                    b => b.AddSource(ParentActivitySource))
+                .When(
+                    shimSamplingDecision == SamplingDecision.RecordAndSample,
+                    b => b.AddSource(ShimTracerName))
+                .When(
+                    childActivitySamplingDecision == SamplingDecision.RecordAndSample,
+                    b => b.AddSource(ChildActivitySource))
+                .Build();
+
+            ITracer otTracer = new TracerShim(
+                tracerProvider.GetTracer(ShimTracerName),
+                Propagators.DefaultTextMapPropagator);
+
+            // Real usage requires a call OpenTracing.Util.GlobalTracer.Register(otTracer),
+            // however, that can only happen once per process, we don't do it here so we
+            // can run multiple tests in the same process.
+
+            using var parentActivitySource = new ActivitySource(ParentActivitySource);
+            using var childActivitySource = new ActivitySource(ChildActivitySource);
+
+            using (var parentActivity = parentActivitySource.StartActivity(ParentActivityName))
+            {
+                using (IScope parentScope = otTracer.BuildSpan(ShimActivityName).StartActive())
+                {
+                    parentScope.Span.SetTag("parent", true);
+
+                    using var childActivity = childActivitySource.StartActivity(ChildActivityName);
+                }
+            }
+
+            var expectedExportedSpans = new string[]
+                {
+                    childActivitySamplingDecision == SamplingDecision.RecordAndSample ? ChildActivityName : null,
+                    shimSamplingDecision == SamplingDecision.RecordAndSample ? ShimActivityName : null,
+                    parentActivitySamplingDecision == SamplingDecision.RecordAndSample ? ParentActivityName : null,
+                }
+                .Where(s => s is not null)
+                .ToList();
+
+            for (int i = 0; i < expectedExportedSpans.Count; i++)
+            {
+                Assert.Equal(expectedExportedSpans[i], exportedSpans[i].DisplayName);
+            }
+
+            if (childActivitySamplingDecision == SamplingDecision.RecordAndSample)
+            {
+                if (shimSamplingDecision == SamplingDecision.RecordAndSample)
+                {
+                    Assert.Same(exportedSpans[1], exportedSpans[0].Parent);
+                }
+            }
+        }
+
+        private class TestSampler : Sampler
+        {
+            private readonly Func<SamplingParameters, SamplingDecision> shouldSampleDelegate;
+
+            public TestSampler(Func<SamplingParameters, SamplingDecision> shouldSampleDelegate)
+            {
+                this.shouldSampleDelegate = shouldSampleDelegate;
+            }
+
+            public override SamplingResult ShouldSample(in SamplingParameters samplingParameters)
+            {
+                return new SamplingResult(this.shouldSampleDelegate(samplingParameters));
+            }
+        }
+    }
+
+    [System.Diagnostics.CodeAnalysis.SuppressMessage(
+        "StyleCop.CSharp.MaintainabilityRules", "SA1402:File may only contain a single type", Justification = "Local use only")]
+    internal static class ConditionalTracerProviderBuilderExtension
+    {
+        public static TracerProviderBuilder When(
+            this TracerProviderBuilder builder,
+            bool condition,
+            Func<TracerProviderBuilder, TracerProviderBuilder> conditionalDelegate)
+        {
+            if (condition)
+            {
+                builder = conditionalDelegate(builder);
+            }
+
+            return builder;
+        }
+    }
+}
diff --git a/test/OpenTelemetry.Shims.OpenTracing.Tests/ListenAndSampleAllActivitySources.cs b/test/OpenTelemetry.Shims.OpenTracing.Tests/ListenAndSampleAllActivitySources.cs
@@ -0,0 +1,49 @@
+// <copyright file="ListenAndSampleAllActivitySources.cs" company="OpenTelemetry Authors">
+// 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.
+// </copyright>
+
+using System.Diagnostics;
+using Xunit;
+
+namespace OpenTelemetry.Shims.OpenTracing.Tests
+{
+    [CollectionDefinition(nameof(ListenAndSampleAllActivitySources))]
+    public sealed class ListenAndSampleAllActivitySources : ICollectionFixture<ListenAndSampleAllActivitySources.Fixture>
+    {
+        public sealed class Fixture : IDisposable
+        {
+            private readonly ActivityListener listener;
+
+            public Fixture()
+            {
+                Activity.DefaultIdFormat = ActivityIdFormat.W3C;
+                Activity.ForceDefaultIdFormat = true;
+
+                this.listener = new ActivityListener
+                {
+                    ShouldListenTo = _ => true,
+                    Sample = (ref ActivityCreationOptions<ActivityContext> options) => ActivitySamplingResult.AllData,
+                };
+
+                ActivitySource.AddActivityListener(this.listener);
+            }
+
+            public void Dispose()
+            {
+                this.listener.Dispose();
+            }
+        }
+    }
+}
diff --git a/test/OpenTelemetry.Shims.OpenTracing.Tests/OpenTelemetry.Shims.OpenTracing.Tests.csproj b/test/OpenTelemetry.Shims.OpenTracing.Tests/OpenTelemetry.Shims.OpenTracing.Tests.csproj
@@ -3,7 +3,7 @@
     <Description>Unit test project for OpenTelemetry.Shims.OpenTracing</Description>
     <!-- OmniSharp/VS Code requires TargetFrameworks to be in descending order for IntelliSense and analysis. -->
     <TargetFrameworks>net7.0;net6.0</TargetFrameworks>
-
+    <TargetFrameworks Condition="$(OS) == 'Windows_NT'">$(TargetFrameworks);net462</TargetFrameworks>
     <!-- this is temporary. will remove in future PR. -->
     <Nullable>disable</Nullable>
   </PropertyGroup>
@@ -19,6 +19,8 @@
   </ItemGroup>
 
   <ItemGroup>
+    <ProjectReference Include="$(RepoRoot)\src\OpenTelemetry\OpenTelemetry.csproj" />
+    <ProjectReference Include="$(RepoRoot)\src\OpenTelemetry.Exporter.InMemory\OpenTelemetry.Exporter.InMemory.csproj" />
     <ProjectReference Include="$(RepoRoot)\src\OpenTelemetry.Shims.OpenTracing\OpenTelemetry.Shims.OpenTracing.csproj" />
   </ItemGroup>
 </Project>
diff --git a/test/OpenTelemetry.Shims.OpenTracing.Tests/ScopeManagerShimTests.cs b/test/OpenTelemetry.Shims.OpenTracing.Tests/ScopeManagerShimTests.cs
@@ -21,25 +21,12 @@
 
 namespace OpenTelemetry.Shims.OpenTracing.Tests
 {
+    [Collection(nameof(ListenAndSampleAllActivitySources))]
     public class ScopeManagerShimTests
     {
         private const string SpanName = "MySpanName/1";
         private const string TracerName = "defaultactivitysource";
 
-        static ScopeManagerShimTests()
-        {
-            Activity.DefaultIdFormat = ActivityIdFormat.W3C;
-            Activity.ForceDefaultIdFormat = true;
-
-            var listener = new ActivityListener
-            {
-                ShouldListenTo = _ => true,
-                Sample = (ref ActivityCreationOptions<ActivityContext> options) => ActivitySamplingResult.AllData,
-            };
-
-            ActivitySource.AddActivityListener(listener);
-        }
-
         [Fact]
         public void CtorArgumentValidation()
         {
diff --git a/test/OpenTelemetry.Shims.OpenTracing.Tests/SpanBuilderShimTests.cs b/test/OpenTelemetry.Shims.OpenTracing.Tests/SpanBuilderShimTests.cs
@@ -20,26 +20,13 @@
 
 namespace OpenTelemetry.Shims.OpenTracing.Tests
 {
+    [Collection(nameof(ListenAndSampleAllActivitySources))]
     public class SpanBuilderShimTests
     {
         private const string SpanName1 = "MySpanName/1";
         private const string SpanName2 = "MySpanName/2";
         private const string TracerName = "defaultactivitysource";
 
-        static SpanBuilderShimTests()
-        {
-            Activity.DefaultIdFormat = ActivityIdFormat.W3C;
-            Activity.ForceDefaultIdFormat = true;
-
-            var listener = new ActivityListener
-            {
-                ShouldListenTo = _ => true,
-                Sample = (ref ActivityCreationOptions<ActivityContext> options) => ActivitySamplingResult.AllData,
-            };
-
-            ActivitySource.AddActivityListener(listener);
-        }
-
         [Fact]
         public void CtorArgumentValidation()
         {
diff --git a/test/OpenTelemetry.Shims.OpenTracing.Tests/SpanContextShimTests.cs b/test/OpenTelemetry.Shims.OpenTracing.Tests/SpanContextShimTests.cs
@@ -22,13 +22,6 @@ namespace OpenTelemetry.Shims.OpenTracing.Tests
 {
     public class SpanContextShimTests
     {
-        [Fact]
-        public void CtorArgumentValidation()
-        {
-            Assert.Throws<ArgumentException>(() => new SpanContextShim(default));
-            Assert.Throws<ArgumentException>(() => new SpanContextShim(new SpanContext(default, default, ActivityTraceFlags.None)));
-        }
-
         [Fact]
         public void GetTraceId()
         {
diff --git a/test/OpenTelemetry.Shims.OpenTracing.Tests/SpanShimTests.cs b/test/OpenTelemetry.Shims.OpenTracing.Tests/SpanShimTests.cs
@@ -14,32 +14,18 @@
 // limitations under the License.
 // </copyright>
 
-using System.Diagnostics;
 using OpenTelemetry.Trace;
 using OpenTracing.Tag;
 using Xunit;
 
 namespace OpenTelemetry.Shims.OpenTracing.Tests
 {
+    [Collection(nameof(ListenAndSampleAllActivitySources))]
     public class SpanShimTests
     {
         private const string SpanName = "MySpanName/1";
         private const string TracerName = "defaultactivitysource";
 
-        static SpanShimTests()
-        {
-            Activity.DefaultIdFormat = ActivityIdFormat.W3C;
-            Activity.ForceDefaultIdFormat = true;
-
-            var listener = new ActivityListener
-            {
-                ShouldListenTo = _ => true,
-                Sample = (ref ActivityCreationOptions<ActivityContext> options) => ActivitySamplingResult.AllData,
-            };
-
-            ActivitySource.AddActivityListener(listener);
-        }
-
         [Fact]
         public void CtorArgumentValidation()
         {
@@ -73,6 +59,14 @@ public void FinishSpanUsingSpecificTimestamp()
             var tracer = TracerProvider.Default.GetTracer(TracerName);
             var shim = new SpanShim(tracer.StartSpan(SpanName));
 
+#if NETFRAMEWORK
+            // Under the hood the Activity start time uses DateTime.UtcNow, which
+            // doesn't have the same precision as DateTimeOffset.UtcNow on the .NET Framework.
+            // Add a sleep big enough to ensure that the test doesn't break due to the
+            // low resolution of DateTime.UtcNow on the .NET Framework.
+            Thread.Sleep(TimeSpan.FromMilliseconds(20));
+#endif
+
             var endTime = DateTimeOffset.UtcNow;
             shim.Finish(endTime);
 

Original file line number	Diff line number	Diff line change
`@@ -22,11 +22,6 @@ internal sealed class SpanContextShim : ISpanContext`
`22`	`22`	`{`
`23`	`23`	`public SpanContextShim(in Trace.SpanContext spanContext)`
`24`	`24`	`{`
`25`		`- if (!spanContext.IsValid)`
`26`		`- {`
`27`		`- throw new ArgumentException($"Invalid '{nameof(Trace.SpanContext)}'", nameof(spanContext));`
`28`		`- }`
`29`		`-`
`30`	`25`	`this.SpanContext = spanContext;`
`31`	`26`	`}`
`32`	`27`
Original file line number	Diff line number	Diff line change
`@@ -45,15 +45,11 @@ public SpanShim(TelemetrySpan span)`
`45`	`45`	`{`
`46`	`46`	`Guard.ThrowIfNull(span);`
`47`	`47`
`48`		`- if (!span.Context.IsValid)`
`49`		`- {`
`50`		`- throw new ArgumentException($"Invalid '{nameof(SpanContext)}'", nameof(span.Context));`
`51`		`- }`
`52`		`-`
`53`	`48`	`this.Span = span;`
`54`	`49`	`this.spanContextShim = new SpanContextShim(this.Span.Context);`
`55`	`50`	`}`
`56`	`51`
	`52`	`+ /// <inheritdoc/>`
`57`	`53`	`public ISpanContext Context => this.spanContextShim;`
`58`	`54`
`59`	`55`	`public TelemetrySpan Span { get; private set; }`
Original file line number	Diff line number	Diff line change
`@@ -22,13 +22,6 @@ namespace OpenTelemetry.Shims.OpenTracing.Tests`
`22`	`22`	`{`
`23`	`23`	`public class SpanContextShimTests`
`24`	`24`	`{`
`25`		`- [Fact]`
`26`		`- public void CtorArgumentValidation()`
`27`		`- {`
`28`		`- Assert.Throws<ArgumentException>(() => new SpanContextShim(default));`
`29`		`- Assert.Throws<ArgumentException>(() => new SpanContextShim(new SpanContext(default, default, ActivityTraceFlags.None)));`
`30`		`- }`
`31`		`-`
`32`	`25`	`[Fact]`
`33`	`26`	`public void GetTraceId()`
`34`	`27`	`{`