danielfm
commented
2 years ago The failure which passes the fail threshold throws a CircuitBreakerError instead of the actual underlying exception. To me, this is semantically incorrect. A real error happened, and the circuit was not bypassed, so why are we showing a CircuitBreakerError?
It makes sense, although the current behavior might also be relied by many projects, so changing this behavior might fix your case, but might break others.
If you agree to allow this behavior to be customized via some option, and to keep the current behavior as-is, I don't see any problems with accepting a PR for improving this use case.
Cruuncher
This behavior I feel is incorrect.
The failure which passes the fail threshold throws a CircuitBreakerError instead of the actual underlying exception. To me, this is semantically incorrect. A real error happened, and the circuit was not bypassed, so why are we showing a CircuitBreakerError?
We are using this in cases of ReadTimeout and ConnectTimeout when connecting to external services, and which one is the case matters. We store the exception name in the DB, and we erroneously get CircuitBreakerError now when it was actually a ReadTimeout. This matters. The difference between a ReadTimeout and a CircuitBreakerError is the difference between an action that was potentially successful, and definitely not successful.
I've looked through the code and this is a fairly easy fix, which I can open a PR for, but I need to know if there's agreement on whether this is semantically the way it should behave or not.
At the very least it should be something like
CircuitBreakerPopped and CircuitBreakerOpen so that the cases of the breaker popping, and skipping operation are differentiable. Then in the CircuitBreakerPopped case, I can grab the next exception up on the stack for logging