Allow exporting argument values in profiles#5914
Allow exporting argument values in profiles#5914squarewave wants to merge 2 commits intofirefox-devtools:mainfrom
Conversation
I'm not actually sure why I initially put this in here - I think it was just overzealous flow type fixing that I then ported blindly to typescript when I had to update the patches.
This adds a checkbox which will allow the user to include argument values in exported profiles. The export also respects the checkbox to only export the specified timerange by reconstructing the values buffer to only include arguments referenced by included samples.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #5914 +/- ##
==========================================
- Coverage 85.41% 85.35% -0.07%
==========================================
Files 321 321
Lines 32157 32201 +44
Branches 8865 8794 -71
==========================================
+ Hits 27468 27486 +18
- Misses 4253 4279 +26
Partials 436 436 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
canova
left a comment
canova
left a comment
There was a problem hiding this comment.
Thanks for this feature!
See my comment below for the place where we mutate the old samples object. It would be good to shallow copy that as well to make sure we don't mutate it.
I'm wondering about the wording in the tooltip. I think we should make it a bit more scary to make sure that the users really understand before they share it. Let me know what you think!
| // We always want to sanitize the private browsing data by default | ||
| includePrivateBrowsingData: false, | ||
| // We always want to sanitize the argument values by default since they may contain PII | ||
| includeArgumentValues: false, |
There was a problem hiding this comment.
That's the right decision, thanks!
| MenuButtons--publish--renderCheckbox-label-private-browsing = Include the data from private browsing windows | ||
| MenuButtons--publish--renderCheckbox-label-private-browsing-warning-image = | ||
| .title = This profile contains private browsing data | ||
| MenuButtons--publish--renderCheckbox-label-argument-values = Include function argument values |
There was a problem hiding this comment.
Should we explicitly mention that these argument values are coming from the JS execution tracing? For example:
Include JavaScript execution tracing function argument values
Also it might make sense to emphasize that the values are kept in all profiled threads to make sure that we mention the non-active tab is also recorded.
| readonly rootRange: StartEndRange; | ||
| readonly shouldShowPreferenceOption: boolean; | ||
| readonly profileContainsPrivateBrowsingInformation: boolean; | ||
| readonly profileHasArgumentValues: boolean; |
There was a problem hiding this comment.
nit: It would be also good to mention that these argument values are related to the JS execution tracing. Maybe something like profileHasJSTracingArgumentValues? (here and below in the patch)
| ); | ||
|
|
||
| newThread.tracedValuesBuffer = bytesToBase64(filtered.valuesBuffer); | ||
| newThread.samples.argumentValues = filtered.entryIndices; |
There was a problem hiding this comment.
Since we only shallow copied the thread in newThread above, this mutates the original samples table.
Can you also shallow clone the samples table and overwrite argumentValues on it?
Since the old profile is mutated here, this might break our "revert to old profile" button after publishing.
3 participants
This adds a checkbox which will allow the user to include argument values in exported profiles. The export also respects the checkbox to only export the specified timerange by reconstructing the values buffer to only include arguments referenced by included samples.