Skip to content

Conversation

frankmcsherry
Copy link
Member

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.

@utaal
Copy link
Member

utaal commented Mar 30, 2021

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?

@frankmcsherry
Copy link
Member Author

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).

@frankmcsherry
Copy link
Member Author

frankmcsherry commented Mar 31, 2021

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.

@github-actions github-actions bot mentioned this pull request Oct 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants