Put an ILogger seam over the Serilog pipeline - #629
Conversation
Framework code reaches logging through Serilog's static Log, which pins the project to one logger implementation and leaks Serilog types outward. Introduce Microsoft.Extensions.Logging.ILogger as the abstraction in front of it, keeping Serilog as the provider and the pipeline itself untouched. AddFalloutLogging configures the pipeline and registers the abstraction over it. It deliberately avoids services.AddLogging, which would install MEL's own filter pipeline with an Information default -- a second level authority that would drop trace and debug records before Serilog saw them, displacing Logging.LevelSwitch. BuildManager.Execute now owns a per-run composition root and feeds the resolved factory to a static facade on Logging, so the ~85 Log.* call sites and the static build engine are unchanged. The provider is declared outside the try so it survives into Finish(), but built inside it so a configuration failure still returns the same exit code as before. The seam is internal: it is framework foundation, not public surface yet. Nothing in the public API changes and no output changes. First of the additive PRs in Fallout-build#428.
|
Couldn't apply labels from the fork (no write access on this repo). Per the PR-creation flow this needs |
|
thanks for raising this PR, I'll have a look today. I applied those labels for you and approved the workflow run. |
ChrisonSimtian
left a comment
There was a problem hiding this comment.
Good PR. The reasoning is sound, the commit message and description are better than most of what lands here, and the tests are pointed at the right things. Two shape issues and one repo-rule miss below — all small, and I'd like them settled here rather than in PR 2, because 2–4 build directly on this.
What I verified
- The unbound-factory analysis is correct.
SerilogLoggerbindsLog.Loggerat construction whenlogger: nulland a category is supplied, andHost.WriteErrorsAndWarnings(src/Fallout.Build/Host.cs:96) reassignsLog.Loggerwithout restoring it — so leaving the factory unbound is genuinely necessary, not defensive. - Avoiding
services.AddLogging(...)is the right call, andThe_bridge_does_not_filter_below_informationis exactly the guard that keeps someone from "tidying" it back. DelegateDisposable.SetAndRestore(() => staticField, ...)matches the existing pattern in this same file (ExecutingTargetLogEventEnricher.SetTargetEventProperty) — idiomatic here.InternalsVisibleToclaim checks out:Fallout.Build,Fallout.Build.Specs, andFallout.Cliare all in the rootAssemblyInfo.cs.- Dispose ordering in the
finallyis right — the scope is restored before the provider that owns the factory is disposed. - Tests correctly join
ProcessGlobalStateCollectionand filter by marker, consistent withInMemorySinkSpecsand the other process-global specs.
One thing that is not yours: Log.CloseAndFlush() ends up closing the errors-and-warnings pipeline rather than the one holding the file sinks, because WriteErrorsAndWarnings swaps Log.Logger during Finish(). Pre-existing — it's what #454 (FT-9) is about. Called out only so it doesn't get attributed to this change later.
Also
docs/dependencies.md needs rows for the new packages — that file asks reviewers to call it out, so consider this the call-out. Microsoft.Extensions.DependencyInjection deserves a sentence of its own: Fallout.Build is consumer-facing, so every consumer now pulls the full container transitively. Sanctioned by #428 ("add refs to Fallout.Build"), just needs to be written down — the doc already makes the same complaint about the Azure packages.
Labels
Applied for you, and skip-changelog was the right instinct — nothing here is consumer-facing. When PR 3 lands the Spectre presenter, that one should be enhancement.
Happy to approve once 1 and 2 are addressed. Neither needs a redesign.
|
@phmatray just checking, did you intentionally open this PR and are you genuinely interested in contributing? Or was that your AI? Just wanna know if you'll actually read the code review or if we take it from here :-) |
|
@ChrisonSimtian Thanks for applying the labels and approving the workflow run, and glad FormCraft was useful to you. Happy it helped :-) To answer directly: yes, I opened this PR intentionally and I'm genuinely in. I found Fallout while digging through NUKE issues (I use NUKE on nearly all my repos) and I'm curious to see where this fork goes. The MCP integration idea in particular appeals to me a lot. It's driven through my own Claude skill kit, but I'm the one steering it. I'll read the code review and finish the work. Fire away. |
Review follow-ups on the ILogger seam. AddFalloutLogging no longer calls Logging.Configure. It registers only, so any container can call it any number of times. Configure is not idempotent: it reassigns Serilog's Log.Logger on every call, and with no build it installs a pipeline with no file sinks, no host sink and no filter. A second caller would have wiped out the pipeline the first one was using. BuildManager.Execute now runs Configure explicitly, just before it builds the provider. ILogger<T> and ILogger are registered transient instead of singleton. Logger<T> binds its inner logger in its constructor, so a singleton pinned every consumer to whichever pipeline was current at the first resolution. That is the same failure Logging.Logger stays uncached to avoid. Transient does not rescue a component that holds a logger across a swap, so the remaining constraint is documented at the registration and on CreateSerilogLoggerFactory. Three specs cover the two changes. Each one fails if its change is reverted. Also: - docs/dependencies.md gains rows for Serilog.Extensions.Logging, Microsoft.Extensions.Logging.Abstractions and Microsoft.Extensions.DependencyInjection. The DI row notes that Fallout.Build is consumer-facing, so every consumer now pulls the container transitively. - StubLoggerFactory.CreateLogger returns NullLogger.Instance instead of throwing. It owns the process-wide Logging.Factory while installed, and Fallout-build#428 moves framework code onto Logging.Logger, so a throwing stub would become an intermittent failure source. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@ChrisonSimtian Thanks for the review. All three points are addressed in 9dfdf33, with replies in each thread. The branch is also synced with
Also took CodeRabbit's nitpick: Verification
Three specs were added, one per behaviour. I checked each one fails when its change is reverted, so they guard rather than just pass:
One thing worth your callOn point 2, transient fixes the resolution, not the holding. A component that resolves a logger once and keeps it across the Noted on the |
Summary
Puts
Microsoft.Extensions.Logging.ILoggerin front of Serilog, so framework code stops referencing Serilog directly. Serilog stays the provider and the pipeline inLogging.Configureis untouched — no behaviour change and no public-API change.Part of #428 — first of that issue's four additive PRs. Theme decoupling, the Spectre presenter, the
[Obsolete]/[Experimental]markers, and the breaking removals are all out of scope here.Directory.Packages.props+ Serilog.Extensions.Logging,+ Microsoft.Extensions.Logging.Abstractionssrc/Fallout.Build/Fallout.Build.csprojPackageReferences for those two plusMicrosoft.Extensions.DependencyInjectionLogging.DependencyInjection.cs(new)AddFalloutLogging— configures the Serilog pipeline, registersILoggerFactory/ILogger<>/ILoggerover itLogging.csFactory,Logger,UseLoggerFactoryExecution/BuildManager.csLogging.Configure(build)tests/…/LoggerBridgeSpecs.cs(new)Decisions worth reviewing
Not
services.AddLogging(...). That installs MEL's own filter pipeline, default minimumInformation— a second level authority that would drop trace and debug records before Serilog saw them and displaceLogging.LevelSwitch. Registering the Serilog factory directly leaves the level switch as the only gate.The_bridge_does_not_filter_below_informationis the regression guard.The factory is left unbound (
SerilogLoggerFactory(logger: null, dispose: false)), becauseLog.Loggeris not stable for the process lifetime —Configureinstalls it late andHost.WriteErrorsAndWarningsswaps it again for the end-of-build summary. Binding still happens once per logger rather than per write, since the category is attached asSourceContextat construction. Two consequences the code depends on, both documented at the call sites:AddFalloutLoggingconfigures the pipeline before registering the factory, so a container-resolved logger can never bind a stale one; andLogging.Loggeris deliberately uncached.Internal, not public. This is framework foundation, not public surface yet — consistent with the "internal foundation" note in
AGENTS.md. The rootAssemblyInfo.csalready grantsInternalsVisibleTotoFallout.Cliand the spec assemblies, which covers PR 4's CLI wiring.Façade over DI, per the issue's decision: the ~85
Log.*call sites and the staticBuildManager.Execute<T>are unchanged.Provider lifetime. The
ServiceProvideris declared outside thetryso it survives intofinally(Finish()still writes the outcome summary), but constructed inside it so a configuration failure returns the same exit code as before.Test plan
dotnet build fallout.slnx— 0 errors; the 42 warnings are pre-existing and none are in touched filesdotnet test fallout.slnx— 830 passed, 7 skipped, 0 failedTrace→Verbose…Critical→Fatal), no sub-Informationfiltering, level-switch gating, message templates staying templates, exceptions reachingLogEvent.Exception, factory not pinned to one pipeline, façade fallback with no container,UseLoggerFactoryrestore./build.ps1 Compile— exit 0, file sinks and rolling cleanup still writing.fallout/temp/build.log,OnBuildFinishedextensions still firing after the dispose reordering, console theming unchangedgit diffreviewed — no public API member added or changedThe bridge specs write through the process-global
Log.Logger, so every message carries a marker and collected events are filtered to it; without that, a concurrent spec class's warning lands in the sink and fails an assertion.Note on labels
Labelled
skip-changelograther than a category: nothing here is consumer-facing. Happy to switch it toenhancementif you'd rather the #428 work show up in the notes as it lands.Generated with Claude Code