Switch to Microsoft.Extensions.Logging#4247
Conversation
roji
force-pushed
the
Logging
branch
2 times, most recently
from
6e070e5 to
8fbc412
Compare
| if (process.ExitCode != 0) | ||
| { | ||
| Log.Debug($"klist exited with code {process.ExitCode}: {process.StandardError.ReadToEnd()}"); | ||
| Logger.LogDebug($"klist exited with code {process.ExitCode}: {process.StandardError.ReadToEnd()}"); |
There was a problem hiding this comment.
You missed several strings using interpolating formatting. Was that intentional?
There was a problem hiding this comment.
You're right - opened #4277 to track cleaning this up.
| Debug.Assert(State != ConnectorState.Ready, "Forgot to start a user action..."); | ||
|
|
||
| Log.Trace("Executing internal pregenerated command", Id); | ||
| Logger.LogTrace("Executing internal pregenerated command", Id); |
There was a problem hiding this comment.
Some strings in this PR have arguments but not corresponding parameter in the template.
There was a problem hiding this comment.
That's intentional - the idea is to report certain structured fields but without necessarily including them in the message (see dotnet/runtime#35995), since it's not always appropriate to pack all possible data into that string. The connection ID is a good example - I'm not sure we should overload textual output for every single message with the ID (because in many/most cases that doesn't matter), but the data should be there in case someone wants to filter by a specific connection id, for example.
Any thoughts on this? Do you think it's problematic practice?
There was a problem hiding this comment.
All good, wanted to check if it was intentional.
3 participants
This is a first phase of work on our logging implementation - it switches us to use Microsoft.Extensions.Logging.
Closes #2103