fix(sentry app authorizations): Return token if already made for sentry #91507
Conversation
github-actions
bot
added
the
Scope: Backend
| except (SentryAppIntegratorError, SentryAppSentryError): | ||
| logger.info( | ||
| "refresher.context", | ||
| extra={ | ||
| "application_id": self.application.id, | ||
| "refresh_token": self.refresh_token[-4:], | ||
| }, |
There was a problem hiding this comment.
Taking these logs out as they were to debug a prior customer issue a while back
| @@ -106,7 +119,7 @@ def application(self) -> ApiApplication: | |||
| try: | |||
| return ApiApplication.objects.get(client_id=self.client_id) | |||
| except ApiApplication.DoesNotExist: | |||
| raise SentryAppIntegratorError( | |||
| raise SentryAppSentryError( | |||
There was a problem hiding this comment.
Making this consistent with _validator, this step could fail because either the user entered the wrong client id or our db actually borked. I leaned to the latter to be a bit more defensive but we can see later how common this issue is.
Codecov ReportAttention: Patch coverage is
|
| Files with missing lines | Patch % | Lines |
|---|---|---|
| src/sentry/sentry_apps/token_exchange/refresher.py | 92.30% | 1 Missing |
Additional details and impacted files
@@ Coverage Diff @@
## master #91507 +/- ##
========================================
Coverage 87.63% 87.63%
========================================
Files 10356 10349 -7
Lines 587133 587670 +537
Branches 22585 22653 +68
========================================
+ Hits 514506 515033 +527
- Misses 72199 72218 +19
+ Partials 428 419 -9 
| }, | ||
| token = self._create_new_token() | ||
| return token | ||
| except OperationalError as e: |
There was a problem hiding this comment.
Instead of catching an OperationalError here, I wonder if we should instead catch and raise this in the outbox_context decorator as a distinct error. Handling DB level concerns at the API layer seems like something we want to avoid if possible.
There was a problem hiding this comment.
I tried to do this via adding OutboxFlushError at the on_commit process since that lambda is responsible for all actions after the database has committed.
I was having trouble understanding what the test_outbox tests do & how to write one for the error handling changes. (I also didn't want to spend too much more time digging so I only made a test for refresher.py :bufo-hide:)
Lmk thoughts /
| try: | ||
| transaction.on_commit( | ||
| lambda: self.drain_shard(), using=router.db_for_write(type(self)) | ||
| ) | ||
| except DatabaseError as e: | ||
| raise OutboxFlushError( | ||
| f"Failed to process Outbox, {OutboxCategory(self.category).name} due to database error", | ||
| self, | ||
| ) from e |
There was a problem hiding this comment.
Should this try catch be inside drain_shard()? Looking at the other call sites of drain_shard(), all locations are already catching OutboxFlushError or they don't have a try/except block.
There was a problem hiding this comment.
+1, we discussed this via slack and came to the same conclusion. @markstory what do you think about using a distinct exception type for this. OutboxFlushError can be nebulous, but often indicates bad receiver/RPC logic, but maybe it's still the appropriate exception type to raise?
There was a problem hiding this comment.
ah yeah, sorry I didn't make the changes yesterday but yes we should be excepting within drain_shard since on_commit is only queueing up drain_shard. +1 to the question of what the purpose of OutboxFlushError is though, from when @GabeVillalobos and I talked yesterday it sounded like OutboxFlushError is used to indicate some implementer error. If so, I think it's appropriate if we make raise something else like OutboxDatabaseError.
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
Made from this issue (and thread) where if our outbox requests took too long or failed in some way, the authorizations endpoint would 500 and not return an
ApiTokenleaving users without a workingApiTokenand no way to get a new one