Downgrade some expected "already exists" errors to info-level logging (#958)#1075
Downgrade some expected "already exists" errors to info-level logging (#958)#1075markstos wants to merge 3 commits into
Conversation
…#958) For web/db apps, a common source of exceptions stem from users double-clicking some button or link to add a thing when it only needs a single click. In these cases, the first request will trigger a successful addition and the second one will trigger some kind of "already exists" condition, which is harmless to ignore. For some known cases like that, we downgrade them from throwing exceptions to info-level logging. Other cases of adding things where we still would not expect exceptions to be triggered reamin as exceptions.
MelissaAutumn
left a comment
MelissaAutumn
left a comment
There was a problem hiding this comment.
Good start, I'd like to see those private functions dropped and code moved inside our existing classes where it makes sense.
| sentry_sdk.capture_exception(ex) | ||
| if ex.is_already_exists and not can_register_with_username(partial_username): | ||
| logging.info( | ||
| 'Signup duplicate suppressed because a local username/address now exists (status=%s, error=%s)', |
There was a problem hiding this comment.
We should maybe avoid the old python formatting. fstrings are fine here.
There was a problem hiding this comment.
Good idea for consistency. I opened a PR to automatically enforce f-strings. #1088
| DELETE = 'delete' | ||
|
|
||
|
|
||
| def _keycloak_error_details(exc: RequestException) -> tuple[Optional[int], str, dict]: |
There was a problem hiding this comment.
This should really live inside an exception that we throw from a requestexception instead of as a misc private function.
Let's avoid creating these misc private functions if we can, it creates a lot of noise within a file. I think we can shove most of this code in the authentication.exceptions.KeycloakError class and just automagically handle the data as it comes in.
There was a problem hiding this comment.
I'll refactor to move the code into the error class and in-line a couple of the small single-use functions.
When you suggest to "avoid creating these misc private functions", perhaps you are referring to small single-use functions? I can try lean towards inlining code more often in those cases.
| POSTGRES_DUPLICATE_KEY_MARKER = 'duplicate key value violates unique constraint' | ||
|
|
||
|
|
||
| def is_already_exists_error( |
There was a problem hiding this comment.
This can live inside the KeycloakError class. No need for it to live in outside.
There was a problem hiding this comment.
The is_already_exists_error was put in the global scope with the understanding that the pattern of generating exceptions from double-click-to-insert affects all things we insert-- the exception we noticed related to keycloak happens to be the first one. As we scale, the pattern will repeat for everything we insert, see note below.
But in the spirit of YAGNI, I've moved it into the narrowest scope where it's actually used now, the ImportUserError class. Later when we hit similar cases and refactor to handle those, we can refactor to move this back to a shared location.
| return True | ||
|
|
||
| error_text = ' '.join(filter(None, [error_code, error_desc, error])).lower() | ||
| return POSTGRES_DUPLICATE_KEY_MARKER in error_text |
There was a problem hiding this comment.
Is this something we hit? I don't remember duplicate key errors here, but also I wasn't really looking at the response other than status code.
There was a problem hiding this comment.
That addition was experience-based rather than evidence-based. Having managed busy Postgres-backed websites in the past, at scale that became a common pattern: The user's double-click behavior would lead to a race condition to insert twice. One insert would win and the other would throw the "duplicate key" error.
We could remove it because we haven't hit it yet, but based on the scale we are trying to build we likely will. Also, it seems is the best solution: there's not a reasonable way to prevent the condition from happening: the best path seems to be accept that we'll trigger the duplicate key violation periodically, then explicitly accept this condition as OK in cases were we expect them to be safe, like a double-click for inserting record targeted here.
- Re-organize code - Remove necessary logging
|
Feedback has been addressed in a fixup commit that can be reviewed and then squashed during merging. |
2 participants
For web/db apps, a common source of exceptions stem from users double-clicking
some button or link to add a thing when it only needs a single click.
In these cases, the first request will trigger a successful addition and the second
one will trigger some kind of "already exists" condition, which is harmless to ignore.
For some known cases like that, we downgrade them from throwing exceptions to
info-level logging.
Other cases of adding things where we still would not expect exceptions to be
triggered reamin as exceptions.
Applicable Issues
QA Log
Sentry.