#1550 #1706 Addressing the QoS options ExceptionsAllowedBeforeBreaking issue

[ggnaegi]

Fixes #1550 #1706

• 1550

• 1706

Proposed Changes

• We shouldn't create a Circuit Breaker per route / request, only maintain one Circuit Breaker per route
• The Circuit Breaker is Expecting an Exception to kick in, a response with status 500 is not an Exception. If status 500, then throwing.

[ggnaegi]

@raman-m @RaynaldM This PR is ready for review.

I have added several unit tests:

• Circuit breaker should build
• it contains only one policy if ExceptionsAllowedBeforeBreaking = 0
• it contains two policies of type AsyncCircuitBreakerPolicy<HttpResponseMessage> and AsyncTimeoutPolicy<HttpResponseMessage> if ExceptionsAllowedBeforeBreaking > 0
• should return same circuit breaker for given route
• should return different circuit breakers for two different routes
• should throw exception BrokenCircuitException<HttpResponseMessage> on third faulty attempt if ExceptionsAllowedBeforeBreaking = 2
• should not throw if status code is OK and not within 500...508 range
• should throw but after delay allow one new request
• should throw but atfer delay allow one faulty request and then throw again
• should throw but atfer delay allow one correct response and put counter back to zero acceptance tests:
• should open circuit breaker after two exceptions

Hopefully I have a solution for the possibly broken service, I have simplified the logic here: https://github.com/ThreeMammals/Ocelot/issues/1706

 private void GivenThereIsAPossiblyBrokenServiceRunningOn(string url, string responseBody)
    {
        var requestCount = 0;
        _serviceHandler.GivenThereIsAServiceRunningOn(url, async context =>
        {
            if (requestCount == 1)
            {
                await Task.Delay(1000);
            }

            requestCount++;
            context.Response.StatusCode = 200;
            await context.Response.WriteAsync(responseBody);
        });
    }

[raman-m]

Gui, would you like to include this fix in October release?

[ggnaegi]

Gui, would you like to include this fix in October release?

I think so yes.

[raman-m]

Gui, As you see, I've rebased feature branch... And I'll review tomorrow... Happy presentation tomorrow! 😉

[ggnaegi]

@raman-m are you sure that it's not a bit over-engineered? (FileConfiguration)

[raman-m]

@ggnaegi commented on Nov 3 a bit over-engineered?

Why not if I have some time for this... I agree that refactoring of tests doesn't provide much benefit. 🆗 I won't you bother on tests... Let's focus on real code...

[raman-m]

@ggnaegi Let me know you're 🆗 to merge plz? I'm going to add a couple of tiny fixes... So, it seems, this PR will be merged first than #1745 ... And you will need to apply new logger string-factories for all new logging operators being added by this PR.

[raman-m]

Regarding the Open_circuit_should_not_effect_different_route() test... Here are my testing results of 1000 runs for the test;

It is definitely stable now. But I don't know how to get CircleCI environment tests... Seems we have to watch for this test on CircleCI in the latest builds after merging PR...

Anyway, I would say, It should be stable now, and #1706 will not be reproducible after merging this PR. So, we could attach #1706 to this PR. Could we, Gui?

[ggnaegi]

Great!