Will the circuit breaker trip if enough minor transient errors happen? Should frequency of exceptions or interval between exceptions be a factor?

Hi,

The circuit breaker doesn't handle the scenario where we get a small amount of errors over an extended period of time. The circuit breaker state keeps just a simple count and breaks for a hard set amount of time once it's seen the set number of exceptions.

Say we're using Polly to wrap a call an external web service that errors out once an hour. After three hours we'll lock up hard, even though we probably don't want to break the circuit in this scenario.

Does it make sense to have some sort of expiration on when an exception happens so that their effect dampens over time?

Agreed, I was thinking that you could specify another timespan that would reset the count if an exception hadn't occurred in that period of time.

Thoughts?

Good idea, however could you have some internal checks on the handle exception to get round it....? You can argue that to keep it simple, you should implement it yourself in this layer instead, instead of adding the override to the circuit breaker?

Tbh, I would like to see the self-implementation of circuit breaker fork merged instead if that is in-line with your design, as that sort of covers this, ie do it yourself in a custom implementation, and also allows us to handle circuit breaks in a central place across several out-of-process services........ Though maybe my solution to tims request is the same for me... Haven't thought it through fully yet...😏

@TimGebhardt

I'm not sure from @TimGebhardt's original question, if he was assuming there were successes in-between the failures across the three-hour period or not. My reading of the codebase was that the breaker state is Reset() after any successful call to the supplied action. (Have I got this right?). So, if [A] a service failed intermittently (with successes in-between) across three hours, the existing CircuitBreakerPolicy implementation wouldn't trip. If on the other hand [B] the service only received three calls across a three-hour-period and all failed, the existing CircuitBreakerPolicy would trip. (Seems to make sense - all calls have failed - but not sure how long one would choose to trip a circuit for on a service only averaging a call per hour?)

The question made me think tho about a frequency-based circuit-breaker (which Michael Nygard also hints at in his book). Something like: "If the policy experiences N exceptions in any period T, break the circuit for a given timespan" (eg "Break the circuit if we receive 3 exceptions in 30 seconds"). I have just firmed up a sketch I put together a few days ago (when first saw Tim's q) and committed to https://github.com/reisenberger/Polly/tree/FrequencyBasedCircuitBreaker - is this an avenue worth pursuing at all?

It essentially maintains a FIFO queue of the timestamps of the last N exceptions, and uses this quite simply to determine if N have occurred within the given timespan.

Or for an alternative mathematical averaging approach? (doesn't maintain a FIFO queue)
https://github.com/reisenberger/Polly/tree/FrequencyBasedCircuitBreaker2

(I can see merits and demerits to all of the alternatives ...)

Regardless of the mathematics, I think your approach to the problem is fine as it makes more sense to what you would want a circuit breaker to be in practice.

However, to refer back to the other part of my note, all of these circuit breakers are great for the simple one-process approach. IMO, the type of things you are circuit breaking (external services) are highly likely to be used across processes. Unless I've completely missed it, I don't think Polly handles this scenario.

So, should Polly implement this ? (I think not, as the backing store to hold this data, and the appropriate trigger logic for the breaker, is probably very system specific) but I'm not sure of the best way to go about implemented this outside of Polly and injecting the functionality into it's fluent based framework...

@bunceg

Hi @bunceg . If I understand you rightly, the scenario is that if services A, B and C (say) all use another service M, then if service A breaks its circuit to M, it could be useful for services B and C to know about this and break their calls to service M too? We went through a similar line of thinking, but concluded - is there a need? If service A cannot reach service M for a reason that would also prevent service B, won't service B just discover that of its own accord (and also circuit-break) in due course? Is the overhead of inter-process co-ordination (which might also fail) worth it? (And A could also fail to reach M for a reason which doesn't affect B ...)

We did, however, go through a similar line of thinking - that it could be useful to be able to inject state (as you say) into a CircuitBreaker - for manually breaking a circuit. Sam Newman's (highly recommended) book on microservices suggests one might manually break all circuits to a downstream service in order to upgrade that downstream service. I fretted for a while over the fact that we couldn't inject state into a Polly CircuitBreaker to achieve this. For now, we have however gone with the same pragmatic above logic - do we need to? If we build our processes robust enough to be able to survive downstream unavailability (whether unexpected or 'expected' eg due to upgrade), then that should be enough. But @michael-wolfenden - could being able to manually break a circuit (inject state) perhaps be a useful addition sometime?

HI,

Yes you do understand, and we are following the same principle in that A, B or C will find out of their own accord eventually. I take on board your comment about the co-ordinator could fail, which is a real concern to our situation too, but IMO that is a decision for the implementing team based on the services involved and the consumers of those services.

However, your approach of injecting state may well solve the problem anyway - something else can check the shared state first and manually break the circuit and I like the way of doing this. Our implementation uses a master switch to avoid calling the service anyway but your option sounds a bit cleaner.

ps: thanks for the book recommendation, I'll add it to my collection with "release it!" :)

How about using a MemoryCache for success/failure rates by default, and getting a failure ratio from that? We prefer to use ratios rather than hard numbers (and likewise, ratios when the circuit is in the half-open state). Then just a simple x% of requests in n minutes have failed, trip it. TTL on the cache can easily give you the n minutes automatically.

Also agree re not having a centralised way to notifying other services about failures. What if the connection from just Serivce A -> M goes down/degrades, but the rest are OK? Now you're telling services B & C that they can't talk to M even though they can. Dangerous and lots of code/systems overhead. They'll figure it out.

@reisenberger

Hi @reisenberger. Thanks to the team for developing Polly. It is a nice library.

In regards to this issue, I concur it would be nice with a feature that would allow the circuit to break if N exceptions occur over a period of T.

One example, where I would like a circuit to break is in the case, where a client is communicating with a server that is overloaded. The server would once in a while successfully handle a request, but most of the time return timeouts (As it will not be able to handle a request within the alloted time). Breaking the circuit for a little while, might help the server get it self up again.

Another example is the same as @TimGebhardt talked about where, during slow periods (Maybe nighttime), a single request is being retried and failing every once in a while, which breaks the circuit and can keep the circuit open, when good requests begin to filter in.

I looked at both implementations you made and think that both would solve the two examples above. The first implementation is easier to read. The second implementation requires are bit more thought to convience that there isn't an outlying case that could result in improper behavior.

Is this a feature that the team wants into Polly?
Anything I can help with?

@kristianhald

@kristianhald Thank you for your interest in Polly - and offer of help!

More sophisticated approaches to circuit-breaking are very much on the Polly roadmap (see towards end of doc) (for reasons TimGebhardt and you mention, and others)

Right now AppvNext are reflecting further on the best forward path for circuit-breaker: whether to go for the frequency-based and proportion-based circuit-breakers already mooted in the roadmap, and/or whether something more sophisticated along the lines of Hystrix’s time-slicing approach (somewhat similar to the suggestion from @DarrellMozingo above). Community views?

(may set out further views/options for feedback, later in this thread)

@kristianhald

( and @kristianhald - we may yet take you up on your offer of help! )

@reisenberger

Looked through the source for Hysterix regarding their Circuit Breaker implementation. As I read it, they use a bucket, where each item is a slice of the '_durationOfExceptionRelevance'. The precision is then defined by the size of the slice.

I think that the 'Frequency-based' solution is the same that Hysterix has implemented (Except that they look at percentages instead of the total number within the frequency given). The hubris in me, would state that the first implementation that @reisenberger has made with the queue of exception times, is time slicing where the window is very small and each window only contains a single information that an error occurred. 😃

I also like that the Hysterix implementation, before triggering the circuit breaker, checks if the number of requests received during the period is above a certain threshold (This is probably necessary, as else a single error will have a large impact on the error percentage, if the throughput is low).

// check if we are past the statisticalWindowVolumeThreshold
if (health.getTotalRequests() < properties.circuitBreakerRequestVolumeThreshold().get()) {
    // we are not past the minimum volume threshold for the statisticalWindow so we'll return false immediately and not calculate anything
    return false;
}

Personally, the few times I have used circuit breaker, it has mostly been leaning towards the frequency based. It is probably a personal preference, because I always like comparing stuff that happens with when they happened.

I now feel relatively clear on the effects of setting different parameters near boundaries, as follows.

I agree with every variable comment.

To take the documentation away from too abstract discussion, I may add a 'suggested typical starting configuration' for downstream network calls.

I looked at the draft documentation again and the part about configuration recommendations for the sampling duration requires some thought to it. I think that beginning the documentation with suggested configuration, which can allow the user to quickly be up an running and then having more detailed information later is a very good way of going 😸

@kristianhald

@kristianhald Re:

updated my fork with some additional features

Thank you for this extremely productive contribution! Hoping to review and then we can pull this together for release very shortly (keen altogether to get this feature out this week or next, due to other commitments) (just need find time to review 😄 ).

@kristianhald

@kristianhald

the part about configuration recommendations for the sampling duration requires some thought [...]
beginning [...] with suggested configuration [...] then more detailed information later

Completely agree! Will adjust ...

@kristianhald

@kristianhald Merged your work on RollingHealthMetrics to my local fork. Thanks for the great contribution!

@kristianhald @joelhulen Remaining to do (on my side) before this ready to PR against master:

minor tidies
rename TimesliceCircuitController etc to preferred terminology
remove breaker implementations (proportion, frequency) not currently intended for release
add readme.md doco [brief]
yaml etc

Hopefully in the next day or so ...

@kristianhald

@kristianhald To offer some commentary on final decisions taken on previous points you raised:

At the moment the decision on using [RollingHealthMetrics and SingleHealthMetrics]
is happening in the controller

Left this where you had it. Encapsulates the concern.

on my machine there goes about '10000' ticks per millisecond

See const TimeSpan.TicksPerMillisecond

after a bit of thought, I came to the conclusion that it did not make sense to open the circuit
on a success even if the failureThreshold has been reached

I reviewed and agree with this. Breaking on a success seems counterintuitive. The circuit may have 100% recovered for now (sssssssssss...), in which case breaking would be counterproductive. If the circuit hasn't 100% recovered and is still intermitting (ssfsfsfsfsf), circuit will receive a failure soon enough within the same period, to break on.

@kristianhald Please feel free to review the final changes to https://github.com/reisenberger/Polly/tree/CircuitBreakerSpikeMarch16 if you have an interest. I intend to PR this to master later today.

See const TimeSpan.TicksPerMillisecond

Ahh, did not know that the ticks per millisecond for DateTime and TimeSpan is constant. Nice to know.
Normally ticks per millisecond is defined by the CPU and OS based on the frequency they work on, which is why I provided the number of ticks 😄

Please feel free to review the final changes to https://github.com/reisenberger/Polly/tree/CircuitBreakerSpikeMarch16 if you have an interest

Did a quick lookthrough the changes and I think they are good.

I intend to PR this to master later today

Cool. Out of curiosity, what is the procedure for nuget packaging the master branch?

@joelhulen

what is the procedure for nuget packaging the master branch?

Merging to App-vNext/master (and earlier, creating the PR against it) automatically runs an AppVeyor verification build. We could push packages to nuget automatically on merging any PR, but opt to push them to Nuget manually. (Manual gives us the ability to occasionally merge PRs (eg readme doco fixes) without pushing a package and bumping rev number.)

@joelhulen owns this process, and can correct me if any of that is wrong / out of date.

@kristianhald

@kristianhald PR to master in process. Thanks for your awesome contributions to this feature in code and thought.

@kristianhald

@kristianhald Yes, @reisenberger has it right. The AppVeyor build process generates release-level nuget packages when we merge a PR. We control the version number via a file named GitVersionConfig.yaml, which is used for applying the release version for the signed and unsigned versions of the packages. We also update the Polly.nuspec file to include the change notes that are added as part of the package, as well as on the nuget.org site. Once the files are generated by the build server as artifacts, I manually upload them to nuget.org. As Dylan rightly states, this is to control the deployment of the packages.

@kristianhald

A further post to this closed issue just to document an issue that was discussed between @kristianhald and @reisenberger during merging down on to @reisenberger's fork. This post copies parts of the discussion from there (reisenberger#1) in case that is ever lost:

[The issue has no bearing on the operation of the v4.2 AdvancedCircuitBreaker (proved by unit tests), but would come into play if the Polly AdvancedCircuitBreaker were to want to evolve to emit health statistics at regular intervals.]

@reisenberger wrote:

Hey @kristianhald. Here is a conceptual issue. I observe that, from our implementation, the
windows we create within the timeslice are not necessarily adjacent. For instance, imagining
windows of size 1, if the first action records at t = 0, and the second action at t = 1.5, we will be
running two windows from 0->1, and 1.5->2.5, with a 0.5 gap in between them. Reasoning
suggests this has no effect on the statistics, as the gaps only represent 'empty' times with no
results.

I considered an outside case: imagining windows of size 1 in slice size 10, we could have a window
at t = 0 and another at t = 9.9, this latter in theory extending +1 to t = 10.9 ... Again, reasoning
suggests that if any statistic comes in at t >= 10, the t = 0 window will be dropped, so at no point
will we be considering statistics for a period >10. However, it might be nice (for regression value, in
case anyone refactors...) to have a test proving this.

It looks like this could be done by duplicating the test
Should_not_open_circuit_if_failure_threshold_exceeded_but_throughput_threshold_not_met_before_timeslice_expires__even_if_timeslice_expires_only_exactly()
and adding SystemClock.UtcNow = () => time.Add(timesliceDuration.Subtract(TimeSpan.FromTicks(1))); just before the third exception?
Would this cover it? It should cause it to create a new bucket which overlaps into the following
timeslice ... but the t = 0 window should be dropped when the fourth exception comes in?

In general: Do you agree that the non-adjacent windows issue has no effect on statistics, or am I
missing something?

Perhaps controversially, my first instinct is not to code to make windows adjacent before merging. It
adds complication and doesn't add anything to solving the problem at hand. Following the mantra
'Keep the code as simple as possible to deliver the job needed', we don't need to correct for this,
and the current creation/disposal of window code reads intuitively. However, there would be future
scenarios (eg emitting regular statistics, as Hystrix does) where it would become important. And
interested in your thoughts...

@kristianhald :

@reisenberger Don't have such a test and I believe that you are correct that we should have one to
ensure that it works as expected. I think that the test would ensure the outer case is valid.
I will add it immediately.

[this test was added and proves correct operation of the current statistics]

@kristianhald :

I am under the impression, like you state, that as long as we remove buckets/windows that are
older than Now() - timesliceDuration then it will not have an impact. Also I felt that the
implementation would be smaller and easier to read.

Did a little thinking and it might actually be quite easy to add the logic for adjacent
buckets/windows.
If we need the buckets to align adjacent to each other, then I think that the easiest solution would
be to decide on a 'start time' (maybe construction time of the class or just zero) and when we
create a new bucket, we move the 'StartedAt' timestamp backwards in time to where the bucket
should have been created.

Lets take the following scenario:
Start time: Could be decided at constructor time or just be zero (Beginning of the universe
😸). Lets say it is 23 seconds (from when the universe began).
Bucket/Window duration: 0.7 seconds

At the current moment in time, DateTime.Now() reads 26.457 seconds and an error occurs.
From 23 seconds and forward until 26.457 the buckets StartedAt reads:
23, 23.7, 24.4, 25.1, 25.8, 26.5

As the 26.5 bucket is in the future, then this cannot be created. Therefore the bucket we are in is
25.8. The question is then, how do we get from 26.457 to 25.8 without having to create all buckets
from 23 seconds. The answer (I think 😄. Just throwing a quick idea here) is the following
calculation: StartedAt = Now() - ((Now() - StartTime) MOD BucketDuration)

If we take the above numbers we should get:
StartedAt = 26.457 - ((26.457 - 23) MOD 0.7)
= 26.457 - (3.457 MOD 0.7)
= 26.457 - 0.657
= 25.8

It will still have empty buckets as holes, but it should align the buckets to each other.

Question: Would aligning the buckets actually solve the statistics issue?

eg emitting regular statistics, as Hystrix does

When you say 'regular' do you mean bucket/window duration regular, every 1 second regular or a
delegate call, when an event occurs (Success or Error)?

@reisenberger:

At regular intervals (every call = too frequent). But the frequency of emitting these statistics would
absolutely be aligned to either timesliceDuration or windowDuration. Definitely don't want some
different system of timing to interact with ;~) Vague memory, Hystrix emits stats at one of these
intervals I think.

Options for adding this (later) to Polly:

+emit a HealthMetric when dequeueing it from front of the queue;
OR (more timely, fractionally more code)
+emit a metric when it ceases being the _current

.

This essentially documents the issue, tho there is further implementation discussion at
reisenberger#1.

Implementation would be as @kristianhald suggested with:
StartedAt = Now() - ((Now() - StartTime) MOD BucketDuration)

An alternative could be:
StartedAt = Now() - ((Now() - StartTimeOfLastBucket) MOD BucketDuration)
but this opens up complications across a circuit reset, when there isn't a last bucket.

Noting against this closed issue for future reference, possible optimisations that could be made in the AdvancedCircuitBreaker implementation in future:

Any optimisation decisions should be driven by need, and by running before-and-after performance stats, for example with the test harness here: https://gist.github.com/reisenberger/92dc8d73b4df127b296ed8daee3ed93d

Current performance:

Performance analysis of the AdvancedCircuitBreaker across one-hundred-thousand (100000) iterations, including frequent circuit-breaking, suggests introducing the AdvancedCircuitBreaker costs around 30 ticks (3 millionths of a second) per call. Performance for the original Polly CircuitBreaker broadly matches.

Possible optimisations:

[a] Replacing the use of Queue with an internal fixed-size array we manage ourselves. While this has little benefit in itself (the implementation of Queue is backed by a similar array...), it may open up [b].

[b] Avoid the generation (and subsequent garbage-collection once out of scope) of HealthCount instances. This could be done by retaining a fixed set of instances ourselves, in an array [a] we own, and blanking-and-reusing existing instances rather than letting them go out of scope and creating new.

[c] Maintain a running total of successes/failures across the timeslice, rather than re-summing it each failure (to be checked by performance analysis whether this optimises or not; it’s swings and roundabouts; the change would add a ++ to each success/fail; avoids larger summing each fail)

EDIT: As stated, it would have to be measured whether these optimisations add value. However, they are all items which Hystrix optimise away for their high throughput implementation.

Navigation Menu

Repository navigation

Will the circuit breaker trip if enough minor transient errors happen? Should frequency of exceptions or interval between exceptions be a factor? #41

26 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Will the circuit breaker trip if enough minor transient errors happen? Should frequency of exceptions or interval between exceptions be a factor? #41

Description

Activity

michael-wolfenden commented on Aug 9, 2015

bunceg commented on Aug 9, 2015

reisenberger commented on Aug 9, 2015

reisenberger commented on Aug 9, 2015

bunceg commented on Aug 9, 2015

reisenberger commented on Aug 9, 2015

bunceg commented on Aug 10, 2015

DarrellMozingo commented on Aug 25, 2015

kristianhald commented on Mar 17, 2016

reisenberger commented on Mar 20, 2016

reisenberger commented on Mar 20, 2016

kristianhald commented on Mar 20, 2016

26 remaining items

kristianhald commented on Apr 12, 2016

reisenberger commented on Apr 12, 2016

reisenberger commented on Apr 12, 2016

reisenberger commented on Apr 13, 2016

reisenberger commented on Apr 14, 2016

kristianhald commented on Apr 14, 2016

kristianhald commented on Apr 14, 2016

reisenberger commented on Apr 14, 2016

reisenberger commented on Apr 14, 2016

joelhulen commented on Apr 14, 2016

reisenberger commented on Apr 15, 2016

reisenberger commented on Apr 15, 2016

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions