Display traced values in Stack Chart view#5363
Conversation
| @squarewave note that this is crashing on chrome |
squarewave force-pushed the main branch 2 times, most recently from 72940af to eee6ac3 Compare 
canova changed the title 
There was a problem hiding this comment.
Thanks for the PR! It looks like it's mostly working well! I have a few questions below, but my main concern is the crashes/exceptions I was getting when I was trying the deploy preview.
I changed the devtools.performance.recording.ui-base-url pref to "https://deploy-preview-5363--perf-html.netlify.app" and started capturing some example profiles with the JS tracer enabled. While hovering over some of the sampples in stack chart, I've encountered these crashes:
An unhandled error was thrown in a React component. Error: Bad object kind MC value-summaries.js:423 FC value-summaries.js:476 NC value-summaries.js:257 MC value-summaries.js:410 FC value-summaries.js:476 RC value-summaries.js:495 _getHoveredStackInfo Canvas.js:616 _getHoveredItemInfo Canvas.js:342 render Canvas.js:398 React 8 w scheduler.production.min.js:13 _ scheduler.production.min.js:14 7463 scheduler.production.min.js:14 Webpack 13 rC@https://deploy-preview-5363--perf-html.netlify.app/main.2fcff16d608871ec81f3.bundle.js:120:47154 qE@https://deploy-preview-5363--perf-html.netlify.app/main.2fcff16d608871ec81f3.bundle.js:120:129223 div t@https://deploy-preview-5363--perf-html.netlify.app/main.2fcff16d608871ec81f3.bundle.js:120:36629 c@https://deploy-preview-5363--perf-html.netlify.app/main.2fcff16d608871ec81f3.bundle.js:2:230673 div div aA@https://deploy-preview-5363--perf-html.netlify.app/main.2fcff16d608871ec81f3.bundle.js:117:43084 iA@https://deploy-preview-5363--perf-html.netlify.app/main.2fcff16d608871ec81f3.bundle.js:117:45644 div tk@https://deploy-preview-5363--perf-html.netlify.app/main.2fcff16d608871ec81f3.bundle.js:123:481 c@https://deploy-preview-5363--perf-html.netlify.app/main.2fcff16d608871ec81f3.bundle.js:2:230673 fg@https://deploy-preview-5363--perf-html.netlify.app/main.2fcff16d608871ec81f3.bundle.js:117:20380 Ag $e@https://deploy-preview-5363--perf-html.netlify.app/main.2fcff16d608871ec81f3.bundle.js:2:223805 div Gx@https://deploy-preview-5363--perf-html.netlify.app/main.2fcff16d608871ec81f3.bundle.js:132:49519 c@https://deploy-preview-5363--perf-html.netlify.app/main.2fcff16d608871ec81f3.bundle.js:2:230673 div s@https://deploy-preview-5363--perf-html.netlify.app/main.2fcff16d608871ec81f3.bundle.js:207:46805 div e@https://deploy-preview-5363--perf-html.netlify.app/main.2fcff16d608871ec81f3.bundle.js:207:48453 component@https://deploy-preview-5363--perf-html.netlify.app/main.2fcff16d608871ec81f3.bundle.js:132:51659 c@https://deploy-preview-5363--perf-html.netlify.app/main.2fcff16d608871ec81f3.bundle.js:2:230673 div s@https://deploy-preview-5363--perf-html.netlify.app/main.2fcff16d608871ec81f3.bundle.js:207:46805 div e@https://deploy-preview-5363--perf-html.netlify.app/main.2fcff16d608871ec81f3.bundle.js:207:48453 div s@https://deploy-preview-5363--perf-html.netlify.app/main.2fcff16d608871ec81f3.bundle.js:207:46805 div e@https://deploy-preview-5363--perf-html.netlify.app/main.2fcff16d608871ec81f3.bundle.js:207:48453 div div JT@https://deploy-preview-5363--perf-html.netlify.app/main.2fcff16d608871ec81f3.bundle.js:139:26671 BP@https://deploy-preview-5363--perf-html.netlify.app/main.2fcff16d608871ec81f3.bundle.js:143:117837 c@https://deploy-preview-5363--perf-html.netlify.app/main.2fcff16d608871ec81f3.bundle.js:2:230673 CM@https://deploy-preview-5363--perf-html.netlify.app/main.2fcff16d608871ec81f3.bundle.js:143:150225 c@https://deploy-preview-5363--perf-html.netlify.app/main.2fcff16d608871ec81f3.bundle.js:2:230673 ef@https://deploy-preview-5363--perf-html.netlify.app/main.2fcff16d608871ec81f3.bundle.js:117:3169 c@https://deploy-preview-5363--perf-html.netlify.app/main.2fcff16d608871ec81f3.bundle.js:2:230673 div MP@https://deploy-preview-5363--perf-html.netlify.app/main.2fcff16d608871ec81f3.bundle.js:143:119982 c@https://deploy-preview-5363--perf-html.netlify.app/main.2fcff16d608871ec81f3.bundle.js:2:230673 fg@https://deploy-preview-5363--perf-html.netlify.app/main.2fcff16d608871ec81f3.bundle.js:117:20380 Ag $e@https://deploy-preview-5363--perf-html.netlify.app/main.2fcff16d608871ec81f3.bundle.js:2:223805 Ve@https://deploy-preview-5363--perf-html.netlify.app/main.2fcff16d608871ec81f3.bundle.js:2:223718 ZM@https://deploy-preview-5363--perf-html.netlify.app/main.2fcff16d608871ec81f3.bundle.js:143:170476 c@https://deploy-preview-5363--perf-html.netlify.app/main.2fcff16d608871ec81f3.bundle.js:2:230673 jt@https://deploy-preview-5363--perf-html.netlify.app/main.2fcff16d608871ec81f3.bundle.js:2:229088 fg@https://deploy-preview-5363--perf-html.netlify.app/main.2fcff16d608871ec81f3.bundle.js:117:20380 gg eF@https://deploy-preview-5363--perf-html.netlify.app/main.2fcff16d608871ec81f3.bundle.js:143:171580 ErrorBoundary.js:43:13 componentDidCatch ErrorBoundary.js:43 React 8 w scheduler.production.min.js:13 _ scheduler.production.min.js:14 (Async: EventHandlerNonNull) 7463 scheduler.production.min.js:14 Webpack 13 And I got this:
An unhandled error was thrown in a React component. Error: Bad value type FC value-summaries.js:479 NC value-summaries.js:257 MC value-summaries.js:410 FC value-summaries.js:476 RC value-summaries.js:495 _getHoveredStackInfo Canvas.js:616 _getHoveredItemInfo Canvas.js:342 render Canvas.js:398 React 8 w scheduler.production.min.js:13 _ scheduler.production.min.js:14 rC@https://deploy-preview-5363--perf-html.netlify.app/main.2fcff16d608871ec81f3.bundle.js:120:47154 qE@https://deploy-preview-5363--perf-html.netlify.app/main.2fcff16d608871ec81f3.bundle.js:120:129223 div t@https://deploy-preview-5363--perf-html.netlify.app/main.2fcff16d608871ec81f3.bundle.js:120:36629 c@https://deploy-preview-5363--perf-html.netlify.app/main.2fcff16d608871ec81f3.bundle.js:2:230673 div div aA@https://deploy-preview-5363--perf-html.netlify.app/main.2fcff16d608871ec81f3.bundle.js:117:43084 iA@https://deploy-preview-5363--perf-html.netlify.app/main.2fcff16d608871ec81f3.bundle.js:117:45644 div tk@https://deploy-preview-5363--perf-html.netlify.app/main.2fcff16d608871ec81f3.bundle.js:123:481 c@https://deploy-preview-5363--perf-html.netlify.app/main.2fcff16d608871ec81f3.bundle.js:2:230673 fg@https://deploy-preview-5363--perf-html.netlify.app/main.2fcff16d608871ec81f3.bundle.js:117:20380 Ag $e@https://deploy-preview-5363--perf-html.netlify.app/main.2fcff16d608871ec81f3.bundle.js:2:223805 div Gx@https://deploy-preview-5363--perf-html.netlify.app/main.2fcff16d608871ec81f3.bundle.js:132:49519 c@https://deploy-preview-5363--perf-html.netlify.app/main.2fcff16d608871ec81f3.bundle.js:2:230673 div s@https://deploy-preview-5363--perf-html.netlify.app/main.2fcff16d608871ec81f3.bundle.js:207:46805 div e@https://deploy-preview-5363--perf-html.netlify.app/main.2fcff16d608871ec81f3.bundle.js:207:48453 component@https://deploy-preview-5363--perf-html.netlify.app/main.2fcff16d608871ec81f3.bundle.js:132:51659 c@https://deploy-preview-5363--perf-html.netlify.app/main.2fcff16d608871ec81f3.bundle.js:2:230673 div s@https://deploy-preview-5363--perf-html.netlify.app/main.2fcff16d608871ec81f3.bundle.js:207:46805 div e@https://deploy-preview-5363--perf-html.netlify.app/main.2fcff16d608871ec81f3.bundle.js:207:48453 div s@https://deploy-preview-5363--perf-html.netlify.app/main.2fcff16d608871ec81f3.bundle.js:207:46805 div e@https://deploy-preview-5363--perf-html.netlify.app/main.2fcff16d608871ec81f3.bundle.js:207:48453 div div JT@https://deploy-preview-5363--perf-html.netlify.app/main.2fcff16d608871ec81f3.bundle.js:139:26671 BP@https://deploy-preview-5363--perf-html.netlify.app/main.2fcff16d608871ec81f3.bundle.js:143:117837 c@https://deploy-preview-5363--perf-html.netlify.app/main.2fcff16d608871ec81f3.bundle.js:2:230673 CM@https://deploy-preview-5363--perf-html.netlify.app/main.2fcff16d608871ec81f3.bundle.js:143:150225 c@https://deploy-preview-5363--perf-html.netlify.app/main.2fcff16d608871ec81f3.bundle.js:2:230673 ef@https://deploy-preview-5363--perf-html.netlify.app/main.2fcff16d608871ec81f3.bundle.js:117:3169 c@https://deploy-preview-5363--perf-html.netlify.app/main.2fcff16d608871ec81f3.bundle.js:2:230673 div MP@https://deploy-preview-5363--perf-html.netlify.app/main.2fcff16d608871ec81f3.bundle.js:143:119982 c@https://deploy-preview-5363--perf-html.netlify.app/main.2fcff16d608871ec81f3.bundle.js:2:230673 fg@https://deploy-preview-5363--perf-html.netlify.app/main.2fcff16d608871ec81f3.bundle.js:117:20380 Ag $e@https://deploy-preview-5363--perf-html.netlify.app/main.2fcff16d608871ec81f3.bundle.js:2:223805 Ve@https://deploy-preview-5363--perf-html.netlify.app/main.2fcff16d608871ec81f3.bundle.js:2:223718 ZM@https://deploy-preview-5363--perf-html.netlify.app/main.2fcff16d608871ec81f3.bundle.js:143:170476 c@https://deploy-preview-5363--perf-html.netlify.app/main.2fcff16d608871ec81f3.bundle.js:2:230673 jt@https://deploy-preview-5363--perf-html.netlify.app/main.2fcff16d608871ec81f3.bundle.js:2:229088 fg@https://deploy-preview-5363--perf-html.netlify.app/main.2fcff16d608871ec81f3.bundle.js:117:20380 gg eF@https://deploy-preview-5363--perf-html.netlify.app/main.2fcff16d608871ec81f3.bundle.js:143:171580 ErrorBoundary.js:43:13 So it looks like we get both bad object and value types somehow. We should fix these issues before landing, but I also have a few questions:
- Are we expected to have unknown object/value types? Is this controlled in the backend somewhere? (like we only allow some "known" types to be put inside the profiles etc.)
- What happens if new types get added in the future? Is there a version control somewhere? Currently we have gecko and processes profile versioning. I think it might be good to make sure that we don't break the profiler when a new type gets added in the future.
res/css/global.css Outdated
| --number-color: #058b00; | ||
| --string-color: #dd00a9; | ||
| --null-color: #5c5c5f; | ||
| --object-color: #0074e8; | ||
| --caption-color: #0074e8; | ||
| --location-color: #5c5c5f; | ||
| --source-link-color: #0060df; | ||
| --node-color: #003eaa; | ||
| --reference-color: #0074e8; | ||
| --comment-node-color: #5c5c5f; |
There was a problem hiding this comment.
Are they the variables used inside reps css file?
There was a problem hiding this comment.
They are - do you have a suggestion on where they should live, if you'd like them to live elsewhere?
There was a problem hiding this comment.
I think it's fine for them to live there. From the package devtools-reps perspective maybe it would be better to have a prefix in their name to make sure that they don't clash with other variables (like --reps-number-color etc.). But since devtools-reps is published already I don't think it's something we should change right away. It might be good to file a bug in the devtools side though. cc @ochameau
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@ ## main #5363 +/- ## ========================================== + Coverage 85.62% 85.64% +0.01% ========================================== Files 319 319 Lines 31247 31318 +71 Branches 8596 8627 +31 ========================================== + Hits 26756 26822 +66 - Misses 4061 4065 +4 - Partials 430 431 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| ], | ||
| | ||
| transform: { | ||
| '\\.([jt]sx?|mjs)$': 'babel-jest', |
There was a problem hiding this comment.
nit: it would be good to add a comment here saying that \\.[jt]sx?$ is the default regexp and we change it to make sure that mjs files are included.
res/css/global.css Outdated
| --number-color: #058b00; | ||
| --string-color: #dd00a9; | ||
| --null-color: #5c5c5f; | ||
| --object-color: #0074e8; | ||
| --caption-color: #0074e8; | ||
| --location-color: #5c5c5f; | ||
| --source-link-color: #0060df; | ||
| --node-color: #003eaa; | ||
| --reference-color: #0074e8; | ||
| --comment-node-color: #5c5c5f; |
There was a problem hiding this comment.
I think it's fine for them to live there. From the package devtools-reps perspective maybe it would be better to have a prefix in their name to make sure that they don't clash with other variables (like --reps-number-color etc.). But since devtools-reps is published already I don't think it's something we should change right away. It might be good to file a bug in the devtools side though. cc @ochameau
| // It's absent in Firefox 97 and before, or in Firefox 98+ when this thread | ||
| // had no extra attribute at all. | ||
| userContextId?: number; | ||
| tracedObjectShapes?: Array<string[] | null>; |
There was a problem hiding this comment.
In the src/types/gecko-profile.ts file the string array is not nullable. Is that expected?
| Oh weird, the main review comment I wrote wasn't send with the rest of the review, thankfully it was saved in another tab. Here's it: Thanks for all the work! It's working well locally! I added some smaller comments below, but I also wanna talk about some more high concepts (that we can improve later, not necessarily for this PR). First thing is the sharing the traced values. Since it's a potential PII leak, we should try to be careful. Ideally it would be good to have a checkbox in the sharing panel that's always unchecked by default for traced values, and then share only when the user checks that explicitly. But this is a bit more work. As a first step, we can always strip the traced values for now when we are sharing a profile. Then we can work on a follow-up to add a checkbox in the UI. I can also help with that. But in this PR at least we should remove the traced values by default so we don't leak user info and file a follow-up issue. To be able to do that, we should update the sanitization code after here to delete the thread fields: profiler/src/profile-logic/sanitize.ts Line 406 in 5dd4c64 and a very small test here would be great: profiler/src/test/unit/sanitize.test.ts Line 35 in 5dd4c64 Like we discussed a bit before, another thing to discuss is the UX around the stack chart tooltips. It would be good to make the values expandable. Currently if the object is very nested/large, we miss a lot of values there. But again, this can be worked on as a follow-up. It's good to discuss its UX in a new issue though. And lastly, I think "Arguments" label looks unaligned in the tooltips:  I assume it's this way since the Reps values are full width, and not like the key-label values above. We can maybe improve the UI if we have a clear separation between the key-label values vs the arguments. |
There was a problem hiding this comment.
Sorry for the delay on the reviews Alex! And thank you a lot for working on this. I filed #5790 for adding a sanitization step for the traced values.
It looks like this PR bitrotted again (sorry!) :( Could you rebase this on top of the recent main, so we can land it?
I saw one more place to improve in terms of UI, but I think we can also work on that in a follow up. I don't want this PR to bitrot more. See the "No arguments" text here:
So let's land this after rebase.
Changes: [Nazım Can Altınova] Make the range duration text white again (#5792) [Alex Thayer] Display traced values in Stack Chart view (#5363) [Nazım Can Altınova] Improve the JS traced arguments visualization in call node tooltips (#5795) [Markus Stange] Make the argument-values.json profile fixture go through profile upgrading (#5796) [Markus Stange] Add thread.usedInnerWindowIDs to the processed profile format (#5780) [fatadel] Fix context menu and hover preview z-index (#5797) [fatadel] add TrackPower--tooltip-power-microwatt (#5799) [Markus Stange] Two small test fixes (#5801) [fatadel] fix selected thread pid color in light mode (#5805) [Markus Stange] Some fixes to the profile merging code (#5802) [fatadel] fix disabled button color in dark mode (#5808) [fatadel] add comment for uptime label translation (#5806) [Nazım Can Altınova] Add a theme toggle to the home page and follow the system theme by default (#5800) [Nazım Can Altınova] 🔃 Sync: l10n -> main (Feb 4, 2025) (#5813) And huge thanks to our localizers: de: Ger de: Michael Köhler de: Nazım Can Altınova el: Jim Spentzos en-GB: Ian Neal es-CL: ravmn fy-NL: Fjoerfoks ia: Melo46 it: Francesco Lodolo [:flod] nl: Mark Heijl pt-BR: Marcelo Ghelman ru: Valery Ledovskoy sv-SE: Andreas Pettersson tr: Selim Şumlu zh-TW: Pin-guang Chen
3 participants
This is a rough draft of adding in the support for displaying traced values (from the JS Execution Tracing option) in the Stack Chart view. This currently requires the work in https://phabricator.services.mozilla.com/D236936 to function.