feat: Wrap rrweb callbacks in wrapper to better handle errors #41
Conversation
mydea
force-pushed
the
fn/wrap-callback-errors
branch
from
589151b to
6605ec2
Compare
| @@ -17,7 +17,7 @@ import * as fs from 'fs'; | |||
|
|
|||
| export async function launchPuppeteer() { | |||
| return await puppeteer.launch({ | |||
| headless: process.env.PUPPETEER_HEADLESS ? true : false, | |||
| headless: process.env.PUPPETEER_DEBUG ? false : true, | |||
There was a problem hiding this comment.
I switched this to default to headless, as actually I got some flaky tests locally without this. IMHO headless makes sense as default, unless you really want to debug the chrome instance. If you think that's a bad change, I can revert this!
There was a problem hiding this comment.
for the flaky local tests, I've had better success with runInBand
mydea
force-pushed
the
fn/wrap-callback-errors
branch
from
6605ec2 to
e9aac32
Compare
| @@ -27,6 +27,7 @@ import { | |||
| import { IframeManager } from './iframe-manager'; | |||
| import { ShadowDomManager } from './shadow-dom-manager'; | |||
| import { CanvasManager } from './observers/canvas/canvas-manager'; | |||
| import { callbackWrapper } from '../sentry/callbackWrapper'; | |||
There was a problem hiding this comment.
I think this should be generally upstreamable, so I would try to name this in a way that is not sentry specific.
There was a problem hiding this comment.
Actually disregard this. I can see we need to separately upstream this with the amount of pending upstream changes already.
There was a problem hiding this comment.
Yeah, I'm actually thinking to upstream this as a more generic configurable errorHandler kind of thing. IMHO that probably makes the most sense from a generic rrweb usage perspective! Then we can configure the error handler to add the __rrweb__ stuff in sentry-javascript for us (long term, when we eventually update our fork to v2)
| @@ -27,6 +27,7 @@ import { | |||
| import { IframeManager } from './iframe-manager'; | |||
| import { ShadowDomManager } from './shadow-dom-manager'; | |||
| import { CanvasManager } from './observers/canvas/canvas-manager'; | |||
| import { callbackWrapper } from '../sentry/callbackWrapper'; | |||
There was a problem hiding this comment.
Actually disregard this. I can see we need to separately upstream this with the amount of pending upstream changes already.
|
ref rrweb-io#1107 - Upstream, I made a PR to add an |
| return callbackWrapper(() => { | ||
| handlers.forEach((h) => h()); | ||
| }; | ||
| }); |
There was a problem hiding this comment.
In observer you also wrap the callbacks in each handler, do you need to do that here as well?
There was a problem hiding this comment.
As the forEach is called sync inside of the callbackWrapper, I think it is not necessary there!
| } | ||
| return insertRule.apply(this, arguments); | ||
| }; | ||
| win.CSSStyleSheet.prototype.insertRule = new Proxy(insertRule, { |
There was a problem hiding this comment.
I'm wondering if we need callback wrappers for these methods... from what I've seen, exceptions haven't happened inside of these methods but rather when defining them as CSSStyleSheet.prototype isn't defined. -- e.g. here https://github.com/getsentry/rrweb/pull/41/files?diff=split&w=1#diff-b7a42c375ac5ab2de196b5e95e0b2f82b723969c5836fac0ff6f7ca6ad66ebd7R654-R655
There was a problem hiding this comment.
Yeah, I think this is more of an additional safeguard. IMHO it doesn't hurt to have them (and having them be proxies is probably safer as well).
| @@ -17,7 +17,7 @@ import * as fs from 'fs'; | |||
|
|
|||
| export async function launchPuppeteer() { | |||
| return await puppeteer.launch({ | |||
| headless: process.env.PUPPETEER_HEADLESS ? true : false, | |||
| headless: process.env.PUPPETEER_DEBUG ? false : true, | |||
There was a problem hiding this comment.
for the flaky local tests, I've had better success with runInBand
There was a problem hiding this comment.
I went through the PR in VSCode and tried to wrap my head around how these callbacks are are interconnected/nested.
I'm afraid chances are pretty high that we missed some weird async callback/edge case but I think overall it looks good and we now have a simple way of quickly wrapping any CB we might have missed. I guess the biggest challenge is figuring out that the individual, currently unwrapped CB are okay not to be wrapped because they're executed in sync in other CBs that are wrapped.
Great that you upstreamed the error handler PR. If it's accepted I think this is a good reason to consider going to 2.x. Also noted this in getsentry/sentry-javascript#6655 so we don't lose track of it.
Based on getsentry/rrweb#41, we can now ignore rrweb errors even when the code is minified.
Based on getsentry/rrweb#41, we can now ignore rrweb errors even when the code is minified.
This adds a
callbackWrappermethod which we use to wrap any (async) callbacks in the record/observer codebase.This wrapper will automatically try to add an
__rrweb__property to any error thrown in the given callback.Later in sentry, we can use this to determine to ignore a given error.
Note: I had to rewrite the monkey-patched CSSStyleSheet stuff into proxies, as otherwise I couldn't get the this-binding correctly via the prototype stuff. However, I think this is a nicer implementation anyhow, so should be good IMHO.
I also added some tests (this took >50% of the implementation time 🙈 as you can't easily access error objects from puppeteer) to verify this works.
The tests cover:
CSSStyleSheetmonkey patchingCSSGroupingRuleThis should give us a decent certainty that this generally works as expected. There may be a place or two that I missed, but I don't think so. I tried to put the wrappers in a way that it encapsulates any sync stuff going on.
I'd ask you @billyvg & @Lms24 to check this out and look at it in the editor, to really see the flow of functions & if I didn't miss any relevant function call that could slip through.
cc @mitsuhiko