-
Notifications
You must be signed in to change notification settings - Fork 452
Display traced values in Stack Chart view #5363
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
@squarewave note that this is crashing on chrome |
72940af to
eee6ac3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| --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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are they the variables used inside reps css file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.63% 85.63%
========================================
Files 312 312
Lines 30892 30956 +64
Branches 8420 8543 +123
========================================
+ Hits 26453 26509 +56
- Misses 4009 4016 +7
- 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| --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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |

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.