From 245d47b7bdaed73c6f79fd86df07005690a80486 Mon Sep 17 00:00:00 2001
From: Lance Ball <lball@redhat.com>
Date: Mon, 3 Apr 2017 10:39:58 -0400
Subject: [PATCH] feat: prefer an error percentage threshold

Until now, the circuit breaker could only be configured to trip after a
fixed number of failures. This is problematic, because those failures
may accumulate over a long period of time, causing the circuit to open
once the threshold has been hit - even if the failure percentage is
very, very low.

Now when using `options.maxFailures` a deprecation warning is printed to
the error console. The new option is `errorThresholdPercentage`.

Fixes: https://github.com/bucharest-gold/opossum/issues/37
---
 lib/circuit.js | 16 ++++++++++------
 test/test.js   | 12 ++++++++++++
 2 files changed, 22 insertions(+), 6 deletions(-)

diff --git a/lib/circuit.js b/lib/circuit.js
index 5e6cf49d..07fdf1c9 100644
--- a/lib/circuit.js
+++ b/lib/circuit.js
@@ -9,7 +9,6 @@ const CLOSED = Symbol('closed');
 const HALF_OPEN = Symbol('half-open');
 const PENDING_CLOSE = Symbol('pending-close');
 const FALLBACK_FUNCTION = Symbol('fallback');
-const NUM_FAILURES = Symbol('num-failures');
 const STATUS = Symbol('status');
 const NAME = Symbol('name');
 const GROUP = Symbol('group');
@@ -45,13 +44,13 @@ class CircuitBreaker extends EventEmitter {
     this.options = options;
     this.options.rollingCountTimeout = options.rollingCountTimeout || 10000;
     this.options.rollingCountBuckets = options.rollingCountBuckets || 10;
+    this.options.errorThresholdPercentage = 50;
     this.Promise = options.Promise;
 
     this[STATUS] = new Status(this.options);
     this[STATE] = CLOSED;
     this[FALLBACK_FUNCTION] = null;
     this[PENDING_CLOSE] = false;
-    this[NUM_FAILURES] = 0;
     this[NAME] = options.name || action.name || nextName();
     this[GROUP] = options.group || this[NAME];
 
@@ -59,6 +58,8 @@ class CircuitBreaker extends EventEmitter {
       this.action = _ => this.Promise.resolve(action);
     } else this.action = action;
 
+    if (options.maxFailures) console.error('options.maxFailures is deprecated. Please use options.errorThresholdPercentage');
+
     const increment = property => _ => this[STATUS].increment(property);
 
     this.on('success', increment('successes'));
@@ -102,7 +103,6 @@ class CircuitBreaker extends EventEmitter {
    * @fires CircuitBreaker#close
    */
   close () {
-    this[NUM_FAILURES] = 0;
     this[PENDING_CLOSE] = false;
     if (this[STATE] !== CLOSED) {
       this[STATE] = CLOSED;
@@ -329,16 +329,20 @@ function fallback (circuit, err, args) {
 
 function fail (circuit, err, args) {
   /**
-   * Emitted when the circuit breaker action fails,
+   * Emitted when the circuit breaker action fails
    * or when the circuit is fired while open.
    * @event CircuitBreaker#failure
    */
   circuit.emit('failure', err);
-  circuit[NUM_FAILURES] += 1;
 
-  if (circuit[NUM_FAILURES] >= circuit.options.maxFailures) {
+  // check stats to see if the circuit should be opened
+  const stats = circuit.stats;
+  const errorRate = stats.failures / stats.fires * 100;
+  if (errorRate > circuit.options.errorThresholdPercentage ||
+    circuit.options.maxFailures >= stats.failures) {
     circuit.open();
   }
+
   return circuit.Promise.reject.apply(null, [err]);
 }
 
diff --git a/test/test.js b/test/test.js
index 845bbc76..e66a1567 100644
--- a/test/test.js
+++ b/test/test.js
@@ -624,6 +624,18 @@ test('CircuitBreaker fallback as a CircuitBreaker', (t) => {
     .then(t.end);
 });
 
+test('options.maxFailures should be deprecated', (t) => {
+  const options = { maxFailures: 1 };
+  const originalLog = console.error;
+  console.error = (msg) => {
+    t.equals(msg, 'options.maxFailures is deprecated. Please use options.errorThresholdPercentage');
+    // restore console.error
+    console.error = originalLog;
+    t.end();
+  };
+  cb(passFail, options);
+});
+
 /**
  * Returns a promise that resolves if the parameter
  * 'x' evaluates to >= 0. Otherwise the returned promise fails.