-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
ref(seer grouping): Use new CircuitBreaker
class for circuit breaking
#74563
ref(seer grouping): Use new CircuitBreaker
class for circuit breaking
#74563
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #74563 +/- ##
==========================================
+ Coverage 78.14% 78.16% +0.01%
==========================================
Files 6731 6731
Lines 300214 300257 +43
Branches 51642 51648 +6
==========================================
+ Hits 234608 234684 +76
+ Misses 59285 59250 -35
- Partials 6321 6323 +2
|
aa80763
to
e88dfb9
Compare
metrics.incr( | ||
"grouping.similarity.did_call_seer", | ||
sample_rate=1.0, | ||
tags={"call_made": False, "blocker": "circuit-breaker"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: You could use a centralized constant to keep track of all the reasons that could block
e.g.
class SEER_BLOCKING_REASONS(Enum):
CIRCUIT_BREAKER = "circuit-breaker"
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could, it's true. It'd have to be imported from utils
, since different versions of the metric are recorded there, too.
I'm not sure there's a huge advantage there if I'm only using each one once, though, is there?
6bd2c86
to
526edc4
Compare
e88dfb9
to
3762351
Compare
3762351
to
c286d0c
Compare
e0d7108
to
347a4b1
Compare
c286d0c
to
46f8edb
Compare
347a4b1
to
a6d8786
Compare
46f8edb
to
2328eb0
Compare
a6d8786
to
1713e4b
Compare
2328eb0
to
8186b35
Compare
8186b35
to
f7ac804
Compare
@@ -30,6 +36,11 @@ | |||
timeout=settings.SEER_GROUPING_TIMEOUT, | |||
) | |||
|
|||
seer_similarity_circuit_breaker = CircuitBreaker( | |||
SEER_SIMILARITY_CIRCUIT_BREAKER_KEY, | |||
options.get("seer.similarity.circuit-breaker-config"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
an options call at the module is only reflected at import time (and is always the default because the database connection isn't set up)
this is breaking mypy locally because the database isn't set up
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and is always the default because the database connection isn't set up
Oh, very good to know. Thanks for catching that! I'll switch it to initializing on the fly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah yeah -- thought it would've stuck from the last time
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, missed that - thanks for pointing it out!
PR reverted: 1bb6a50 |
This is a redo of #74563 (which got reverted), with the difference that the circuit breaker is now reinstantiated before each use, rather than being instantiated once at the module level. (The problem with that setup was that the module turns out to initialize before our database connections are established, meaning we only get the hardcoded default config rather than the one provided by the options automator.)
This is a redo of #74563 (which got reverted), with the difference that the circuit breaker is now reinstantiated before each use, rather than being instantiated once at the module level. (The problem with that setup was that the module turns out to initialize before our database connections are established, meaning we only get the hardcoded default config rather than the one provided by the options automator.)
The current implementation of circuit breaking for Seer similarity requests, using
with_circuit_breaker
, doesn't work because any errors the breaker might track are caught much farther down the call stack and never make it to the breaker. This fixes that by switching from usingwith_circuit_breaker
to using an instance of the newCircuitBreaker
class, which allows us to track the errors at a low level, before they're caught, while still using the breaker to decide whether to make the request at a much higher level.