Hide the user interface components showing stacks for tracks that don't have stack samples#4133
Conversation
Codecov Report
@@ Coverage Diff @@
## main #4133 +/- ##
==========================================
- Coverage 88.27% 88.27% -0.01%
==========================================
Files 280 280
Lines 24388 24410 +22
Branches 6477 6496 +19
==========================================
+ Hits 21529 21548 +19
- Misses 2655 2658 +3
Partials 204 204
Continue to review full report at Codecov.
|
canova
left a comment
canova
left a comment
There was a problem hiding this comment.
Thanks for the PR Florian and sorry for the late review. This PR somehow slipped through the cracks.
Looks pretty good to me! Thanks for hiding these unnecessary tabs and UI elements when they are not useful. I've added some small nits, we can land it after these small changes.
| threadSelectors.getJsTracerTable, | ||
| ({ processType }, isNetworkChartEmpty, jsTracerTable) => { | ||
| ( | ||
| { processType, samples, stackTable, stringTable, frameTable, funcTable }, |
There was a problem hiding this comment.
nit: It could be better to do this destruction below inside the function. I think it's easy to understand when we only destruct one or two fields here, but when it gets crowded, it becomes a bit hard to read. Instead we can do
const { processType, samples, stackTable, stringTable, frameTable, funcTable } = thread;
below.
| if (stringTable.getString(stringIndex) === '(root)') { | ||
| // If the first sample's stack is only the root, check if any other | ||
| // sample is different. | ||
| hasSamples = samples.stack.some((s) => s !== stackIndex); |
There was a problem hiding this comment.
Hm, this looks a bit costly, but probably it's going to return early after two iterations for most of our profiles. And not sure how else we can do it without looping. I think it's okay to keep like this.
There was a problem hiding this comment.
it could be faster to:
- get the string index for (root) outside of the loop
- compare string indexes here
There was a problem hiding this comment.
The .some call (assuming that's what is being called 'loop' in these comments) should only be done for profiles where the first sample contains a single frame, and then we only compare one integer with an array of integers. That should be fast enough, and we will only compare the entire array of integers (one per sample) when the profile was captured with nostacksampling and all the sample contain only "(root)".
| if (!hasSamples) { | ||
| visibleTabs = visibleTabs.filter( | ||
| (tabSlug) => | ||
| !['calltree', 'flame-graph', 'stack-chart'].includes(tabSlug) |
There was a problem hiding this comment.
Could you move this array to app-logic/tab-handling.js file so we can keep them in sync in case we add another tab that shows samples related visualization? Maybe we can name it something like tabsShowingSampleData or sampleDataTabs or something else?
| rangeFilteredThread | ||
| ); | ||
| hasStack = stack.length > 1 || stack[0].funcName !== '(root)'; | ||
| } |
There was a problem hiding this comment.
It's a good idea to hide the stacks if we only have (root)! It wasn't looking good anyways.
| @@ -33,9 +33,6 @@ describe('getUsefulTabs', function () { | |||
| const profile = getProfileWithMarkers(getNetworkMarkers()); | |||
There was a problem hiding this comment.
Could you add a small tests for this new behavior too? We already see that in some of the tests, we don't have the sample related tabs anymore since the profiles only include markers, but it could be nice to add a simple test that checks this new behavior only.
The test name could be something like does not show sample related tabs when there is no sample data in the profile and it could test a profile like const profile = getMarkerTableProfile() or getProfileWithMarkers(getNetworkMarkers());
canova
left a comment
canova
left a comment
There was a problem hiding this comment.
Thanks for adding some tests and working on this! Looks great to me.
…'t have stack samples.
3 participants
This is to help with profiles where either the
nostacksamplingor themarkersallthreads.When
nostacksamplingis used, we have samples that only contain a '(root)' frame. Whenmarkersallthreadsis used, we have some tracks that have no samples at all. In both cases, the Call Tree, Flame Graph and Stack Chart panels are useless, and having the Marker Chart selected by default would be a lot more useful.Additionally, when we didn't sample the stack but still sampled the CPU, hovering the timeline in areas where some CPU was used shows a tooltip with the CPU percentage (good!) but also a broken empty stack with the unknown category and a single "(root)" frame. I think it's better to hide this stack to avoid distracting the user from the CPU use information that actually exists.
Example profiles: