-
Notifications
You must be signed in to change notification settings - Fork 78
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
#251 Health check circuit breaker #282
base: release/v3
Are you sure you want to change the base?
#251 Health check circuit breaker #282
Conversation
0e0cf3b
to
d1865a5
Compare
Update 2:
Update: Condition added to catch
|
de87cc0
to
5a0fc7b
Compare
b4b46b6
to
4be0dbe
Compare
@EtherZa excellent feature and nice proposed API. Agreed that in the future a consumer interceptor could also observe for failure rates and proactively switch off the circuit - that could be added later. I will be able to give it some review in 1.5 weeks as I am out now without my laptop. |
@zarusz No worries. I just hope that's a pleasure trip :) |
@@ -2,66 +2,122 @@ | |||
|
|||
public abstract class AbstractConsumer : IAsyncDisposable, IConsumerControl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we invert the responsibilities here?
As it is the SlimMessageBus.Host knows about circuit breakers while its an opt-in plugin. I sense a leakage of concerns here.
What if the circuit breaker plugin (and its background host task) could traverse the IMasterBus
and its child buses and Consumers to find the ones with matching tags and either start or stop them depending on the circuit status?
That way we contain the circuit breaker logic in the plugin assembly, and the .Host
layer is free of that logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I originally had a look at this approach, but if a circuit breaker traverses IMasterBus
as you suggest:
- it will be ignorant to any other status changes made by other workflows and then may unwittingly reverse an intended configuration change
- be side stepped by toggling the
IConsumerControl
methods directly viaMessageBusBase.Consumers
(unable to start with circuit breakers already in the loop) - require a centralised co-ordinator to aggregate state for all circuit breakers (shifting complexity but still resulting in an inferior experience)
The alternate approach that I investigated was to create a shim/proxy that intercepts the Consumers
property in MessageBusBase
. It could then trip/open the circuits as required, but would need to change the collection to be that of IConsumerControl
so that a proxy could intercept the methods. This approach then cuts off all access to the underlying consumers.
ie.
protected void AddConsumer(AbstractConsumer consumer) => _consumers.Add(new CircuitBreakerProxy(consumer));
In the end the supplied implementation rendered the best results at the expense of being more invasive than ideal, but please let me know if you have another approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel pretty strongly we need to go for externalization of this logic outside of the .Host
layer and into the plugin layer which needs to coordinate consumers and maintain the Started/Stopped state based on the respective circuit state (I see the plugin acting as an adapter).
If anything we could add a factory method to be able to wrap these consumers into some proxies (from the circuit plugin) - or another smart design.
I am less concerned that someone will use IConsumerControl
explicitly when using a circuit breaker. And even if so, then if the consumers were never started (or are stopped imperatively) the breaker logic could detect that and could withhold from starting such consumers if the breaker is on again.
The one thing that might get tricky with externalized logic is to maintain identity (or a pointer) to the consumer instance, but if anything we could (assign some identity to these consumers to find them easily by ID). However recalling vaguely the AbstractConsumer instances should remain (not being disposed) between Start/Stop cycles.
I am open to prototyping this logic on the side together if that helps express my intent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I'll need to dig into it a little bit, but I am not sure when I will be able to do so. I have some pretty tight deadlines over the next 3 months which are going to leave little time unfortunately.
I think that I have a fair grasp of what you are after, but there are some immediate issues that will need to be overcome as you have described:
MessageBusBase
does create/dispose the consumers between Start/Stop cycles.- There is no reference between configuration and consumer. I added this in the current PR but making it externally visible is not ideal as the objects are not immutable.
MessageBusHostedService
can be replaced to manage the running state, but due to 1, there is no interception point to stop a consumer from starting if the circuit has already been tripped.
4be0dbe
to
947df45
Compare
Quality Gate passedIssues Measures |
@EtherZa would it be possible to either:
|
947df45
to
e17fbf0
Compare
Signed-off-by: Richard Pringle <richardpringle@gmail.com>
e17fbf0
to
3ca9335
Compare
@zarusz I have rebased the change on |
Quality Gate passedIssues Measures |
Addition of a health check circuit breaker for consumers (
SlimMessageBus.Host.CircuitBreaker.HealthCheck
).Consumers can be optionally associated with one or more tags (and severities) in order to temporarily pause the consumer while a health check is failing.
The
HealthCheckBackgroundService
taps into the published health reports to distribute health status changes generated by theMicrosoft.Extensions.Diagnostics.HealthChecks
library. It is added as a hosted service to catch reports outside of the life time of theSlimMessageBus
.To add the functionality, a few questionable changes have been made. Please shout if you are not comfortable with any of them/would prefer an alternate implementation.
IEnumerable<AbstractConsumerSettings>
is required byAbstractConsumer's
constructor (retrieve associated circuit breaker configuration)AbstractConsumerSettings
maintains a reference to the associatedMessageBusSettings
(IServiceProvider
to create theIConsumerCircuitBreaker
instances and to retrieve the associated bus name for logging)AbstractConsumerBuilder
extendsIHasPostConfigurationActions
that will be executed by theMessageBusBuilder
pipeline (circuit breakers service registrations)A sample application has been included (
Sample.CircuitBreaker.HealthCheck
) to demonstrate the functionality with health checks that fail randomly.Ideas for future improvements
Although I have no plans on doing this now, the functionality could be extended by incorporating an
IConsumerInterceptor
to watch for exceptions and, on catching one, initiate an ad hoc health check (once per exception type/period/etc). The evaluation period could then be reduced from the standard health check publishing interval to a reactive check based on an application failure backed up by the default cadence.