Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Feature request: minimum request volume #232

Closed
arkwright opened this issue Oct 10, 2018 · 2 comments · May be fixed by #854
Closed

Feature request: minimum request volume #232

arkwright opened this issue Oct 10, 2018 · 2 comments · May be fixed by #854
Assignees
@lance
Labels
enhancement

Comments

@arkwright
Copy link

Hey, thanks for writing such an awesome library!

It would be really helpful if opossum could support a minimum request volume within the rolling window, below which the circuit would never open, even if 100% of requests result in errors. I am currently experiencing an issue wherein traffic occasionally drops to a trickle, and then the circuit opens because the first error after a long, silent period tips the statistics beyond the errorThresholdPercentage. I could increase the rollingCountTimeout, though this would effectively decrease the circuit breaker's sensitivity when traffic volume is sufficient.
@lance lance self-assigned this Oct 10, 2018
@lance
Copy link
Member

lance commented Oct 10, 2018

Hi @arkwright thanks for the suggestion and nice words. :) This is very similar to the options.warmUp property that was recently added. But I can see that it is different in that the warm up phase only happens when the application starts up. Your suggestion seems to encompass both a warm up phase, and an idle phase, so I need to give it some thought. I believe it should be possible to have a single property that handles both cases. I'd rather not have two separate code paths doing essentially the same thing.

Brainstorming a hack/workaround until I figure this out... Could your fallback function check stats.fires to see if it meets your threshold, and if it does not then call circuit.close()?

@arkwright
Copy link
Author

Hey @lance, thanks for giving this some thought!

I recently considered writing a fallback function which looks at stats.fires, and that would provide the functionality I'm looking for. I just felt that this would be such a common use case that it would make sense to expose it as a configuration option. circuit-breaker-js provides a volumeThreshold option, for example.

I'm not sure if warmUp could handle both cases, but I think the inverse API might work. If warmUp was deprecated and replaced with a volumeThreshold:

  1. At application start up, the circuit would remain closed until at least volumeThreshold requests have been sent, providing behavior similar to warmUp.
  2. During idle periods, the circuit would remain closed until volumeThreshold requests occur within the rolling window, providing the desired low volume protection.

Of course, if backward compatibility is a concern, these two options could coexist safely.

I'm not sure if that helps, but that's how I might approach it!

lance added a commit that referenced this issue Oct 21, 2018
@lance
feat: add options.volumeThreshold
601118d 
This option prevents the circuit from opening unless the
number of requests during the statistical window exceeds this
threshold.

Fixes: #232
@ghost ghost added the in progress label Oct 21, 2018
@lance lance mentioned this issue Oct 21, 2018
Merged
lance added a commit that referenced this issue Oct 21, 2018
@lance
feat: add options.volumeThreshold
f9a720e 
This option prevents the circuit from opening unless the
number of requests during the statistical window exceeds this
threshold.

Fixes: #232
@ghost ghost removed the in progress label Oct 21, 2018
@lholmquist lholmquist mentioned this issue May 1, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@lance lance

Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[Snyk] Upgrade eslint-plugin-n from 15.6.1 to 17.1.0 nodeshift/opossum
2 participants
@lance @arkwright