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

[CodeBlanch]

Relates to #4471

Changes

• Fix a bug which prevents the TracerProviderBuilder.AddInstrumentation(IServiceProvider, TracerProvider) factory extension from being called during construction of the SDK TracerProvider.

Merge requirement checklist

• CONTRIBUTING guidelines followed (nullable enabled, static analysis, etc.)
• Unit tests added/updated
• Appropriate CHANGELOG.md files updated for non-trivial changes

[CodeBlanch]

@utpilla @cijothomas @alanwest 1.4.0 is bugged. Do we want to hotfix?

[codecov]

Codecov Report

Merging #4468 (7854f38) into main (b9908d1) will increase coverage by 0.03%.
The diff coverage is 100.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4468      +/-   ##
==========================================
+ Coverage   83.30%   83.34%   +0.03%     
==========================================
  Files         314      314              
  Lines       12522    12522              
==========================================
+ Hits        10431    10436       +5     
+ Misses       2091     2086       -5     

Impacted Files	Coverage Δ
...endencyInjectionTracerProviderBuilderExtensions.cs	100.00% <100.00%> (ø)

... and 5 files with indirect coverage changes

[alanwest]

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?

[CodeBlanch]

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.

[utpilla]

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?

[CodeBlanch]

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 😢

[alanwest]

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.

[CodeBlanch]

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.

[alanwest]

1.4.0 is bugged. Do we want to hotfix?

I'm open to considering this, but from what I can tell it's not a very pressing bug fix. Have you heard from anyone bumping into this problem? If not, maybe it would be good to open a corresponding issue to link to this PR, in case anyone does run into this bug and searches issues for AddInstrumentation?

[CodeBlanch]

@alanwest

Have you heard from anyone bumping into this problem?

Not yet! I'm good with leaving it broken in 1.4.0 until a fix is needed.