Implement custom pocket logging formatters for commands and events. #2996
Conversation
| @@ -135,7 +132,8 @@ private void Run(ScheduledOperation operation) | |||
| { | |||
| _currentTopLevelOperation ??= operation.Value; | |||
|
|
|||
| using var logOp = Log.OnEnterAndConfirmOnExit($"Run: {operation.Value}"); | |||
| using var logOp = Log.OnEnterAndConfirmOnExit(); | |||
| logOp.Info("{0}", operation.Value); | |||
There was a problem hiding this comment.
It would be great if we could say logOp.Infi(operation.Value) instead. This would need a change in PocketLogger to make the message template string optional.
There was a problem hiding this comment.
Adding ability to pass argument(s) in OnEnterAndConfrimOnExit would also make the above case simpler. We could then just say Log.OnEnterAndConfirmOnExit(operation.Value).
shyamnamboodiripad
force-pushed
the
improve-logging
branch
2 times, most recently
from
63e6406
to
5515946
Compare
| @@ -60,4 +62,14 @@ private static IReadOnlyCollection<Diagnostic> RemapDiagnosticsFromLanguageNode( | |||
| ) | |||
| ).ToImmutableList(); | |||
| } | |||
|
|
|||
| internal static void StampRoutingSlipAndLog(this KernelEvent @event, Uri uri) | |||
There was a problem hiding this comment.
Why not just add the log call into the Stamp* methods? Does it get too verbose?
There was a problem hiding this comment.
The Stamp* methods don't have access to the event /command. So we need to add it to the caller of those methods.
Unless you think its ok to change the public Stamp* methods to pass in the event / command... But even then, we only want to log when routing slip is stamped inside the tool process - but not when it is stamped within the (de)serializer.
There was a problem hiding this comment.
The deserialization process should not call Stamp. If it still does that we should fix it.
There was a problem hiding this comment.
It does currently. See https://github.com/dotnet/interactive/blob/main/src/Microsoft.DotNet.Interactive/Connection/KernelCommandEnvelope.cs#L186.
To fix this, I guess RoutingSlip should have a constructor that accepts all deserialized uris (and tags), and we need a KernelCommand.SetRoutingSlip (similar to KernelCommand.SetToken) to initialize KernelCommand.RoutingSlip post deserialization.
Is this what you were implying? If yes, I can look into that in a follow up PR.
There was a problem hiding this comment.
Yeah, that sounds right.
shyamnamboodiripad
force-pushed
the
improve-logging
branch
from
5515946
to
ad4eb81
Compare
shyamnamboodiripad
force-pushed
the
improve-logging
branch
from
ad4eb81
to
da04a1a
Compare
Also remove the #!log magic command, improve logging of routing slips and other random improvements around logging.
shyamnamboodiripad
force-pushed
the
improve-logging
branch
from
da04a1a
to
5bbe1e3
Compare
Also remove the #!log magic command, improve logging of routing slips and other random improvements around logging.