Skip to content
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

sentry errors for captcha web views and registration attempts #3761

Merged
merged 12 commits into from
May 1, 2024

Conversation

haileyok
Copy link
Contributor

Why

We should have some more robust error reporting around signups. This adds Sentry logs to the most sensitive areas of that flow:

  • Captcha webview errors
  • Other cases where a captcha isn't set (we shouldn't hit this one but just in case?)
  • Any failure after submitting the registration request

Test Plan

Not really sure how to test this one, aside from waiting for these to come through on Sentry. For what it's worth, we already had some of these coming through but this makes the errors a little easier to set up alerts for using the "Signup Flow Error:" prefix.

Copy link

render bot commented Apr 30, 2024

@haileyok haileyok changed the title sentry errors for captcha web views sentry errors for captcha web views and registration attempts Apr 30, 2024
Copy link

github-actions bot commented Apr 30, 2024

Old size New size Diff
6.49 MB 6.49 MB 508 B (0.01%)

@@ -26,7 +26,7 @@ export function CaptchaWebView({
stateParam: string
state?: SignupState
onSuccess: (code: string) => void
onError: () => void
onError: (error: object) => void
Copy link
Collaborator

@gaearon gaearon Apr 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe not important in this particular case but in general we should get out of the habit of assuming that an error has a particular type. E.g. you technically can throw null or throw undefined or throw 'foo' and it's important that this doesn't trip or break error reporting. So errors are generally unknown maybe.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yea here these do have an object value, since they come from nativeEvent. But for consistency sake going forward, sure lets switch to unknown here 👍

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm are passing the event itself? If so, does it turn into something reasonable when we stringify it later?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The nativeEvent is just an object of various values that comes back from the native code when it fires the event. In this case, nativeEvent has the following values (depending on where the error comes from)

Screenshot 2024-04-29 at 10 25 04 PM Screenshot 2024-04-29 at 10 25 12 PM

Copy link
Collaborator

@gaearon gaearon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok but test the error paths for real errors (e.g. make the iframe trigger onError for real etc)

@haileyok
Copy link
Contributor Author

haileyok commented Apr 30, 2024

iframe error might not be reliable (I think onLoad will actually always fire here https://developer.mozilla.org/en-US/docs/Web/HTML/Element/iframe#error_and_load_event_behavior) so let's track with a few different things:

  • User didn't complete the captcha in 20 seconds
  • User pressed the back button on the captcha page (likely indicates they encountered some sort of error)
Screenshot 2024-04-29 at 10 23 36 PM Screenshot 2024-04-29 at 10 23 51 PM

@haileyok
Copy link
Contributor Author

haileyok commented Apr 30, 2024

@gaearon Can you think of any other meaningful ways we could get an error from here, mainly on web? The onHttpError and onError do fire correctly on web, though we can't get errors from the iframe. I think these are pretty good indicators though of errors (over 20s to complete and pressing back), and if we see a spike we can take it as a problem.

Edit: Let's make that 30 seconds instead. 20 seems kind of low honestly.

@haileyok haileyok merged commit b8d8bec into main May 1, 2024
6 checks passed
@haileyok haileyok deleted the hailey/signup-error-recording branch May 1, 2024 08:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants