Circuit breakers #566

bsimpson63 · 2021-02-26T18:41:07Z

This is largely copied from reddit/reddit-service-graphql/tree/master/graphql_api/lib/circuit_breaker. That code is great and running in production in a few service.

I added docstrings and types where missing and then added the CircuitBreakerClientWrapperFactory interface to hopefully make it a bit easier to integrate with baseplate service.

I'm going to copy/paste this code into thing service and r2 to ensure it works as expected and isn't missing anything. Then we can consider merging this PR into baseplate.

👓 @suzie-su @pnovotnak @spladug @manishapme @jennLam

no changes have been made yet!

suzie-su

good refactor 👍
just some questions.

suzie-su · 2021-02-26T19:28:00Z

baseplate/lib/circuit_breaker/factory.py

+
+    def make_object_for_context(self, name: str, span: Span) -> Any:
+        client = self.client_factory.make_object_for_context(name, span)
+


extra line?

suzie-su · 2021-02-26T19:47:47Z

baseplate/lib/circuit_breaker/factory.py

+    # they will be None and passed through to the Breaker() constructor
+    # which will override the defaults set in Breaker()
+    assert prefix.endswith(".")
+    parser = config.SpecParser(


what is difference between config.parse_config vs config.SpecParser

https://github.com/reddit/baseplate.py/blob/develop/baseplate/lib/config.py#L625-L635

parse_config is essentially SpecParser (returned by Parser.from_spec) and then parser.parse("", app_config), so it seems it doesn't let you specify the prefix.

but in the parser.parse{"", {prefix: }} can set the prefix in the dict right

suzie-su · 2021-02-26T19:53:52Z

baseplate/lib/circuit_breaker/factory.py

+
+    def __init__(self, client_factory: ContextFactory, breaker_box: "CircuitBreakerBox"):
+        self.client_factory = client_factory
+        self.breaker_box = breaker_box


so in this way, people will have to initialize the breaker_box in. init file and pass it in to factory?

yeah exactly. that's similar to how we do other clients in baseplate.py, and will make sure that all workers/greenlets share the same breakers.

pnovotnak · 2021-02-26T20:29:40Z

baseplate/lib/circuit_breaker/breaker.py

+        if not success:
+            self.failures += 1
+
+        if self._is_bucket_full and not self.results_bucket[0]:


🔕 can we grab this comment or put something like it here?

pnovotnak · 2021-02-26T20:32:15Z

baseplate/lib/circuit_breaker/breaker.py

+    :param trip_for: how long to remain tripped before resetting the breaker
+    :param fuzz_ratio: how much to randomly add/subtract to the trip_for time
+    """
+


🔕 My linter complains about variables initialized outside init, feel free to disregard

Suggested change

failures: int

yeah that's a little funky that it's only initialized in reset()(which is called from the constructor), but it does prevent some duplication. I'm leaning towards leaving it as is because this is copied exactly from the gql implementation.

it'd be good to add the type annotation just so everything knows it can expect the attribute to be there

pnovotnak

All in all I think this is good and I like the ergonomics. Stoked to have this in baseplate! Thank you for putting in the time, Brian

bsimpson63 · 2021-02-26T21:56:29Z

Note from Manisha:

only request is some docs that show how you set it up in the tests in the docstring somewhere. like add this to your init.py file

breaker_box = breaker_box_from_config(
            app_config={
                "brkr.samples": "4",
                "brkr.trip_failure_ratio": "0.75",
                "brkr.trip_for": "1 minute",
                "brkr.fuzz_ratio": "0.1",
            }
wrapped_client_factory = CircuitBreakerClientWrapperFactory(
            client_factory, self.breaker_box
        )
baseplate.add_to_context("wrapped_client", wrapped_client_factory)

spladug

super cool! i've left a few comments. also, it'd be good to add docs/api/baseplate/lib/circuit_breaker.rst to make this show up in docs. that's a great place to include example usage and any exposition on how it works. see the docs/api/baseplate/clients/ docs for some good examples.

spladug · 2021-03-01T17:50:55Z

baseplate/lib/circuit_breaker/breaker.py

+    :param trip_for: how long to remain tripped before resetting the breaker
+    :param fuzz_ratio: how much to randomly add/subtract to the trip_for time
+    """
+


it'd be good to add the type annotation just so everything knows it can expect the attribute to be there

spladug · 2021-03-01T17:51:45Z

baseplate/lib/circuit_breaker/breaker.py

+    WORKING = "working"
+    TRIPPED = "tripped"
+    # trip immediately after failure
+    TESTING = "testing"


given the sequence of states described below, it's a bit weird that testing is after tripped in this enum

spladug · 2021-03-01T17:56:08Z

baseplate/lib/circuit_breaker/breaker.py

+        self.name = name
+        self.samples = samples
+        self.results_bucket: Deque = deque([], self.samples)
+        self.tripped_until: datetime = datetime.utcnow()


🔕 would time.monotonic() be better for this? much less baggage and no worries about the clock shifting under us

also happens to be a bit faster

$ python3 -m timeit -s 'import datetime' -- 'datetime.datetime.utcnow()' 2000000 loops, best of 5: 150 nsec per loop $ python3 -m timeit -s 'import time' -- 'time.monotonic()' 5000000 loops, best of 5: 60.8 nsec per loop

spladug · 2021-03-01T18:04:04Z

baseplate/lib/circuit_breaker/factory.py

+
+
+logger = logging.getLogger(__name__)
+


It'd be nice to add a config.Parser subclass like https://github.com/reddit/baseplate.py/blob/develop/baseplate/clients/thrift.py#L24-L44 here so this circuit breaker can be used with baseplate.configure_context()

spladug · 2021-03-01T18:10:50Z

tests/integration/circuit_breaker_tests.py

+
+        breaker = self.breaker_box.breaker_box["get_something"]
+        assert breaker.name == "test_breaker.get_something"
+        assert breaker.results_bucket == deque([True, True, False, False])


the dequeue is an implementation detail, right? if we assert the exact internal state of the breaker like this it means we have to change all these tests if we change the implementation at all. it'd be less brittle to only test the externally visible state.

curioussavage · 2021-03-28T17:50:45Z

baseplate/lib/circuit_breaker/factory.py

+logger = logging.getLogger(__name__)
+
+
+class CircuitBreakerClientWrapperFactory(ContextFactory):


Should you also be exposing a report_runtime_metrics that calls one on the client_factory if it exists? Or is the idea that you would still attach the bare client factory to the context?

curioussavage · 2021-03-28T18:07:36Z

baseplate/lib/circuit_breaker/factory.py

+        return CircuitBreakerWrappedClient(span, self.breaker_box, client)
+
+
+class CircuitBreakerWrappedClient:


Either way may be ideally out of scope for this PR but it could be nice to have a ThriftCircuitBreakerWrappedClient that acts like a proxy class and just decorates the thrift interface functions with the breaker_context logic.

I wrote a little implementation in our codebase for this, works fine. I wonder if a patch to integrate a circuit breaker with the thrift client would be accepted after this merges.

Also, since retry logic can cause N requests to be sent before it lets the last exception bubble up to the breaker any approach not integrated with the thrift client doesn't really have an accurate picture of failures.

manishapme · 2021-06-02T21:19:39Z

🏃‍♂️

bsimpson63 added 2 commits February 26, 2021 10:23

(squash) Copy code from graphql

5d34fda

no changes have been made yet!

Add circuit breakers

53f7105

bsimpson63 requested a review from a team as a code owner February 26, 2021 18:41

suzie-su reviewed Feb 26, 2021

View reviewed changes

pnovotnak reviewed Feb 26, 2021

View reviewed changes

pnovotnak approved these changes Feb 26, 2021

View reviewed changes

suzie-su approved these changes Feb 26, 2021

View reviewed changes

spladug suggested changes Mar 1, 2021

View reviewed changes

curioussavage reviewed Mar 28, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Circuit breakers #566

Circuit breakers #566

bsimpson63 commented Feb 26, 2021 •

edited by spladug

Loading

suzie-su left a comment

suzie-su Feb 26, 2021

suzie-su Feb 26, 2021

bsimpson63 Feb 26, 2021

suzie-su Feb 26, 2021

suzie-su Feb 26, 2021

bsimpson63 Feb 26, 2021

pnovotnak Feb 26, 2021

pnovotnak Feb 26, 2021

bsimpson63 Feb 26, 2021

spladug Mar 1, 2021

pnovotnak left a comment

bsimpson63 commented Feb 26, 2021

spladug left a comment

spladug Mar 1, 2021

spladug Mar 1, 2021

spladug Mar 1, 2021

spladug Mar 1, 2021

spladug Mar 1, 2021

curioussavage Mar 28, 2021

curioussavage Mar 28, 2021

curioussavage Mar 29, 2021 •

edited

Loading

manishapme commented Jun 2, 2021


		def make_object_for_context(self, name: str, span: Span) -> Any:
		client = self.client_factory.make_object_for_context(name, span)

		logger = logging.getLogger(__name__)


		class CircuitBreakerClientWrapperFactory(ContextFactory):

		return CircuitBreakerWrappedClient(span, self.breaker_box, client)


		class CircuitBreakerWrappedClient:

Circuit breakers #566

Are you sure you want to change the base?

Circuit breakers #566

Conversation

bsimpson63 commented Feb 26, 2021 • edited by spladug Loading

suzie-su left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pnovotnak left a comment

Choose a reason for hiding this comment

bsimpson63 commented Feb 26, 2021

spladug left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

curioussavage Mar 29, 2021 • edited Loading

Choose a reason for hiding this comment

manishapme commented Jun 2, 2021

bsimpson63 commented Feb 26, 2021 •

edited by spladug

Loading

curioussavage Mar 29, 2021 •

edited

Loading