Skip to content

Put an ILogger seam over the Serilog pipeline #629

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

Open
phmatray wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

Put an ILogger seam over the Serilog pipeline #629

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fa22542
Put an ILogger seam over the Serilog pipeline
Aug 10, 2026
316349a
Merge remote-tracking branch 'upstream/develop' into feat/428-ilogger…
Aug 27, 2026
9dfdf33
Split logging registration from pipeline configuration
Aug 27, 2026
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
2 changes: 2 additions & 0 deletions Directory.Packages.props
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,13 @@
<PackageVersion Include="JetBrains.Annotations" Version="2026.2.0" />
<PackageVersion Include="Microsoft.Extensions.DependencyInjection" Version="10.0.0" />
<PackageVersion Include="Microsoft.Extensions.DependencyModel" Version="10.0.0" />
<PackageVersion Include="Microsoft.Extensions.Logging.Abstractions" Version="10.0.0" />
<PackageVersion Include="Nerdbank.GitVersioning" Version="3.7.115" />
<PackageVersion Include="Microsoft.SourceLink.GitHub" Version="8.0.0" />
<PackageVersion Include="NuGet.Packaging" Version="6.14.3" />
<PackageVersion Include="Octokit" Version="14.0.0" />
<PackageVersion Include="Serilog" Version="4.3.0" />
<PackageVersion Include="Serilog.Extensions.Logging" Version="10.0.0" />
<PackageVersion Include="Serilog.Formatting.Compact" Version="3.0.0" />
<PackageVersion Include="Serilog.Formatting.Compact.Reader" Version="4.0.0" />
<PackageVersion Include="Serilog.Sinks.Console" Version="6.1.1" />
Expand Down
13 changes: 12 additions & 1 deletion src/Fallout.Build/Execution/BuildManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,9 @@
using System.Linq.Expressions;
using System.Runtime.CompilerServices;
using System.Text;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.DependencyModel;
using Microsoft.Extensions.Logging;
using Fallout.Common.Tooling;
using Fallout.Common.Utilities;
using Fallout.Common.Utilities.Collections;
Expand Down Expand Up @@ -44,9 +46,16 @@ public static int Execute<T>(Expression<Func<T, Target>>[] defaultTargetExpressi
using var context = BuildContext.Activate();
var build = new T();

// The composition root for the run. Declared out here so it survives into `finally` —
// Finish() still writes the outcome summary — but built inside the `try`, so a failure while
// configuring logging is reported the same way it was before there was a container.
ServiceProvider services = null;
IDisposable loggerFactoryScope = null;

try
{
Logging.Configure(build);
services = new ServiceCollection().AddFalloutLogging(build).BuildServiceProvider();
loggerFactoryScope = Logging.UseLoggerFactory(services.GetRequiredService<ILoggerFactory>());

build.ExecutableTargets = ExecutableTargetFactory.CreateAll(build, defaultTargetExpressions);
build.ExecuteExtension<IOnBuildCreated>(x => x.OnBuildCreated(build.ExecutableTargets));
Expand Down Expand Up @@ -89,6 +98,8 @@ public static int Execute<T>(Expression<Func<T, Target>>[] defaultTargetExpressi
{
Finish();
Log.CloseAndFlush();
loggerFactoryScope?.Dispose();
services?.Dispose();
// Per-run teardown (handler unsubscription + state reset) is owned by the BuildContext,
// run when `context` is disposed at method exit.
}
Expand Down
3 changes: 3 additions & 0 deletions src/Fallout.Build/Fallout.Build.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,10 @@
</ItemGroup>

<ItemGroup>
<PackageReference Include="Microsoft.Extensions.DependencyInjection" />

@ChrisonSimtian ChrisonSimtian Aug 13, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

New transitive dependency on the full MS.DI container for every Fallout.Build consumer. Fine by me — #428 calls for it explicitly — but it needs a row in docs/dependencies.md along with Serilog.Extensions.Logging and Microsoft.Extensions.Logging.Abstractions.

Worth a note on the DI row that the container currently serves one internal composition root, so the footprint is deliberate rather than accidental. The Logging section is the natural home for the two Serilog/MEL entries.

@phmatray phmatray Aug 27, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Added in 9dfdf33. Serilog.Extensions.Logging and Microsoft.Extensions.Logging.Abstractions went into the Logging section, as you suggested.

Microsoft.Extensions.DependencyInjection went into the Microsoft / .NET BCL table, with the footprint note in the Used by column:

Fallout.Build is consumer-facing, so every consumer now pulls the container transitively. Deliberate, not accidental. It currently serves one internal composition root, and the plugin foundation (milestone #6) is what will use it more widely.

<PackageReference Include="Microsoft.Extensions.DependencyModel" />
<PackageReference Include="Microsoft.Extensions.Logging.Abstractions" />
<PackageReference Include="Serilog.Extensions.Logging" />
<PackageReference Include="Serilog.Formatting.Compact" />
<PackageReference Include="Serilog.Formatting.Compact.Reader" />
<PackageReference Include="Serilog.Sinks.Console" />
Expand Down
44 changes: 44 additions & 0 deletions src/Fallout.Build/Logging.DependencyInjection.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
using System;
using System.Linq;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.DependencyInjection.Extensions;
using Microsoft.Extensions.Logging;

namespace Fallout.Common.Execution;

/// <summary>
/// Composition root for the logging seam. Serilog stays the provider — this only puts
/// <see cref="ILogger"/> in front of it so framework code can stop referencing Serilog directly.
/// </summary>
/// <remarks>
/// Internal on purpose: the abstraction is the framework's own foundation, not public surface yet.
/// The root <c>AssemblyInfo.cs</c> grants <c>InternalsVisibleTo</c> to <c>Fallout.Cli</c> and the
/// spec assemblies, which is everything that needs to wire a container today.
/// </remarks>
internal static class LoggingServiceCollectionExtensions
{
/// <summary>
/// Configures the Serilog pipeline for <paramref name="build"/> and registers the
/// <see cref="ILogger"/> abstraction over it.
/// </summary>
/// <remarks>
/// Deliberately not <c>services.AddLogging(...)</c>. That installs Microsoft.Extensions.Logging's
/// own filter pipeline, whose default minimum is <see cref="LogLevel.Information"/> — a second
/// level authority that would silently drop trace and debug records before Serilog ever saw
/// them. Registering the Serilog factory directly leaves <see cref="Logging.LevelSwitch"/> as the
/// only thing deciding what gets logged.
/// </remarks>
public static IServiceCollection AddFalloutLogging(this IServiceCollection services, IFalloutBuild build = null)
{
Logging.Configure(build);

@ChrisonSimtian ChrisonSimtian Aug 13, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is the one I'd like changed before merge. AddFalloutLogging mutates process-global state from inside a DI registration method.

TryAdd* is idempotent; Logging.Configure is not. It reassigns Log.Logger unconditionally, every call.

That's harmless today because BuildManager is the only caller. But #428's PR 4 plans to "wire the CLI container to the same registrations" — and Fallout.Cli calling AddFalloutLogging() at container-build time runs Logging.Configure(null), which per the build == null early-returns throughout Logging.cs installs a pipeline with no file sinks, no host sink, and no filter. Whatever pipeline was there gets clobbered, and the already-registered factory won't notice.

Suggest splitting the two concerns:

// registration only — safe to call from any container, any number of times
public static IServiceCollection AddFalloutLogging(this IServiceCollection services)

and leaving Logging.Configure(build) explicit at the BuildManager.Execute call site, immediately before the container is built. Same ordering guarantee you document on CreateSerilogLoggerFactory (pipeline configured before the factory is registered), but it survives a second caller.

@phmatray phmatray Aug 27, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Done in 9dfdf33. AddFalloutLogging now registers only, and never touches the pipeline. BuildManager.Execute calls Logging.Configure(build) explicitly, immediately before it builds the provider:

Logging.Configure(build);
services = new ServiceCollection().AddFalloutLogging().BuildServiceProvider();

The IFalloutBuild parameter is gone from the signature, so PR 4 cannot reach the Configure(null) path you described by accident.

New spec Registering_the_seam_leaves_the_pipeline_alone installs a pipeline, calls AddFalloutLogging on a second container, then asserts Log.Logger is still the same instance. I checked it fails when the Configure call is put back inside the registration.


services.TryAddSingleton(_ => Logging.CreateSerilogLoggerFactory());

// Logger<T> is a thin wrapper that defers to ILoggerFactory, so it inherits the factory
// above rather than introducing a filter pipeline of its own.
services.TryAddSingleton(typeof(ILogger<>), typeof(Logger<>));
services.TryAddSingleton(sp => sp.GetRequiredService<ILoggerFactory>().CreateLogger(Logging.DefaultCategoryName));

@ChrisonSimtian ChrisonSimtian Aug 13, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

These two registrations are singletons, which quietly contradicts the invariant the rest of the PR is built on.

Logging.Logger is deliberately uncached — the XML doc explains at length that binding happens once per logger construction, so caching one would strand it on a stale pipeline. But Logger<T> resolves its inner logger once in its constructor, and the non-generic factory lambda runs once per container. Both pin to whatever Log.Logger was current at first resolution.

So a component that resolves ILogger<T> and holds it across the WriteErrorsAndWarnings swap writes into the pre-swap pipeline — the exact failure Logging.Logger is uncached to avoid.

Nothing resolves these yet, so it's latent. What makes it worth fixing now: PR 2 and 3 put IHostTheme and IHostOutput into this same container, and IHostOutput is the component that renders the end-of-build summary — i.e. the one holding an injected logger at precisely the moment the pipeline is swapped.

Either register them transient, or add a note here saying why the pinning is acceptable and what callers must not do. A spec pairing with The_facade_logger_tracks_the_current_pipeline — asserting the container-resolved logger behaves the same way — would lock in whichever answer you pick.

@phmatray phmatray Aug 27, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Agreed, and fixed in 9dfdf33. Both are transient now:

services.TryAddTransient(typeof(ILogger<>), typeof(Logger<>));
services.TryAddTransient(sp => sp.GetRequiredService<ILoggerFactory>().CreateLogger(Logging.DefaultCategoryName));

ILoggerFactory stays a singleton. It is left unbound, so it reads the ambient pipeline every time it creates a logger and pins nothing.

I took your first option and the second one, because transient alone does not close the hole you are pointing at. Each resolution now binds the current pipeline, but a component that resolves once and holds the logger across the WriteErrorsAndWarnings swap still writes into the old pipeline. That is inherent to constructor injection. So the residual constraint is written down at the registration and on CreateSerilogLoggerFactory: anything that outlives a swap must read Logging.Logger at the point of writing rather than hold an injected logger.

That is the rule IHostOutput will have to follow in PR 3. Happy to revisit if you would rather it get something swap-aware instead, but that felt like a design PR 2 and 3 should make, not this one.

Two specs, as you suggested, paired with The_facade_logger_tracks_the_current_pipeline:

  • A_container_logger_binds_the_pipeline_current_at_resolution resolves, swaps Log.Logger, resolves again, and asserts the write lands in the new sink.
  • The_container_hands_out_a_new_logger_per_resolution covers both registrations.

Both fail if the registrations go back to singleton. I checked.


return services;
}
}
63 changes: 63 additions & 0 deletions src/Fallout.Build/Logging.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,15 +10,27 @@
using Serilog;
using Serilog.Core;
using Serilog.Events;
using Serilog.Extensions.Logging;
using Serilog.Formatting.Compact;
using Serilog.Sinks.SystemConsole.Themes;

// Both Serilog and Microsoft.Extensions.Logging declare an ILogger. This file is the seam between
// them, so the unqualified name is bound to the abstraction the framework codes against; Serilog's
// own pipeline is reached through the static Log class below.
using ILogger = Microsoft.Extensions.Logging.ILogger;
using ILoggerFactory = Microsoft.Extensions.Logging.ILoggerFactory;

namespace Fallout.Common.Execution;

public static class Logging
{
public static readonly LoggingLevelSwitch LevelSwitch = new();

/// <summary>Category for framework log records written without a category of their own.</summary>
internal const string DefaultCategoryName = "Fallout";

private static ILoggerFactory loggerFactory;

internal static bool SupportsAnsiOutput => Environment.GetEnvironmentVariable("TERM") is { } term && term.StartsWithOrdinalIgnoreCase("xterm");
internal static IHostTheme DefaultTheme { get; } = SupportsAnsiOutput
? AnsiConsoleHostTheme.Default256AnsiColorTheme
Expand All @@ -36,6 +48,57 @@ public static LogLevel Level
set => LevelSwitch.MinimumLevel = value.ToLogEventLevel();
}

/// <summary>
/// Logger factory for the current build run, backed by the Serilog pipeline that
/// <see cref="Configure"/> installs. <c>BuildManager</c> feeds this from its composition root
/// (see <c>AddFalloutLogging</c>). Outside a run there is no container — the CLI commands call
/// <see cref="Configure"/> directly — so this falls back to a factory over the ambient Serilog
/// pipeline, and the seam is usable either way.
/// </summary>
internal static ILoggerFactory Factory => loggerFactory ??= CreateSerilogLoggerFactory();

/// <summary>
/// Logger for framework code that has no category of its own. Deliberately not cached — each
/// access creates a logger against the pipeline that is current right now, which is what keeps
/// the façade correct across the reassignments described on
/// <see cref="CreateSerilogLoggerFactory"/>.
/// </summary>
internal static ILogger Logger => Factory.CreateLogger(DefaultCategoryName);

/// <summary>
/// Points <see cref="Factory"/> at <paramref name="factory"/> until the returned bracket is
/// disposed. Ownership stays with the caller: disposing the bracket restores the previous
/// factory, it does not dispose <paramref name="factory"/>.
/// </summary>
internal static IDisposable UseLoggerFactory(ILoggerFactory factory)
{
return DelegateDisposable.SetAndRestore(() => loggerFactory, factory.NotNull());
}

/// <summary>
/// Bridges <see cref="ILogger"/> onto Serilog.
/// </summary>
/// <remarks>
/// Passing no logger leaves the factory itself unbound, so each logger it hands out reads the
/// ambient <see cref="Log.Logger"/> as it is created. That matters because the pipeline is not
/// stable for the lifetime of the process: <see cref="Configure"/> installs it late and
/// replaces it on re-entry, and <c>Host.WriteErrorsAndWarnings</c> swaps it again to render the
/// end-of-build summary. Pinning a logger into the factory would strand every consumer on
/// whichever pipeline happened to exist first.
///
/// Binding still happens per logger rather than per write, because the category name is
/// attached as Serilog's <c>SourceContext</c> at construction. Two consequences the callers
/// depend on: <c>AddFalloutLogging</c> configures the pipeline <em>before</em> it registers the
/// factory, so a container-resolved logger can never bind a stale one; and <see cref="Logger"/>
/// is not cached.
///
/// Serilog owns the pipeline's lifetime (<c>Log.CloseAndFlush</c>), hence <c>dispose: false</c>.
/// </remarks>
internal static ILoggerFactory CreateSerilogLoggerFactory()
{
return new SerilogLoggerFactory(logger: null, dispose: false);
}

public static void Configure(IFalloutBuild build = null)
{
if (build != null)
Expand Down
Loading
Loading