DI-injected logger, attempt 2#775
Conversation
test: Run #2170
🎉 All tests passed!🔄 This comment has been updated |
jakubfijalkowski
left a comment
jakubfijalkowski
left a comment
There was a problem hiding this comment.
About the Leancode.Logging.Loggers - LeanCode.Logging.AspNetCore, as the main use case is to integrate the LeanCode.Logging package with ASP.NET (analogous to LeanCode.CQRS.AspNetCore).
|
|
||
| public static ILoggingBuilder TryAddContextualLeanCodeLogger(this ILoggingBuilder builder) | ||
| { | ||
| builder.Services.TryAddSingleton(typeof(LeanCode.Logging.ILogger<>), typeof(ContextualLogger<>)); |
There was a problem hiding this comment.
Transient, not singleton - otherwise the loggers will stay instantiated even if the class using them will be long gone (think rarely invoked handlers).
There was a problem hiding this comment.
Wouldn't that cause DI validation to complain if we try to resolve this logger in a service registered as singleton?
And even if not, wouldn't Scoped be better than Transient? I don't see a reason to instanciate the logger for the same context multiple times per request.
There was a problem hiding this comment.
Good point, dunno how to tackle the singleton problem. :( Nevertheless, the main usecase is "quickly instantiate logger for a very short time", so would go with it either way (which also would work the same way as previously when it comes to GC/instantiation).
Ad. Scoped/Transient - yep, you're right. I forgot the nomenclature.
| return services.AddSingleton<IViewRenderer>(sp => new RazorViewRenderer( | ||
| config, | ||
| sp.GetRequiredService<ILogger<RazorViewRenderer>>() | ||
| )); |
There was a problem hiding this comment.
Indeed, but it was a small inconsistency I wanted to fix along the way.
| Assert.Equal("Three", evt.MessageTemplate.Text); | ||
| } | ||
|
|
||
| internal class SingleLogEventCapturerSink : ILogEventSink |
Co-authored-by: Krzysztof Bogacki <krzysztof.bogacki@leancode.pl>
Saancreed
force-pushed
the
feature/di-injected-logger-v2
branch
from
4fe8da4 to
8cd227a
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## v9.0-preview #775 +/- ##
===================================
===================================
☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
3 participants
Supersedes #757.
I'll probably squash all that in the end, but for now I kept my own commit separate for easier reviewing.
The idea is to have one slim package that references Serilog and has only interfaces, adapters and null logger, with the rest moved to a new package. Naming suggestions are welcome;
LeanCode.Logging.Loggersis kind of horrible.TODO: reorder constructor parameters to match the logger-is-last convention?