You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This PR is an attempt at replicating #321 with some firm isolation between existing logging machinery and any new logging machinery. It has some opinions about where and how frequently we should log things (in batch, after consolidation), and it uses the abstraction of #352 to allow timestamps to be recovered via dyn Any downcasting.
cc @utaal for any thoughts, but I should be able to move this forward even without any particular comments. I think if I extend it to capture the topology as well as the events it pretty closely resembles the intent of #321.
This looks great! Glad the dyn Any abstraction is working out here too. I think one delta from #321 is that this doesn't log after every step, but that was probably unnecessary except for our specific use (which we can just replicate with a standalone, more instrumented Tracker).
Topology capture was, I believe, essentially just Summaryies and timestamp type information: do you think you'd add those to the other new progress logging events or here directly?
Sweet. I think I'll land it. The topology information is a bit harder to capture in that T::Summary isn't as trivially packed into the timestamp vector trait. I think it could be put behind the same sort of abstraction (perhaps the same one, if we removed "timestamp" from its name) but we'll need to add that in the future (no worries to do so, and you can grab the edge information out of vanilla timely logging, minus any T::Summary decorations).
Topology capture was, I believe, essentially just Summaryies and timestamp type information: do you think you'd add those to the other new progress logging events or here directly?
Probably the right thing to do is log them here. Like a CreateTopology(...) event that signifies the start, and perhaps a ShutDown event at the end so that folks can clean up. I stalled a bit on figuring out the right way to put a new trait in place for dyn stuff, and wanted to try it out to see how well what was here worked for some use cases.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR is an attempt at replicating #321 with some firm isolation between existing logging machinery and any new logging machinery. It has some opinions about where and how frequently we should log things (in batch, after consolidation), and it uses the abstraction of #352 to allow timestamps to be recovered via
dyn Anydowncasting.cc @utaal for any thoughts, but I should be able to move this forward even without any particular comments. I think if I extend it to capture the topology as well as the events it pretty closely resembles the intent of #321.