[Sdk] TracerProviderBuilderBase chained/fluent call fix (#4529)

Co-authored-by: Utkarsh Umesan Pillai <[email protected]>
open-telemetry · May 31, 2023 · 0b81631 · 0b81631
1 parent e85bd93
commit 0b81631
Show file tree

Hide file tree

Showing 3 changed files with 61 additions and 9 deletions.
diff --git a/src/OpenTelemetry/CHANGELOG.md b/src/OpenTelemetry/CHANGELOG.md
@@ -2,6 +2,12 @@
 
 ## Unreleased
 
+* Fixed a bug introduced by
+  [#4508](https://github.com/open-telemetry/opentelemetry-dotnet/pull/4508) in
+  1.5.0-rc.1 which caused the "Build" extension to return `null` when performing
+  chained/fluent calls.
+  ([#4529](https://github.com/open-telemetry/opentelemetry-dotnet/pull/4529))
+
 ## 1.5.0-rc.1
 
 Released 2023-May-25

diff --git a/src/OpenTelemetry/Trace/Builder/TracerProviderBuilderBase.cs b/src/OpenTelemetry/Trace/Builder/TracerProviderBuilderBase.cs
@@ -90,11 +90,19 @@ public override TracerProviderBuilder AddLegacySource(string operationName)
 
     /// <inheritdoc />
     TracerProviderBuilder ITracerProviderBuilder.ConfigureServices(Action<IServiceCollection> configure)
-        => this.innerBuilder.ConfigureServices(configure);
+    {
+        this.innerBuilder.ConfigureServices(configure);
+
+        return this;
+    }
 
     /// <inheritdoc />
     TracerProviderBuilder IDeferredTracerProviderBuilder.Configure(Action<IServiceProvider, TracerProviderBuilder> configure)
-        => this.innerBuilder.ConfigureBuilder(configure);
+    {
+        this.innerBuilder.ConfigureBuilder(configure);
+
+        return this;
+    }
 
     internal TracerProvider InvokeBuild()
         => this.Build();
@@ -115,7 +123,7 @@ protected TracerProviderBuilder AddInstrumentation(
         Guard.ThrowIfNullOrWhitespace(instrumentationVersion);
         Guard.ThrowIfNull(instrumentationFactory);
 
-        return this.innerBuilder.ConfigureBuilder((sp, builder) =>
+        this.innerBuilder.ConfigureBuilder((sp, builder) =>
         {
             if (builder is TracerProviderBuilderSdk tracerProviderBuilderState)
             {
@@ -125,6 +133,8 @@ protected TracerProviderBuilder AddInstrumentation(
                     instrumentationFactory);
             }
         });
+
+        return this;
     }
 
     /// <summary>
@@ -138,12 +148,8 @@ protected TracerProvider Build()
             throw new NotSupportedException("A TracerProviderBuilder bound to external service cannot be built directly. Access the TracerProvider using the application IServiceProvider instead.");
         }
 
-        var services = this.innerBuilder.Services;
-
-        if (services == null)
-        {
-            throw new NotSupportedException("TracerProviderBuilder build method cannot be called multiple times.");
-        }
+        var services = this.innerBuilder.Services
+            ?? throw new NotSupportedException("TracerProviderBuilder build method cannot be called multiple times.");
 
         this.innerBuilder.Services = null;
 

diff --git a/test/OpenTelemetry.Tests/Trace/TracerProviderSdkTest.cs b/test/OpenTelemetry.Tests/Trace/TracerProviderSdkTest.cs
@@ -1218,11 +1218,51 @@ public void SdkSamplesAndProcessesLegacySourceWhenAddLegacySourceIsCalledWithWil
             Assert.Contains(nonLegacyActivity.OperationName, onStopProcessedActivities);  // Processor.OnEnd is called since we added a legacy OperationName
         }
 
+        [Fact]
+        public void BuilderTypeDoesNotChangeTest()
+        {
+            var originalBuilder = new TestTracerProviderBuilder();
+
+            // Tests the protected version of AddInstrumentation on TracerProviderBuilderBase
+            var currentBuilder = originalBuilder.AddInstrumentation();
+            Assert.True(ReferenceEquals(originalBuilder, currentBuilder));
+
+            var deferredBuilder = currentBuilder as IDeferredTracerProviderBuilder;
+            Assert.NotNull(deferredBuilder);
+
+            currentBuilder = deferredBuilder.Configure((sp, innerBuilder) => { });
+            Assert.True(ReferenceEquals(originalBuilder, currentBuilder));
+
+            currentBuilder = currentBuilder.ConfigureServices(s => { });
+            Assert.True(ReferenceEquals(originalBuilder, currentBuilder));
+
+            currentBuilder = currentBuilder.AddInstrumentation(() => new object());
+            Assert.True(ReferenceEquals(originalBuilder, currentBuilder));
+
+            currentBuilder = currentBuilder.AddSource("MySource");
+            Assert.True(ReferenceEquals(originalBuilder, currentBuilder));
+
+            currentBuilder = currentBuilder.AddLegacySource("MyLegacySource");
+            Assert.True(ReferenceEquals(originalBuilder, currentBuilder));
+
+            using var provider = currentBuilder.Build();
+
+            Assert.NotNull(provider);
+        }
+
         public void Dispose()
         {
             GC.SuppressFinalize(this);
         }
 
+        private sealed class TestTracerProviderBuilder : TracerProviderBuilderBase
+        {
+            public TracerProviderBuilder AddInstrumentation()
+            {
+                return this.AddInstrumentation("SomeInstrumentation", "1.0.0", () => new object());
+            }
+        }
+
         private class TestInstrumentation : IDisposable
         {
             public bool IsDisposed;