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
Centralise frontend error reporting (and suppress unactionable Sentry errors) #3850
Conversation
I'm leaving this as a draft intentionally. I'll work on the Playwright tests we discussed in the original PR tomorrow. |
…eature flag json See note regarding setup-after-env in this section of Jest documentation: https://jestjs.io/docs/jest-object\#jestmockmodulename-factory-options
89704b2
to
9379db2
Compare
Full-stack documentation: https://docs.openverse.org/_preview/3850 Please note that GitHub pages takes a little time to deploy newly pushed code, if the links above don't work or you see old versions, wait 5 minutes and try again. You can check the GitHub pages deployment action list to see the current status of the deployments. |
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've tested this PR locally, and it seems to work really well.
// Only match the search requests, rather than any other API request the search page makes | ||
return Boolean( | ||
url.pathname.match(/v1\/(audio|images)/) && | ||
url.searchParams.has("q", "galah") |
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.
Nice way of matching by params!
/** | ||
* Record network errors using the appropriate tool, as needed, | ||
* based on response code, status, and request kind. | ||
* @param error - The error to record |
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.
This docstring needs to be updated.
{ | ||
id, | ||
} |
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.
{ | |
id, | |
} | |
{ id } |
Nit
{ | ||
searchTerm: queryParams.q ?? "", | ||
} |
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.
{ | |
searchTerm: queryParams.q ?? "", | |
} | |
{ searchTerm: queryParams.q ?? "" } |
Nit: these seem more readable when on a single line
* implementation or configuration error suddenly causing malformed | ||
* identifiers to be used. Neither Sentry nor Plausible are the right tool | ||
* for that task. If the 404s are caused by an API issue, we'd see that in | ||
* API response code monitoring, where we can more easily trace the cause |
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.
* API response code monitoring, where we can more easily trace the cause | |
* API response code monitoring, where we can more easily trace the cause. |
} else { | ||
context.$sentry.captureException(originalError, { | ||
extra: { fetchingError }, | ||
}) | ||
} | ||
} |
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.
``
} else { | |
context.$sentry.captureException(originalError, { | |
extra: { fetchingError }, | |
}) | |
} | |
} | |
return | |
} | |
// Send all other errors to Sentry | |
context.$sentry.captureException(originalError, { | |
extra: { fetchingError }, | |
}) | |
} |
Style nit: perhaps this would make adding future additional cases less error-prone, or avoid confusion if folks think there's some special relationship between the process.client && fetchingError.code === "ERR_NETWORK"
errors and Sentry specifically.
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 went back and forth many times on this, and ultimately found both just as confusing, and went with a coin toss. I'll switch this, as any actual preference is better than my 50/50 non-choice!
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.
This is really, really nice! Wow. Great code and works perfectly. Unit tests passed locally, failed correctly when modified, and I was able to produce a NETWORK_ERROR analytics event using the instructions. I left a minor typo and code style suggestion but LGTM.
I merged this, forgetting that the reason I didn't do so earlier was because of some outstanding minor feedback. Apologies! I am not intentionally disregarding them. If any of those pieces feel critical enough to make issues for, please let me know which ones and I can create the issues @zackkrida @obulat. Some of them are things ESlint should be able to catch: early return vs |
Looks fine as-is for me 👍 |
Fixes
Fixes #3468 by @krysal
Superceeds #3721 after rebasing the branch to include #3811.
Description
Introduces a new plugin to centralise network error handling. This allows us to make large-scale changes to how we handle certain classes of networking errors under certain conditions. To resolve the problems in the issue, this PR ignores the types of errors mentioned there, or sends information about them to a place we can better understand them (Plausible). For many ignored errors, we have other mechanisms to inspect and understand them, like API logs, or Cloudflare traffic analysis.
Testing Instructions
Confirm the new tests sufficiently verify the behaviour of the plugin.
Run the app locally, and visit a search page. Open your browser dev tools network tab, and navigate to a single result. Find the "Related" request for the result, and block it using your dev tools (in Firefox you right click and "Block this URL" from the context menu). Navigate back to search, reload to clear the pinia cache of the related request, then go back to the same single result page. Confirm that the dev console has a log event for an analytics event, NETWORK_ERROR, and that no Sentry event was sent. You can confirm the sentry events by adding a console log at that line so you can see it in your nuxt dev console.
Checklist
Update index.md
).main
) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin