Add support for showing stacks from selected events - #2418
Add support for showing stacks from selected events#2418Paul Dorn (paulusakademius) wants to merge 1 commit into
Conversation
- Before this commit you could only show stacks for either a single time or a time range - This commit makes selecting any amount of events (no matterer what column of the event) and opening its stack (if there is one) possible - The stacks shown are now not only filtered by the time but also event id so no other events that happened at the same time are shown. - The behavior for selecting a time range remains the same as before
|
I thought this might be the most intuitive solution because it gives you the stacks for the events you selected. I kept the functionality for selecting a time range as it is, but when I noticed it only filters by time and it is possible for other events to happen at the same time, I thought maybe you could filter there for the event id too, like I did in the feature I implemented. What do you think about this, I think it might be more intuitive for the selection in the EventViewer. I am looking forward to hear your opinion on the idea and the code I already wrote. |
|
Brian Robbins (@brianrob) Hi Brian, have you had any chance to look at this? If there are any issues let us know. |
|
Thank you for pinging me on this one. I had not looked yet - I am looking now. With my initial testing, it looks great. I'm reviewing the code now and will get back to you on that. |
Brian Robbins (brianrob)
left a comment
Brian Robbins (brianrob)
left a comment
There was a problem hiding this comment.
Thank you very much for contributing this feature. I tried it out and it's slick.
I've added some comments and questions. It would also be great to have some additional test coverage for this UI feature to make sure that we keep it functional as changes are made in the future. The PerfView tests have a good pattern for implementing UI tests and AI agents are pretty good at picking it up.
| if (selectedCells.Count != 1) | ||
| { | ||
| StatusBar.LogError("Could not parse " + start + " as a number."); | ||
| OpenSelectedStacks(stackSourceName, selectedCells); |
There was a problem hiding this comment.
Previous behavior was that if no cells were selected, the views would still open and populate. Now, they populate with no data. It would be good to preserve this behavior.
|
|
||
| // For ETW sources, filter by exact EventIndex so concurrent events on other | ||
| // threads at the same timestamp are excluded. | ||
| var etwRecords = uniqueRecords.OfType<ETWEventSource.ETWEventRecord>().ToList(); |
There was a problem hiding this comment.
Another potential type that could be held in uniqueRecords is CsvEventRecord. In that case, this will produce an empty list and Min and Max will throw. Will need to handle that here.
| } | ||
| } | ||
| this.StatusBar.LogError("Could not parse " + end + " as a number."); | ||
| this.OpenSelectedStacks(stackSourceName, selectedCells); |
There was a problem hiding this comment.
The previous behavior was that if nothing was selected then the views would populate with all data. Now, the views will come up but with no data. We should preserve the previous behavior.
| // TODO not clear I want this method | ||
| public virtual StackSource GetStackSource(TextWriter log, double startRelativeMSec, double endRelativeMSec, Predicate<TraceEvent> predicate) | ||
| { | ||
| StackSource ret = DataFile.OpenStackSourceImpl(SourceName, log, startRelativeMSec, endRelativeMSec, predicate); |
There was a problem hiding this comment.
If a predicate is specified, I think we ought to be using it. It might toss all of the events, but if it does, then we shouldn't fall back. Is there a specific reason for the fallback behavior?
| } | ||
|
|
||
| // Collect unique event records (a row may have multiple selected cells). | ||
| var uniqueRecords = selectedCells |
There was a problem hiding this comment.
selectedCells is a reference to Grid.SelectedCells. You'll need to copy this collection before the call to StatusBar.StartWork to ensure that the collection doesn't change underneath you, since the UI is still fluid.
| else | ||
| { | ||
| // Fall back to per-event time windows for non-ETW sources. | ||
| var sources = new List<StackSource>(); |
There was a problem hiding this comment.
I think a reasonable fallback here is to find the min and max and just make a single call that filters the data. I worry that this could be relatively large set of selected fields and we'll be doing lots of work unintentionally.
3 participants
This pull request introduces the ability to show stacks for selected events with the 'Open Any Stacks' function, regardless of the column of the event selected. Previously, stacks were only accessible for either a single time or a time range and defaulted to the everything when some other selection happened. With this update:
Issue #2394