[CORELIB-75] DI injected logger#757
Conversation
test: Run #2139
🎉 All tests passed!🔄 This comment has been updated |
|
Let's also discuss this internally in a wider group. |
wojtek2288
force-pushed
the
feature/di-injected-logger
branch
from
3534e20 to
9b3d38a
Compare
jakubfijalkowski
left a comment
jakubfijalkowski
left a comment
There was a problem hiding this comment.
A couple of changes are needed:
- We must not start
Hostin tests that just configureServiceCollection. This has much more consequences (it really starts a host). - We need to split how we configure the logger, because currently it is very awkward (esp. for tests). We need to leave
ConfigureDefaultLoggeras-is, but we need to introduceAddLeanCodeLoggerthat configures theILogger<T>factory (+ Serilog). - We need
NullLogger<T> - We probably should split the
LeanCode.Loggingpackage into two - one that has the interface (+NullLogger<T>), and the other that has the configuration methods. Now, we have quite unexpected dependencies from e.g. from LeanCode.CQRS.Seciruty to Serilog.Sinks.Console.
| private readonly RequestDelegate next; | ||
|
|
||
| public CQRSMiddleware(CQRSMetrics metrics, ISerializer serializer, RequestDelegate next) | ||
| public CQRSMiddleware( |
There was a problem hiding this comment.
One last (not priority, not mandatory) request - let's put the logger as a last parameter (I think this is also a convention in the .NET examples, but might be wrong here).
| var logger = loggerConfiguration.CreateLogger(); | ||
|
|
||
| if (preserveStaticLogger) | ||
| { | ||
| Log.Logger = logger; | ||
| } | ||
|
|
||
| logging.Services.AddSingleton<Serilog.ILogger>(logger); | ||
| logging.Services.AddSingleton(typeof(ILogger<>), typeof(Logger<>)); | ||
|
|
||
| logging.AddConfiguration(configuration.GetSection(SystemLoggersEntryName)); | ||
| logging.AddSerilog(); | ||
| logging.AddConfiguration(context.Configuration.GetSection(SystemLoggersEntryName)); | ||
| logging.AddSerilog(logger, dispose: true); |
There was a problem hiding this comment.
I think I know why we haven't used AddSerilog in the ConfigureServices, but let's be sure - why?
| serviceCollection.AddScoped<LocalHandlerMiddleware>(); | ||
| serviceCollection.AddScoped<CQRSMetrics>(); | ||
| serviceCollection.AddScoped<ICommandValidatorResolver, CommandValidatorResolver>(); | ||
| var hostBuilder = Host.CreateDefaultBuilder() |
There was a problem hiding this comment.
This is wrong. We don't want to start host here, in any way.
|
|
||
| public StubAuditLogStorage() | ||
| { | ||
| logger = Substitute.For<Logging.ILogger<StubAuditLogStorage>>(); |
There was a problem hiding this comment.
We probably should have a NullLogger<T> implementation. Using NSubstitute for this gives a wrong impression that it is used anyhow.
| private static string GetLogOutput(Action log) | ||
| { | ||
| using var stringWriter = new StringWriter(); | ||
|
|
||
| Console.SetOut(stringWriter); | ||
|
|
||
| log(); | ||
|
|
||
| return stringWriter.ToString(); | ||
| } |
There was a problem hiding this comment.
This will make the test output broken. We can't override console stream.
|
Superseded by #775 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## v9.0-preview #757 +/- ##
===================================
===================================
☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
4 participants
No description provided.