-
Notifications
You must be signed in to change notification settings - Fork 32.9k
chore: log js stacks from unresponsive window #241390
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
chore: log js stacks from unresponsive window #241390
Conversation
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 wonder why we need to collect the stack every second, maybe we just start with collecting it once in the beginning and a second time after 5 seconds?
also let's remove |
5a615f1
to
e4bbec3
Compare
e4bbec3
to
f4c5f95
Compare
f4c5f95
to
4c5d6c8
Compare
ea95872
to
ff1bc9b
Compare
samples += count; | ||
// If the stack appears more than 20 percent of the time, log it | ||
// to the error telemetry as UnresponsiveSampleError. | ||
if (Math.round((count * 100) / this.jsCallStackEffectiveSampleCount) > 20) { |
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.
Taken inspiration from renderer auto profiler but unlike the auto profiler we don't have individual frame timings, given the samples are sorted by their effective appearance over the sample period, I feel this should cover the interesting bits to error telemetry as UnresponsiveSampleError
Adding the window id and process id to the error message so that we can aggregate them. Let me know if there are better markers to add.
} | ||
|
||
this.jsCallStackMap = new Map<string, number>(); | ||
this.jsCallStackEffectiveSampleCount = Math.round(sampleInterval / samplePeriod); |
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.
Won't this be zero, and then cause div-by-zero below?
> parseInt('1000') / parseInt('15000')
0.06666666666666667
> Math.round(parseInt('1000') / parseInt('15000'))
0
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.
@deepak1556 can you check?
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.
Oops, good catch. It should have been the other way.
this.jsCallStackEffectiveSampleCount = Math.round(samplePeriod / sampleInterval);
Fixes #240178
Logs look as follows in a simulated unresponsiveness,