Ignore frontend errors not originating from application script#10087
Merged
Ignore frontend errors not originating from application script#10087
Conversation
changelog: Internal, Error Logging, Ignore frontend errors not originating from application script
mitchellhenke
approved these changes
Feb 14, 2024
| try { | ||
| return new URL(event.filename).host === window.location.host; | ||
| const { host, pathname } = new URL(event.filename); | ||
| return host === window.location.host && pathname.endsWith('.js'); |
Contributor
There was a problem hiding this comment.
do you think it would make sense to add a similar check on the server? I guess we could always add later if it turns out to be too noisy
Contributor
Author
There was a problem hiding this comment.
do you think it would make sense to add a similar check on the server? I guess we could always add later if it turns out to be too noisy
That's a good question. I think generally yes, in the sense that we shouldn't trust any "validation" to occur exclusively in the client.
But it also sparks an idea that we could do this exclusively server-side, on which I'm conflicted:
- Since this is a critical-path script, anything to reduce its size is a win. And doing it in one place is simpler to maintain.
- But if we moved all validation to the server, it would put additional strain in handling X more requests, which could otherwise be filtered on the client
I'd probably lean toward keeping this as-is, and following-up with a pull request to perform the same validation server-side as a fallback.
zachmargolis
approved these changes
Feb 14, 2024
This file contains hidden or 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
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.
🛠 Summary of changes
Updates logic of
isTrackableErrorto ignore any errors which occur from the same host, but which aren't from JavaScript files.Following #10027, data for
filenameshows that errors are originating from filenames such ashttps://secure.login.gov/. The hypothesis is that app webviews or browser extensions may be injecting scripts evaluated on the page and throwing errors, which we want to ignore.📜 Testing Plan
echo "setTimeout(() => { throw new Error('example'); }, 100)" >> app/javascript/packages/analytics/index.tsNODE_ENV=production yarn buildrails sbinding.pryat thenotice_errorcall and check that it's triggered