-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
fix(integrations): Treat specific CustomEvents
as promise rejections
#2429
Merged
kamilogorek
merged 6 commits into
master
from
kmclb-fix-custom-events-wrapping-promise-rejection-events
Feb 14, 2020
Merged
fix(integrations): Treat specific CustomEvents
as promise rejections
#2429
kamilogorek
merged 6 commits into
master
from
kmclb-fix-custom-events-wrapping-promise-rejection-events
Feb 14, 2020
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lobsterkatie
added a commit
to getsentry/relay
that referenced
this pull request
Feb 14, 2020
As discussed at length in the issue and PR linked below, `Extension context invalidated` errors (which result from an extension not being able to communicate with its injected scripts, or vice versa) have lately been spamming a subset of our customers. This filters them out. (It also removes a duplicate entry from the related test.) getsentry/sentry-javascript#2380 getsentry/sentry-javascript#2429
kamilogorek
approved these changes
Feb 14, 2020
kamilogorek
deleted the
kmclb-fix-custom-events-wrapping-promise-rejection-events
branch
February 14, 2020 10:46
These pesky browsers... great fix, thanks! |
This was referenced Mar 14, 2020
9 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As explained here, we've started seeing a rash of events with a specific structure that are spamming certain customers. TL;DR, this handles those events more gracefully so they can be properly filtered.
Background
Both
PromiseRejectionEvent
andCustomEvent
implement theEvent
interface, but they each add at least one property not included inEvent
's spec. In the case ofPromiseRejectionEvent
, it addspromise
(the promise that rejected) andreason
(the argument which would have been passed to a.catch
clause in the promise chain, had there been one; in the case of an error being thrown inside the promise,reason
is that error). ForCustomEvent
, it'sdetail
.The events we're seeing have all the data of a
PromiseRejectionEvent
(includingtype: unhandledrejection
), but they're notPromiseRejectionEvent
s - they'reCustomEvent
s, with thepromise
andreason
properties from the former stored in an object in thedetail
property of the latter. And because they're structured differently, currently the SDK doesn't handle these events gracefully, or rather: it handles them as gracefully as it handles any otherCustomEvent
- pulls as much data as it can off of it and serializes that data inextra
, but doesn't recognize it as something it can do anything more with, even ifevent.detail.reason
is in fact a full-fledgedError
.The Current Problem
This has come up recently because multiple customers are getting spammed by events like this, all with the same error nested inside them:
Extension context invalidated.
This turns out to be an error that Chrome throws when an extension stops being able to communicate with the script it's injected into a given page. If one or the other end of that communication channel continues to ping its (now-missing) interlocutor, and doesn't catch the errors (and then promise rejections) which result, a large number of events can pile up, even from a single end user. (This tracks with what we're seeing in customers' accounts, where in some cases tens of thousands of events are being generated by just a few dozen users.)Unless you're the extension developer, these events are just third-party noise that eat your quota, and while they can be blocked manually using
beforeSend
, this takes effort (and probably writing into support) and feels like a hack when we have a server-side filter explicitly designed to handle such problems. That filter can't currently act on these events because of their weird structure, but...The Solution
By now it should be clear where this is headed. If we can dig the
Error
out of theCustomEvent
, we can then filter it like we would any other browser extension error. This does that digging, both in theGlobalHandlers
integration and the pared down version present in the loader. (It also fixes a random typo I found in the dedupe integration while researching this.)Tests can be found in #2431.
Fixes #2380.