Add 'Analytic Summary' & PerformanceRecorderEngine to Quiet Background Processes feature #2596
Conversation
dhoehna
requested review from
a team,
EricJohnson327,
SohamDas2021,
bbonaby,
brialmsft,
dkbennett,
huzaifa-d,
jaholme,
krschau,
nieubank,
sshilov7,
ssparach and
vineeththomasalex
There was a problem hiding this comment.
Probs can put DevHome.QuietBackgroundProcess.UI.ViewModels into a using statement, ya? #Resolved
There was a problem hiding this comment.
NIT: UInt64 unless process IDs can be negative. #Resolved
There was a problem hiding this comment.
Why Single instead of Double? #Resolved
...groundProcesses/DevHome.QuietBackgroundProcesses.Common/DevHome.QuietBackgroundProcesses.idl
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Windows.Foundation.TimeSpan instead of an int. This way Start can convert to microseconds. #Resolved
...Processes/DevHome.QuietBackgroundProcesses.ElevatedServer/PerformanceRecorderEngineWinrt.cpp
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
I believe DevHome does not support ARM. #Resolved
There was a problem hiding this comment.
i can't figure out why VS keeps adding this...
There was a problem hiding this comment.
Please use SDK version 22621. The SDK version was recently updated. #Resolved
...ses/DevHome.QuietBackgroundProcesses.PerformanceRecorderEngine/PerformanceRecorderEngine.cpp
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Bracket should be on a new line. #Resolved
...ses/DevHome.QuietBackgroundProcesses.PerformanceRecorderEngine/PerformanceRecorderEngine.cpp
Outdated
Show resolved
Hide resolved
...ses/DevHome.QuietBackgroundProcesses.PerformanceRecorderEngine/PerformanceRecorderEngine.cpp
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
DevHome.QuietBackgroundProcesses.UI.ProcessData can go into a using statement, ya? #Resolved
...QuietBackgroundProcesses/DevHome.QuietBackgroundProcesses.UI/Views/AnalyticSummaryPopup.xaml
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Does Sort By need localization? #Resolved
There was a problem hiding this comment.
Columns default to a width of * #Resolved
There was a problem hiding this comment.
You should make this a RelayCommand on the ViewModel and not have to route it through the View. #Resolved
There was a problem hiding this comment.
I'm having trouble finding the role of the view in this MVVM paradigm. Is it always supposed to be empty?
There was a problem hiding this comment.
It seems weird at first, but actually kind of yes -- the more you can get out of the View and into the ViewModel the better. I think of it as the ViewModel has everything the View needs to know, but the View could take all the information and lay it out totally differently. So, if the text of the Filter changes, you'd want to know and react to that no matter how the page is laid out.
In case you haven't seen it before, this is a really good in-depth paper about MVVM and I usually refer to it when I have questions: https://dotnet.microsoft.com/en-us/download/e-book/maui/pdf
...s/QuietBackgroundProcesses/DevHome.QuietBackgroundProcesses.UI/Views/SimpleTableControl.xaml
Outdated
Show resolved
Hide resolved
...QuietBackgroundProcesses/DevHome.QuietBackgroundProcesses.UI/Views/AnalyticSummaryPopup.xaml
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
so... the tracing traces itself :P (you can see its non-zero impact in its own output)
I'll bring it up with Bridget & Tyler.
There was a problem hiding this comment.
FWIW, on my VM it's about 0.25% of the CPU
…d Processes feature
* When starting a quiet session, also start a performance recording session
* Added Analytic Summary with process performance table (incl. filtering and sorting)
* Displays gathered metrics from processes running during a quiet session
* Add usage telemetry
* Updated icon & strings from Content UX session
jsidewhite
force-pushed
the
user/jwhites/analytic_summary_pr3
branch
from
f35affb to
c822338
Compare
| } | ||
| catch (Exception ex) | ||
| { | ||
| _log.Error("Error populating performance summary table", ex); |
There was a problem hiding this comment.
If there's an exception, a user will see the table up to that point, yeah? Should we show an error instead? Or should we put the try/catch inside the loop so they can get more of the table? If we want to do either of these it can come in as a separate change. #WontFix
There was a problem hiding this comment.
The table will be empty - we never populate ProcessDatasAd. I think showing the user an error is a good idea - but nobody can tell me how to surface errors to the user in Dev Home.
| public void SortProcessesComboBoxChanged(string selectedValue) | ||
| { | ||
| ProcessDatasAd.SortDescriptions.Clear(); | ||
| if (selectedValue == "Process") |
There was a problem hiding this comment.
Better to use string.Equals #Resolved
| ProcessDatasAd.SortDescriptions.Clear(); | ||
| if (selectedValue == "Process") | ||
| { | ||
| ProcessDatasAd.SortDescriptions.Add(new SortDescription("Name", SortDirection.Ascending)); |
There was a problem hiding this comment.
Would it be better/safer to have these Name/Category/TimeAboveThreshold strings be an enum? #Resolved
| <Grid.RowDefinitions> | ||
| <RowDefinition /> | ||
| </Grid.RowDefinitions> |
There was a problem hiding this comment.
You don't need this.
| <Grid.RowDefinitions> | |
| <RowDefinition /> | |
| </Grid.RowDefinitions> | |
| ``` #Resolved |
| <ListView.Header> | ||
| <Grid ColumnSpacing="0"> | ||
| <Grid.RowDefinitions> | ||
| <RowDefinition Height="40"/> |
There was a problem hiding this comment.
At some point you should go through this with System Text scaling up to 200%. You'll probably want to hard-code less and/or make some things that are currently Height/Width => MinHeight/MinWidth, to prevent text getting cut off and accessibility bugs. #WontFix
There was a problem hiding this comment.
I got super stumped - everything started with * and Auto, but devolved to hardcoded values over days of fumbling around. Need to learn the magics.
There was a problem hiding this comment.
We should definitely address in the next pass.
Because this isn't part of the main Dev Home window, but rather limited to the pop up dialog, going to suggest WontFix (and feel guilty about it)
| HorizontalAlignment="Left" | ||
| HorizontalTextAlignment="Left" | ||
| TextTrimming="CharacterEllipsis" | ||
| Padding="11 11 11 11" /> |
There was a problem hiding this comment.
Super nit/personal preference, but sometimes here it's just simpler to say "Padding="11"" (if they're all the same, you can just use one) #Resolved
| { | ||
| var analyticSummaryPopup = new AnalyticSummaryPopup(ViewModel.GetProcessPerformanceTable()); | ||
| analyticSummaryPopup.XamlRoot = this.Content.XamlRoot; | ||
| analyticSummaryPopup.RequestedTheme = this.ActualTheme; |
There was a problem hiding this comment.
Doing this will prevent the theme from updating if you were to have this open and change the system theme.
Does this work?
| analyticSummaryPopup.RequestedTheme = this.ActualTheme; | |
| analyticSummaryPopup.RequestedTheme = this.RequestedTheme; |
That way "Default" could be passed through instead of only Dark or Light. #ByDesign
There was a problem hiding this comment.
If i change to RequestedTheme, then I get a mix of Dark & Light UI. ActualTheme seems to keep it consistent.
There was a problem hiding this comment.
Probably indicates other issues. Keep it for now but consider trying to fix it in the future.
There was a problem hiding this comment.
sticking with ActualTheme after discussing behavior in side chat
Summary of the pull request
Add 'Analytic Summary' & PerformanceRecorderEngine to Quiet Background Processes feature
When starting a quiet session, also start a performance recording session
Added Analytic Summary with process performance table (incl. filtering and sorting)
Add usage telemetry
Updated icon & strings from Content UX session
References and relevant issues
Detailed description of the pull request / Additional comments
Validation steps performed
PR checklist