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.

Sign up for GitHub

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

[di-tracing] Fix the TracerProviderBuilder.AddInstrumentation factory pattern extension #4468

Merged
merged 4 commits into from
May 8, 2023
Merged

[di-tracing] Fix the TracerProviderBuilder.AddInstrumentation factory pattern extension #4468

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
207f687
Fix the TracerProviderBuilder.AddInstrumentation factory pattern exte…
CodeBlanch May 5, 2023
3576ca7
CHANGELOG patch.
CodeBlanch May 5, 2023
b129258
Merge branch 'main' into tracing-addinstrumentation-factory-fix
CodeBlanch May 5, 2023
7854f38
CHANGELOG tweak.
CodeBlanch May 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/OpenTelemetry.Api.ProviderBuilderExtensions/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@

## Unreleased

* Fixed the `TracerProviderBuilder.AddInstrumentation` factory extension.
Copy link
Member

@alanwest alanwest May 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Can you describe more or give an example of what happened if you used this before the fix? I'm assuming builder is a different instance than tracerProviderBuilder, and so calling AddInstrumentation had no effect?

Copy link
Member Author

Choose a reason for hiding this comment

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

Can't tell if you are asking me to explain that here or in the CHANGELOG 🤣 I'll start with just here...

I'm assuming builder is a different instance than tracerProviderBuilder, and so calling AddInstrumentation had no effect?

Correct! Calling it before this fix it will just no-op.

Copy link
Contributor

Choose a reason for hiding this comment

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

So tracerProviderBuilder instance is not ITracerProviderBuilder, but the builder instance is? Then, what about the same check in ConfigureServices? Do we need to worry about that?

Copy link
Member Author

Choose a reason for hiding this comment

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

There's 2 builders...

  • TracerProviderBuilderBase this is the phase 1 builder. Implements ITracerProviderBuilder. Dumps everything into the IServiceCollection.

  • TracerProviderBuilderSdk this is the phase 2 builder which holds the state. Everything from phase 1 is re-played against this builder once IServiceProvider is available in the order it was registered. The state then gets consumed into the TracerProviderSdk.

The working version of the clause is...

            if (builder is ITracerProviderBuilder iTracerProviderBuilder
                && iTracerProviderBuilder.Provider != null)

  • The first part (is ITracerProviderBuilder iTracerProviderBuilder) is true for both TracerProviderBuilderBase & TracerProviderBuilderSdk.

  • The second part (iTracerProviderBuilder.Provider != null) is the more interesting one! Only TracerProviderBuilderSdk has access to the IServiceProvider. During phase 1 we operate on the IServiceCollection. During phase 2 IServiceCollection has been closed and we have the IServiceProvider.

The way the code was written before it was capturing the "phase 1" builder (TracerProviderBuilderBase) so the second clause would evaluate to false even when the final "phase 2" builder was in play.

Yes, this is confusing 😢

Copy link
Member

@alanwest alanwest May 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Can't tell if you are asking me to explain that here or in the CHANGELOG 🤣 I'll start with just here...

Both 😆, appreciate the explanation here, but I also think elaborating in the changelog (or at least the PR description) would be good as it might help users determine if they're impacted.

It does not appear that any of our instrumentation is using this particular overload of AddInstrumentation, so I'd imagine that end users who may be impacted by this bug are ones that are using this overload in some kind of DI scenario.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated the CHANGELOG text to be more specific. LMK if you want it to be more verbose. I'll also spin up an issue for this and link to it from the PR description.

([#4468](https://github.com/open-telemetry/opentelemetry-dotnet/pull/4468))

## 1.5.0-alpha.2

Released 2023-Mar-31
Expand Down
2 changes: 1 addition & 1 deletion ...uilderExtensions/Trace/OpenTelemetryDependencyInjectionTracerProviderBuilderExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ public static TracerProviderBuilder AddInstrumentation<T>(

tracerProviderBuilder.ConfigureBuilder((sp, builder) =>
{
if (tracerProviderBuilder is ITracerProviderBuilder iTracerProviderBuilder
if (builder is ITracerProviderBuilder iTracerProviderBuilder
&& iTracerProviderBuilder.Provider != null)
{
builder.AddInstrumentation(() => instrumentationFactory(sp, iTracerProviderBuilder.Provider));
Expand Down
34 changes: 34 additions & 0 deletions test/OpenTelemetry.Tests/Metrics/MeterProviderBuilderExtensionsTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,39 @@ public void AddReaderUsingDependencyInjectionTest()
Assert.True(reader.Head.Next?.Value is MyReader);
}

[Fact]
public void AddInstrumentationTest()
{
List<object> instrumentation = null;

using (var provider = Sdk.CreateMeterProviderBuilder()
.AddInstrumentation<MyInstrumentation>()
.AddInstrumentation((sp, provider) => new MyInstrumentation() { Provider = provider })
.AddInstrumentation(new MyInstrumentation())
.Build() as MeterProviderSdk)
{
Assert.NotNull(provider);

Assert.Equal(3, provider.Instrumentations.Count);

Assert.Null(((MyInstrumentation)provider.Instrumentations[0]).Provider);
Assert.False(((MyInstrumentation)provider.Instrumentations[0]).Disposed);

Assert.NotNull(((MyInstrumentation)provider.Instrumentations[1]).Provider);
Assert.False(((MyInstrumentation)provider.Instrumentations[1]).Disposed);

Assert.Null(((MyInstrumentation)provider.Instrumentations[2]).Provider);
Assert.False(((MyInstrumentation)provider.Instrumentations[2]).Disposed);

instrumentation = new List<object>(provider.Instrumentations);
}

Assert.NotNull(instrumentation);
Assert.True(((MyInstrumentation)instrumentation[0]).Disposed);
Assert.True(((MyInstrumentation)instrumentation[1]).Disposed);
Assert.True(((MyInstrumentation)instrumentation[2]).Disposed);
}

[Fact]
public void SetAndConfigureResourceTest()
{
Expand Down Expand Up @@ -303,6 +336,7 @@ private static void RunBuilderServiceLifecycleTest(

private sealed class MyInstrumentation : IDisposable
{
internal MeterProvider Provider;
internal bool Disposed;

public void Dispose()
Expand Down
34 changes: 34 additions & 0 deletions test/OpenTelemetry.Tests/Trace/TracerProviderBuilderExtensionsTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,39 @@ public void AddProcessorUsingDependencyInjectionTest()
Assert.True(processor.Head.Next?.Value is MyProcessor);
}

[Fact]
public void AddInstrumentationTest()
{
List<object> instrumentation = null;

using (var provider = Sdk.CreateTracerProviderBuilder()
.AddInstrumentation<MyInstrumentation>()
.AddInstrumentation((sp, provider) => new MyInstrumentation() { Provider = provider })
.AddInstrumentation(new MyInstrumentation())
.Build() as TracerProviderSdk)
{
Assert.NotNull(provider);

Assert.Equal(3, provider.Instrumentations.Count);

Assert.Null(((MyInstrumentation)provider.Instrumentations[0]).Provider);
Assert.False(((MyInstrumentation)provider.Instrumentations[0]).Disposed);

Assert.NotNull(((MyInstrumentation)provider.Instrumentations[1]).Provider);
Assert.False(((MyInstrumentation)provider.Instrumentations[1]).Disposed);

Assert.Null(((MyInstrumentation)provider.Instrumentations[2]).Provider);
Assert.False(((MyInstrumentation)provider.Instrumentations[2]).Disposed);

instrumentation = new List<object>(provider.Instrumentations);
}

Assert.NotNull(instrumentation);
Assert.True(((MyInstrumentation)instrumentation[0]).Disposed);
Assert.True(((MyInstrumentation)instrumentation[1]).Disposed);
Assert.True(((MyInstrumentation)instrumentation[2]).Disposed);
}

[Fact]
public void SetAndConfigureResourceTest()
{
Expand Down Expand Up @@ -431,6 +464,7 @@ public override SamplingResult ShouldSample(in SamplingParameters samplingParame

private sealed class MyInstrumentation : IDisposable
{
internal TracerProvider Provider;
internal bool Disposed;

public void Dispose()
Expand Down