#1550 #1706 Addressing the QoS options ExceptionsAllowedBeforeBreaking issue

[ggnaegi]

Fixes #1550 #1706

• Circuit breaker not handling exception at end points even when ExceptionsAllowedBeforeBreaking is set. #1550
• Unstable test "Open circuit should not effect different route" #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:
#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]

Great job, Gui! 👍
You've got approval! ✅ 😄
If you don't mind, I will add a couple of commits...

[raman-m]

Note 1

[raman-m]

@ggnaegi
Could you send me a full list of ReSharper artifacts being added as standard settings by the tool to .NET solutions please?

I'll add them to ignore list...
In future, if someone will convert, or create std setup of ReSharper tools then we readd them and remove from .gitignore file.
But for now, I want to make sure that contributors don't change line-endings at all. This is critical settings for code review.

[raman-m]

What's your vision to fix the problem with line-endings in ReSharper, Rider?
I don't use these products because I am happy with Visual Studio 💯

[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!