-
Notifications
You must be signed in to change notification settings - Fork 449
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
| sameWidthsStart: number[]; | ||
| sameWidthsEnd: number[]; | ||
| callNode: IndexIntoCallNodeTable[]; | ||
| argumentValues?: number[]; |
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: could you add a comment explaining that this is used for JS execution tracing argument values?
| : formatMilliseconds(duration); | ||
| let argumentSummaries = undefined; | ||
| if (timing.argumentValues) { | ||
| const argumentValues = timing.argumentValues[stackTimingIndex]; |
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.
The argumentValues variable is really a argumentValuesIndex right? Could we maybe rename it to be clearer?
| thread.tracedValuesBuffer as ArrayBuffer, | ||
| thread.tracedObjectShapes as 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.
Can we remove these casts by checking if they are present in the if check above?
if (timing.argumentValues) {Could be updated to cover these:
if (timing.argumentValues && thread.tracedValuesBuffer && thread.tracedObjectShapes) {Technically if argumentValues is present the others should be too, but it's good to be a bit more defensive and avoid an explicit cast.
| if (timing.argumentValues) { | ||
| const argumentValues = timing.argumentValues[stackTimingIndex]; | ||
| if (argumentValues !== -1) { | ||
| argumentSummaries = ValueSummaryReader.getArgumentSummaries( |
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.
Hmm typescript says the return value of ValueSummaryReader.getArgumentSummaries is any which isn't very helpful. Do you know what it returns? I see Array<object> below, I would say that it's better than any. Ideally it would be good to type them.
| export const REPS: any; | ||
| export function getRep(object: any, defaultRep?: any): any; | ||
|
|
||
| export function parseURLEncodedText(text: string): any; | ||
| export function parseURLParams(url: string): any; | ||
| export function maybeEscapePropertyName(name: string): string; | ||
| export function getGripPreviewItems(grip: any): any[]; | ||
|
|
||
| export const ValueSummaryReader: { | ||
| getArgumentSummaries: ( | ||
| valuesBuffer: ArrayBuffer, | ||
| shapes: Array<string[] | null>, | ||
| valuesBufferIndex: number | ||
| ) => any; | ||
| }; |
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.
Ah I see that the types are added here. It would be good to eliminate as much as any as possible. It's fine if we can't type all of them. I can say that if a value is only used inside reps, we can just keep them as object, but if we access any property of it, then it's good to type.
For example, I see that we get the object summaries with getArgumentSummaries and then simply pass it to Reps, so it should be fine to keep them as Array<object> (if that's true)
It seems like we don't use other exported functions (getRep, parseURLEncodedText, parseURLParams, maybeEscapePropertyName, getGripPreviewItems) so we don't need to type them more specifically.
| // 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?
| "common-tags": "^1.8.2", | ||
| "copy-to-clipboard": "^3.3.3", | ||
| "core-js": "^3.47.0", | ||
| "devtools-reps": "^0.27.3", |
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.
Looks like 0.27.4 is out now. I don't know if you need it or if it's crucial. Otherwise our dependency updates will handle it.
| } | ||
| argumentsElement = ( | ||
| <div className="arguments"> | ||
| <div className="argumentsLabel">Arguments</div> |
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: to make it similar to the other fields:
| <div className="argumentsLabel">Arguments</div> | |
| <div className="argumentsLabel">Arguments:</div> |
can we also switch arguments and argumentsLabel classes to tooltipArguments and tooltipArgumentsLabel here and in the css?
And looking at the tooltip, the arguments class needs the same padding as the other tooltip content I think so it looks more unified. We have this for the rest of the tooltip:
profiler/src/components/tooltip/CallNode.css
Lines 91 to 92 in 5dd4c64
| padding: 10px; | |
| padding-bottom: 5px; |
tip: you can call window.persistTooltips = true in the console to make them sticky while working on them.
|
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.