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

feat(utils): Add core CircuitBreaker functionality #74560

Merged
merged 4 commits into from
Jul 23, 2024

Conversation

lobsterkatie
Copy link
Member

This completes the work, started in #74557 and #74559, of adding a new, class-and-rate-limit-based circuit breaker implementation to the codebase. In this PR, the core record_error and should_allow_request methods are added to the CircuitBreaker class, along with accompaying tests.

@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Jul 19, 2024
@lobsterkatie lobsterkatie marked this pull request as ready for review July 19, 2024 15:54
Copy link

codecov bot commented Jul 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.15%. Comparing base (0d530a6) to head (1713e4b).
Report is 4 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #74560      +/-   ##
==========================================
+ Coverage   78.14%   78.15%   +0.01%     
==========================================
  Files        6731     6731              
  Lines      300214   300243      +29     
  Branches    51642    51646       +4     
==========================================
+ Hits       234608   234664      +56     
+ Misses      59285    59256      -29     
- Partials     6321     6323       +2     
Files Coverage Δ
src/sentry/utils/circuit_breaker2.py 97.61% <100.00%> (+30.60%) ⬆️

... and 11 files with indirect coverage changes

Comment on lines +253 to +256
try:
self._set_in_redis(
[
(self.broken_state_key, broken_state_expiry, broken_state_timeout),
(self.recovery_state_key, recovery_state_expiry, recovery_state_timeout),
]
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Having get and set separate might lead to some concurrency weirdness, but I think the worst that can happen is it gets set to BROKEN multiple times in the same moment, which is fine.

@lobsterkatie lobsterkatie force-pushed the kmclb-add-helpers-to-CircuitBreaker-class branch from 55205da to 20822f6 Compare July 23, 2024 00:48
@lobsterkatie lobsterkatie force-pushed the kmclb-add-core-CircuitBreaker-methods branch from 6bd2c86 to 526edc4 Compare July 23, 2024 00:48
@lobsterkatie lobsterkatie force-pushed the kmclb-add-helpers-to-CircuitBreaker-class branch from 9b4a941 to d5c9975 Compare July 23, 2024 06:17
@lobsterkatie lobsterkatie force-pushed the kmclb-add-core-CircuitBreaker-methods branch from e0d7108 to 347a4b1 Compare July 23, 2024 06:17
@lobsterkatie lobsterkatie requested a review from a team as a code owner July 23, 2024 06:18
@lobsterkatie lobsterkatie force-pushed the kmclb-add-core-CircuitBreaker-methods branch from 46f8edb to 347a4b1 Compare July 23, 2024 06:19
@lobsterkatie lobsterkatie force-pushed the kmclb-add-helpers-to-CircuitBreaker-class branch from 8609b63 to df575da Compare July 23, 2024 17:34
@lobsterkatie lobsterkatie force-pushed the kmclb-add-core-CircuitBreaker-methods branch from 347a4b1 to a6d8786 Compare July 23, 2024 17:36
lobsterkatie added a commit that referenced this pull request Jul 23, 2024
#74559)

This is a follow-up to #74557, which added the beginnings of a rate-limit-based circuit breaker, in the form of a new `CircuitBreaker` class. In this PR, various helpers, for checking the state of the breaker and the underlying rate limiters and for communicating with redis, have been added to the class. It also adds a `MockCircuitBreaker` subclass for use in tests, which contains a number of methods for mocking both circuit breaker and rate limiter state.

Note that though these helpers don't have accompanying tests, they are tested (indirectly) in the final PR in the series[1], as part of testing the methods which use them.

[1] #74560
Base automatically changed from kmclb-add-helpers-to-CircuitBreaker-class to master July 23, 2024 18:07
An error occurred while trying to automatically change base from kmclb-add-helpers-to-CircuitBreaker-class to master July 23, 2024 18:07
@lobsterkatie lobsterkatie force-pushed the kmclb-add-core-CircuitBreaker-methods branch from a6d8786 to 1713e4b Compare July 23, 2024 18:13
@lobsterkatie lobsterkatie merged commit b5ecab1 into master Jul 23, 2024
50 checks passed
@lobsterkatie lobsterkatie deleted the kmclb-add-core-CircuitBreaker-methods branch July 23, 2024 18:48
@github-actions github-actions bot locked and limited conversation to collaborators Aug 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Scope: Backend Automatically applied to PRs that change backend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants