[OpenTelementry] Fix circular reference

[martincostello]

Found in #6899.

Changes

Fix circular reference when LoggerProvider requires ILogger.

My only hesitation on this change as-is is the fact that I've had to update two tests outside of OpenTelemetry.Tests to account for this change and that the exception is only now thrown at the point ILogger is resolved and not ILoggerFactory. Thoughts?

Merge requirement checklist

• CONTRIBUTING guidelines followed (license requirements, nullable enabled, static analysis, etc.)
• Unit tests added/updated
• Appropriate CHANGELOG.md files updated for non-trivial changes
• Changes in public API reviewed (if applicable)

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.72%. Comparing base (2ea65f2) to head (8341dbf).
⚠️ Report is 1 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7308      +/-   ##
==========================================
- Coverage   89.81%   89.72%   -0.10%     
==========================================
  Files         276      276              
  Lines       14822    14838      +16     
==========================================
  Hits        13313    13313              
- Misses       1509     1525      +16     

Flag	Coverage Δ
unittests-Project-Experimental	89.61% <100.00%> (-0.15%)	⬇️
unittests-Project-Stable	89.61% <100.00%> (-0.20%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
...emetry/Logs/ILogger/OpenTelemetryLoggerProvider.cs	94.59% <100.00%> (+1.15%)	⬆️
...try/Logs/ILogger/OpenTelemetryLoggingExtensions.cs	100.00% <100.00%> (ø)

... and 5 files with indirect coverage changes

[Copilot]

Pull request overview

This PR addresses a circular dependency in the logging DI integration when LoggerProvider construction requires ILogger/ILoggerFactory (e.g., via dependencies like IHttpClientFactory). It does so by deferring LoggerProvider resolution until the first logger is actually created, avoiding a re-entrant LoggerProvider build.

Changes:

• Update OpenTelemetryLoggingExtensions to pass a Func<LoggerProvider> into OpenTelemetryLoggerProvider, delaying LoggerProvider resolution to CreateLogger.
• Make OpenTelemetryLoggerProvider lazily initialize and cache the LoggerProvider from the provided factory (thread-safe).
• Update affected tests (and changelog) to trigger the new lazy behavior by creating a logger.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/OpenTelemetry/Logs/ILogger/OpenTelemetryLoggingExtensions.cs	Defers LoggerProvider resolution via a factory to avoid circular references during DI construction.
src/OpenTelemetry/Logs/ILogger/OpenTelemetryLoggerProvider.cs	Lazily resolves/caches LoggerProvider with synchronization to support deferred DI resolution.
test/OpenTelemetry.Tests/Logs/OpenTelemetryLoggingExtensionsTests.cs	Adjusts assertions and ensures circular-reference scenario is exercised by creating a logger.
test/OpenTelemetry.Exporter.OpenTelemetryProtocol.Tests/OtlpLogExporterTests.cs	Forces logger creation so configuration delegates are exercised under the new lazy-resolution behavior.
test/OpenTelemetry.Exporter.OpenTelemetryProtocol.Tests/OtlpExporterHelperExtensionsTests.cs	Updates the “throws on gRPC default factory” logging test to trigger failure via CreateLogger.
src/OpenTelemetry/CHANGELOG.md	Notes the circular reference fix.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[rajkumar-rangaraj]

Asked Noah and Tarek to take a look at this PR.

[tarekgh]

Thanks for digging this one out, nice find. The overall approach looks right to me: deferring the LoggerProvider resolution out of the DI construction graph and into a lazily-invoked factory on first CreateLogger is a clean way to break the re-entrant build, and since ILoggerProvider is a singleton bound to the root provider, capturing sp/state in the closure is lifetime-safe. Disposal stays correct too (this.provider?.Dispose() only runs when the provider was actually created).

A few things worth considering before this goes in:

1. The deferred exception timing (your stated hesitation). I think this is the most important point to get explicit agreement on. Today a misconfiguration (like the OTLP gRPC default-factory NotSupportedException) fails at composition time, when ILoggerProvider/ILoggerFactory is resolved. After this change it only surfaces when the first log record is written, as the OtlpExporterHelperExtensionsTests updates show. That trades startup fail-fast for a failure that can now happen on a logging code path after the app already looked healthy. It is probably acceptable, but it feels like a behavior change that deserves a maintainer call and maybe a more explicit note in the changelog than "Fix circular reference".

2. Memory model on the lazy field. The Provider getter reads this.provider lock-free on the fast path, but the field is a plain LoggerProvider? (the write happens under the lock). That is the classic double-checked-locking shape that needs the field to be volatile (or accessed via Volatile.Read/Volatile.Write) to be correct on weaker memory models. Low probability in practice, but easy to make airtight.

3. Worth weighing Lazy<LoggerProvider>? It would give the correct barriers for free and trim the hand-rolled locking. The one semantic difference to be deliberate about: with LazyThreadSafetyMode.ExecutionAndPublication a thrown factory exception is cached and rethrown, whereas the current code leaves the factory in place and retries on the next CreateLogger. Given point 1 (the factory can throw), retry-versus-cache is a real choice. If retry is intended, keeping the manual version plus Volatile is perfectly fine.

4. Does CircularReferenceTest fail without the production change? CircularReferenceTest already passes on main, and with the pre-fix eager resolution the ILoggerFactory-first branch builds the provider fully during factory resolution, so the added CreateLogger call would hit an already-populated Provider. That makes me wonder whether the test actually reproduces the cycle on the currently-built target frameworks, or only under the specific environment where it was found. Could you confirm it fails on a reverted build, and on which target framework? If it only repros in that one spot, a short comment in the test saying so would make the guard clear.

None of these block the direction, which I think is correct. Mostly want to make sure the exception-timing change is a conscious decision. Thanks again!

[noahfalk]

I don't think I have anything to add to what @tarekgh already called out :)

[martincostello]

Comments should now be addressed.