chore(issues): Add LoggedRetry to log retry information #94840
Conversation
This gives us visibility into when and why retries are happening, currently we are only able to see that a set of requests (which may or may not have contained retries) succeeds or fails but not individual retry information. This also enables logged retries for seer requests.
github-actions
bot
added
the
Scope: Backend
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## master #94840 +/- ##
=======================================
Coverage 87.88% 87.88%
=======================================
Files 10441 10443 +2
Lines 603755 603814 +59
Branches 23505 23505
=======================================
+ Hits 530620 530688 +68
+ Misses 72768 72759 -9
Partials 367 367 |
| mock_logger.assert_has_calls( | ||
| [ | ||
| call.info( | ||
| "Request retried", |
There was a problem hiding this comment.
these logs are on top of other logs which log what actually happened on each attempt, right? and this just logs the state of the retry, with the actual failure getting logged however it gets logged
There was a problem hiding this comment.
Yep, these are purely additive
| if response is not None: | ||
| extra["response_status"] = response.status | ||
| if error is not None: | ||
| extra["error"] = error.__class__.__name__ |
There was a problem hiding this comment.
Is there a reason we're logging the error type but not the error message?
| @@ -65,14 +66,18 @@ def get_similarity_data_from_seer( | |||
| options.get("seer.similarity.circuit-breaker-config"), | |||
| ) | |||
|
|
|||
| retries: LoggedRetry | None = None | |||
There was a problem hiding this comment.
So is a Retry really a retry-er? As a naive reader here, it's odd that retries (plural) isn't either a list of something or a number of attempts which should be made.
There was a problem hiding this comment.
Yeah, it's a bit weird but I wanted to keep consistency with urllib3.util.retry.Retry, which LoggedRetry inherits from.
| @@ -183,7 +183,6 @@ def test_simple( | |||
| mock_seer_request.assert_called_with( | |||
| "POST", | |||
| SEER_SIMILAR_ISSUES_URL, | |||
| retries=options.get("seer.similarity.grouping-ingest-retries"), | |||
There was a problem hiding this comment.
I don't totally get how we're not passing this parameter anymore. Doesn't it just have a different value (either a LoggedRetry or None)?
There was a problem hiding this comment.
This was a bit of a cleanup, the option defaults to 0 (and not None) which was previously always being propagated in src/sentry/seer/similarity/similar_issues.py on line 75 (retries=options.get("seer.similarity.grouping-ingest-retries")). Now we only set retries if options.get("seer.similarity.grouping-ingest-retries") is truthy on line 70 (which 0 isn't).
At the end of the day the behavior is the same since POSTs default to 0 retries in urllib3 when retries is None.
This gives us visibility into when and why retries are happening. Currently we are only able to see that a set of requests (which may or may not have contained retries) succeeds or fails but not individual retry information.
This also enables logged retries for seer requests.