-
-
Notifications
You must be signed in to change notification settings - Fork 18
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.
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
Fallout-build:develop
Choose a base branch
from
phmatray:feat/428-ilogger-seam
base: develop
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,59 @@ | ||
| 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> | ||
| /// Registers the <see cref="ILogger"/> abstraction over the Serilog pipeline. | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// Registration only. This method never touches the pipeline, so any container can call it, any | ||
| /// number of times. Installing the pipeline is <see cref="Logging.Configure"/>, which the caller | ||
| /// runs explicitly just before it builds the provider. | ||
| /// | ||
| /// The two are kept apart because <see cref="Logging.Configure"/> is not idempotent. It | ||
| /// reassigns Serilog's <c>Log.Logger</c> on every call. Called with no build, it installs a | ||
| /// pipeline with no file sinks, no host sink and no filter. A second container calling this | ||
| /// method would then wipe out the pipeline the first one had set up. | ||
| /// | ||
| /// Deliberately not <c>services.AddLogging(...)</c>. That installs Microsoft.Extensions.Logging's | ||
| /// own filter pipeline, whose default minimum is <see cref="LogLevel.Information"/>. It would be | ||
| /// a second authority on levels, dropping trace and debug records before Serilog ever saw them. | ||
| /// Registering the Serilog factory directly leaves <see cref="Logging.LevelSwitch"/> as the only | ||
| /// thing that decides what gets logged. | ||
| /// </remarks> | ||
| public static IServiceCollection AddFalloutLogging(this IServiceCollection services) | ||
| { | ||
| // Safe as a singleton because the factory is left unbound: it reads the ambient pipeline | ||
| // every time it creates a logger. See Logging.CreateSerilogLoggerFactory. | ||
| services.TryAddSingleton(_ => Logging.CreateSerilogLoggerFactory()); | ||
|
|
||
| // Transient, not singleton. Logger<T> resolves its inner logger in its constructor, and the | ||
| // non-generic lambda would run once per container. As singletons, both would pin every | ||
| // consumer to whichever pipeline was current at the first resolution. Log.Logger does not | ||
| // stay put during a run: Configure installs it late, and Host.WriteErrorsAndWarnings swaps | ||
| // it again to render the end-of-build summary. | ||
| // | ||
| // Transient means each resolution binds to the pipeline that is current right now. It does | ||
| // not rescue a component that resolves a logger once and holds it across a swap. Any | ||
| // component that outlives a swap must read Logging.Logger at the point of writing instead. | ||
| services.TryAddTransient(typeof(ILogger<>), typeof(Logger<>)); | ||
| services.TryAddTransient(sp => sp.GetRequiredService<ILoggerFactory>().CreateLogger(Logging.DefaultCategoryName)); | ||
|
|
||
| return services; | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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.Buildconsumer. Fine by me — #428 calls for it explicitly — but it needs a row indocs/dependencies.mdalong withSerilog.Extensions.LoggingandMicrosoft.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.
There was a problem hiding this comment.
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.LoggingandMicrosoft.Extensions.Logging.Abstractionswent into the Logging section, as you suggested.Microsoft.Extensions.DependencyInjectionwent into the Microsoft / .NET BCL table, with the footprint note in theUsed bycolumn: