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

Add sentry tracking for registration endpoint/errors #9096

Merged

Conversation

jimchamp
Copy link
Collaborator

@jimchamp jimchamp commented Apr 16, 2024

In support of #8863

  1. Update our Sentry objects to allow for capturing handled exceptions
  2. Creates new OLAuthenticationError, which are now thrown on IA account creation failures
  3. Publishes /account/create POST handler to Sentry

Technical

Testing

  1. Deploy this to testing
  2. Trigger an error during account registration
  3. Check Sentry for an OLAuthenticationError

Screenshot

Stakeholders

@jimchamp jimchamp force-pushed the 8863/registration-instrumentation branch from 35d91c3 to 20b4f0e Compare April 26, 2024 23:25
@jimchamp jimchamp marked this pull request as ready for review April 26, 2024 23:33
@@ -669,13 +673,13 @@ def create(
return ia_account

elif 'screenname' not in response.get('values', {}):
errors = '_'.join(response.get('values', {}))
raise ValueError(errors)
raise OLAuthenticationError('undefined_error')
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Created new undefined_error for this case. I am not familiar with the types of errors that can occur here, I was unable to find any existing errors in Sentry for insight.

ve.value = _screenname
raise ve
e = OLAuthenticationError('username_registered')
e.value = _screenname
Copy link
Collaborator Author

@jimchamp jimchamp Apr 26, 2024

Choose a reason for hiding this comment

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

This value doesn't seem to be accessed anywhere.

except ValueError:
f.note = get_login_error('max_retries_exceeded')
except OLAuthenticationError as e:
f.note = get_login_error(e.__str__())
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

e.__str__() returns the exception's arguments. In this case, its the exception's message, which is a key for the LOGIN_ERRORS dict.

@mekarpeles mekarpeles self-assigned this Apr 29, 2024
except OLAuthenticationError as e:
f.note = get_login_error(e.__str__())
if sentry.enabled:
sentry.capture_exception(e)
Copy link
Collaborator

Choose a reason for hiding this comment

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

We shouldn't need to call this manually; sentry under the hood should be capturing all these errors, even if they're excepted. You should be able to test this by hooking up your local environment to sentry.

@mekarpeles mekarpeles added this to the Sprint 2024-04 milestone May 6, 2024
Copy link
Member

@mekarpeles mekarpeles left a comment

Choose a reason for hiding this comment

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

LGTM, looks like there may be one question about

if sentry.enabled:
                    sentry.capture_exception(e)

but maybe we can remove these two lines and merge... and if we need to add them back in, we do 🤷

@jimchamp jimchamp marked this pull request as draft May 8, 2024 18:27
@jimchamp jimchamp force-pushed the 8863/registration-instrumentation branch from cdc5eb6 to cb16606 Compare May 8, 2024 18:56
@jimchamp jimchamp added the State: Blocked Work has stopped, waiting for something (Info, Dependent fix, etc. See comments). [managed] label May 8, 2024
@jimchamp
Copy link
Collaborator Author

jimchamp commented May 8, 2024

State: Blocked Work has stopped, waiting for something (Info, Dependent fix, etc. See comments). [managed] until the exception test handler commit has been removed.

@jimchamp jimchamp added the On testing.openlibrary.org This PR has been deployed to testing.openlibrary.org for testing label May 8, 2024
@jimchamp jimchamp force-pushed the 8863/registration-instrumentation branch from 0b3a5a4 to 8298d5e Compare May 8, 2024 22:21
@jimchamp jimchamp marked this pull request as ready for review May 8, 2024 22:23
@jimchamp jimchamp removed State: Blocked Work has stopped, waiting for something (Info, Dependent fix, etc. See comments). [managed] On testing.openlibrary.org This PR has been deployed to testing.openlibrary.org for testing labels May 8, 2024
@mekarpeles mekarpeles merged commit 46da379 into internetarchive:master May 9, 2024
7 checks passed
@mekarpeles mekarpeles added the Patch Deployed This PR has been deployed to production independently, outside of the regular deploy cycle. label May 15, 2024
@cdrini cdrini changed the title Instrument registration flow Add sentry tracking for registration endpoint/errors May 23, 2024
@jimchamp jimchamp deleted the 8863/registration-instrumentation branch May 30, 2024 19:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Patch Deployed This PR has been deployed to production independently, outside of the regular deploy cycle.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants