Skip to content

Fix lazy logger provider retry after failed build - #7761

Merged
Kielek merged 5 commits into
open-telemetry:mainfrom
Kielek:lazyloader-fix
Sep 16, 2026
Merged

Kielek merged 5 commits into
open-telemetry:mainfrom
Kielek:lazyloader-fix

Conversation

@Kielek

@Kielek Kielek commented Sep 16, 2026

Copy link
Copy Markdown
Member

Fixes internal Splunk/Cisco codex scams.

Changes

Fixes a lazy logger-provider retry issue where a failed LoggerProviderSdk
construction could leave a partially initialized provider in builder state.

The failed provider is now removed from LoggerProviderBuilderSdk state before
construction cleanup completes. An identity check ensures that only the failed
provider instance is removed, preserving the early registration required to
break circular ILoggerFactory dependencies.

Added a regression test covering a failed first build, a successful retry, and
processing of a subsequent log record.

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 Minor, I do not have a plan to document it.
  • [ ] Changes in public API reviewed (if applicable)

Copilot AI lite review requested due to automatic review settings September 16, 2026 05:44
@Kielek
Kielek requested a review from a team as a code owner September 16, 2026 05:44
@github-actions github-actions Bot added the pkg:OpenTelemetry Issues related to OpenTelemetry NuGet package label Sep 16, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Retry cleanup may reuse disposed pipeline components after state mutation, and required changelog entries are missing.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Fixes lazy logger-provider retries after failed builds while preserving circular-dependency protection.

Changes:

  • Unregisters failed provider instances safely.
  • Adds retry and log-processing regression coverage.
  • Adds identity protection for provider removal.
File summaries
File Summary
test/OpenTelemetry.Tests/Logs/OpenTelemetryLoggingExtensionsTests.cs Adds failed-build retry coverage.
src/OpenTelemetry/Logs/LoggerProviderSdk.cs Cleans up failed provider registration.
src/OpenTelemetry/Logs/Builder/LoggerProviderBuilderSdk.cs Adds identity-safe provider unregistration.
Review details

Suppressed comments (1)

src/OpenTelemetry/Logs/Builder/LoggerProviderBuilderSdk.cs:42

  • This changes runtime logger-provider behavior by enabling a retry after a failed build. The repository requires every behavioral change to be listed in the affected component's CHANGELOG.md under ## Unreleased with a PR link, but src/OpenTelemetry/CHANGELOG.md is unchanged; please add the entry.
    public void UnregisterProvider(LoggerProviderSdk loggerProvider)
  • Files reviewed: 3/3 changed files
  • Comments generated: 2
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/OpenTelemetry/Logs/LoggerProviderSdk.cs Outdated
Comment thread src/OpenTelemetry/Logs/LoggerProviderSdk.cs Outdated
@codecov

codecov Bot commented Sep 16, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 89.06250% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 91.86%. Comparing base (2103f6d) to head (42ba94e).
⚠️ Report is 5 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
src/OpenTelemetry/Logs/ILogger/NullScope.cs 0.00% 4 Missing ⚠️
...emetry/Logs/ILogger/DeferredOpenTelemetryLogger.cs 93.10% 2 Missing ⚠️
...emetry/Logs/ILogger/OpenTelemetryLoggerProvider.cs 92.85% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #7761      +/-   ##
==========================================
- Coverage   91.92%   91.86%   -0.07%     
==========================================
  Files         337      339       +2     
  Lines       18410    18511     +101     
==========================================
+ Hits        16924    17005      +81     
- Misses       1486     1506      +20     
Flag Coverage Δ
unittests-Project-Experimental 91.95% <89.06%> (-0.04%) ⬇️
unittests-Project-Stable 91.92% <89.06%> (-0.12%) ⬇️

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

Files with missing lines Coverage Δ
...Telemetry/Logs/Builder/LoggerProviderBuilderSdk.cs 100.00% <100.00%> (ø)
.../OpenTelemetry/Logs/ILogger/OpenTelemetryLogger.cs 91.75% <ø> (+1.85%) ⬆️
src/OpenTelemetry/Logs/LoggerProviderSdk.cs 88.42% <100.00%> (+0.39%) ⬆️
...emetry/Logs/ILogger/OpenTelemetryLoggerProvider.cs 93.75% <92.85%> (-0.70%) ⬇️
...emetry/Logs/ILogger/DeferredOpenTelemetryLogger.cs 93.10% <93.10%> (ø)
src/OpenTelemetry/Logs/ILogger/NullScope.cs 0.00% <0.00%> (ø)

... and 6 files with indirect coverage changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Approval recommended

No unresolved blocking issues were identified, and regression coverage is included.

Review details
  • Files reviewed: 6/6 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

Comment thread src/OpenTelemetry/Logs/ILogger/DeferredOpenTelemetryLogger.cs Outdated
This was referenced Sep 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pkg:OpenTelemetry Issues related to OpenTelemetry NuGet package

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants