Cancelation and scheduling improvements#2915
Conversation
f56f96c to
ce31f37
Compare
| return _id; | ||
| } | ||
|
|
||
| public bool Equals(KernelCommand other) |
There was a problem hiding this comment.
consider: It may be a good idea to override object.Equals as well (and operators == and !=)
| return GetOrCreateId() == other.GetOrCreateId(); | ||
| } | ||
|
|
||
| public override int GetHashCode() |
There was a problem hiding this comment.
So, I am looking at adding more logging and it looks like the logging for command objects currently relies on each command type overriding ToString. I was planning to override ToString on the base KernelCommand to include the token, and then update each derived ToString to call the base implementation (in addition to returning derived-specific strings)...
I would have preferred if we didn't need to touch every command type to add this logging - I have a concern that this may not scale well. For example, if we miss the call to base.ToString() when implementing ToString for some new command(s) in the future, this could make it difficult to debug some problem in already shipped bits where the info is missing...
Another alternative would be to log detailed info about the command (including token) in a separate message. Perhaps I could do that at the same time when we stamp the routing slip (which as we discussed would also be a good place to log the routing slip info).
However, not including the token in the command.ToString() would also mean that we may miss it in some places where having that info may be useful (such as when logging the command associated with an event). So, there are pros and cons...
Any thoughts on which approach may work better @jonsequitur ?
There was a problem hiding this comment.
I guess we could also introduce a new virtual ToDisplayString method to log the derived-specific info and then only override ToString in the base type. The base ToString could then call ToDisplayString and also log the command token to ensure that the token info is always logged whenever a command object is logged. One downside is that derived command implementations should remember to only override ToDisplayString (and leave ToString untouched) to get the desired logging behavior - but that doesn't seem too bad...
There was a problem hiding this comment.
We could have dedicated formatters for logging.
The .NET Interactive formatter infrastructure will let us define a single formatter for all current and future implementations of KernelCommand in one place. ToString is a fallback so we shouldn't modify it for the sake of logging.
There was a problem hiding this comment.
Ah thanks I hadn't considered that. I guess we could just log $"Run: {value.ToDisplayString("text/logging")}" inside KernelScheduler (instead of logging $"Run: {value}" as we do currently). And the registration could look something like -
Formatter.Register(
formatter: obj =>
{
if (obj is KernelCommand command)
{
var prefix = $"{command.GetType().Name} ({command.GetOrCreateToken()})";
if (obj is SubmitCode submitCode)
{
return $"{prefix} {submitCode.Code}";
}
...
// Handle other command types that need custom logging
...
}
if (obj is KernelEvent @event)
{
...
// Handle logging for events
...
}
return obj.ToString();
},
mimeType: "text/logging");
Does that match what you were thinking?
There was a problem hiding this comment.
Yeah, that looks great!
90cfc96 to
b41c9e0
Compare
b41c9e0 to
2ffd5cf
Compare
No description provided.