[CORELIB-75] DI injected logger #757
Conversation
test: Run #2123
🎉 All tests passed!🔄 This comment has been updated |
|
Let's also discuss this internally in a wider group. |
| @@ -40,7 +45,7 @@ public async Task InvokeAsync(HttpContext httpContext) | |||
| activity?.SetTag("error.code", ex.ErrorCode); | |||
|
|
|||
| var result = WrapInCommandResult(ex); | |||
| logger.Warning("Command {@Command} is not valid with result {@Result}", cqrsPayload.Payload, result); | |||
| logger.LogWarning("Command {@Command} is not valid with result {@Result}", cqrsPayload.Payload, result); | |||
There was a problem hiding this comment.
Is {@Command} syntax an extensions logging standard? I've always though it was a serilog goodie (chat gpt confirms it). So, I sb ever want's to change logger implementation I guess it'd break.
There was a problem hiding this comment.
Hm, right in some loggers ToString() might be called here. We probably need to change @Command to Command and pass JsonSerializer.Serialize(cqrsPayload.Payload) as an argument to make it work with any logger.
There was a problem hiding this comment.
I don't think this is a good approach. It will break the "sanitizer" logic that we have and we need to support that.
There was a problem hiding this comment.
Then I'm not sure if we can do anything about it right now other then making sanitization logger agnostic.
There was a problem hiding this comment.
So if we'd abstract this way from Serilog we'd lose this functionality, which would be sad. Is there a way to not abstract the Serilog, and still use the DI based loggers? If so, I'd say let's go with it (unless it isn't too ugly), and in the other case I'd stick with what we've done until now.
There was a problem hiding this comment.
If serilog does not provide anything out of the box, I think we can safely register Serilog.ILogger<T> as open generic and link it with DI
No description provided.