Addition of streaming hystrix data. #41

lholmquist · 2017-03-31T15:36:21Z

DO NOT MERGE - this is a WIP for #39

I've added a Class, HystrixStats, which produces a stream of the hystrix metrics.

A circuit breaker will register with this class during the creation of a circuit breaker, https://github.com/lholmquist/opossum/blob/status-stream-data/lib/circuit.js#L101 so for every snapshot event emitted, the stats stream is updated.

I'm storing the instance of the circuit that is passed in, in a WeakMap so the hystrix event stream can get a handle on it to give me the circuits name, group, etc..

coveralls · 2017-03-31T15:39:55Z

Coverage decreased (-0.5%) to 98.374% when pulling 81412f1 on lholmquist:status-stream-data into 7bb3931 on bucharest-gold:master.

lance

I realize this is just your initial pass. Just providing some suggestions.

lance · 2017-04-01T00:47:04Z

lib/circuit.js

@@ -93,6 +94,9 @@ class CircuitBreaker extends EventEmitter {
    if (this.options.cache) {
      CACHE.set(this, undefined);
    }
+
+    // Register this instance of the circuit breaker with the hystrix stats listener
+    hystrixStats.register(this[STATUS]);


With my latest push, this should be this[STATUS].stats. That will return an Object that has the properties you can see here. It represents the cumulative data for each event over the entire window.

lance · 2017-04-01T00:47:49Z

lib/hystrix-formatter.js

+function hystrixFormatter (stats) {
+  const json = {};
+  json.type = 'HystrixCommand';
+  json.name = 'ola';


The circuit breaker now has a name property that you can use for this.

I'm not sure if i can get access to this information currently. Since the Status object emits the snapshot event, i only get the stats data during that event. maybe the Circuit class should emit the event(which i'm not sure about), or we add some metadata to the stats object that gets emitted during that snapshot event

lance · 2017-04-01T00:49:27Z

lib/hystrix-stats.js

+    const formattedStats = hystrixFormatter(stats);
+
+    // Need to take the stats and map them to the hystrix format
+    return cb(null, `data: ${JSON.stringify(formattedStats)}\n\n`);


Isn't hystrixFormatter already returning JSON?

it does, this is turning it into a string for the event stream

lance · 2017-04-01T00:51:05Z

lib/hystrix-stats.js

+  // Register the instance of a circuit to listen for a snapshot event
+  // during that event, call the hystrixSnapshotListener to do the stream stuff
+  register (circuitInstance) {
+    circuitInstance.on('snapshot', this._hystrixSnapshotListener.bind(this));


Currently it's the Status object that emits snapshot. That could change if you think it's better suited in CircuitBreaker, but I think it would be unnecessary indirection. It would mean that the circuit breaker listens to the status object for snapshot events and just propagates them to the client.

coveralls · 2017-04-03T15:06:37Z

Coverage decreased (-0.5%) to 98.367% when pulling 1284d9b on lholmquist:status-stream-data into 9ba4dfc on bucharest-gold:master.

coveralls · 2017-04-03T17:43:18Z

Coverage decreased (-0.6%) to 98.79% when pulling 6401e0b on lholmquist:status-stream-data into 3619678 on bucharest-gold:master.

coveralls · 2017-04-03T17:45:38Z

Coverage decreased (-0.6%) to 98.79% when pulling 6401e0b on lholmquist:status-stream-data into 3619678 on bucharest-gold:master.

lance · 2017-04-03T17:58:48Z

lib/hystrix-formatter.js

+  json.rollingCountFallbackMissing = 0;
+  json.rollingCountFallbackRejection = 0;
+  json.rollingCountFallbackSuccess = 0;
+  json.rollingCountResponsesFromCache = 0;


You can use circuit.stats.cacheHits here.

lance · 2017-04-03T18:00:37Z

lib/hystrix-formatter.js

+  json.latencyTotal = { '0': 0, '25': 0, '50': 0, '75': 0, '90': 0, '95': 0, '99': 0, '99.5': 0, '100': 0 };
+  json.propertyValue_circuitBreakerRequestVolumeThreshold = 5;
+  json.propertyValue_circuitBreakerSleepWindowInMilliseconds = 5000;
+  json.propertyValue_circuitBreakerErrorThresholdPercentage = 50;


You should use circuit.options to get this value as well as the previous two

that last property i think i found in the options, but what would those other 2 be, the rollingCountTimeout and buckets?

@lholmquist circuitBreakerSleepWindowInMilliseconds should be our options.resetTimeout. I guess we don't measure request volume, so we don't have a property for it. :)

lance · 2017-04-03T18:01:27Z

lib/hystrix-formatter.js

+  json.propertyValue_circuitBreakerRequestVolumeThreshold = 5;
+  json.propertyValue_circuitBreakerSleepWindowInMilliseconds = 5000;
+  json.propertyValue_circuitBreakerErrorThresholdPercentage = 50;
+  json.propertyValue_circuitBreakerForceOpen = false;


We have functions to force open/closed a circuit. Maybe we should add an issue for the circuit to keep track of this so it can be reported.

that sounds like a good idea

i've created an issue #46

lance · 2017-04-03T18:02:58Z

lib/hystrix-formatter.js

+  json.propertyValue_circuitBreakerErrorThresholdPercentage = 50;
+  json.propertyValue_circuitBreakerForceOpen = false;
+  json.propertyValue_circuitBreakerForceClosed = false;
+  json.propertyValue_circuitBreakerEnabled = true;


Use circuit.closed here, maybe? I'm not sure if this stat is supposed to reflect the current state of the circuit breaker, or if with Hystrix, a user could enable/disable a circuit.

looking at what the brakes library is doing for this value, they have it hard coded to true and have a comment about it being "not reported"

I can check the java lib to see what they do with it

looking at the java lib, its here: https://github.com/Netflix/Hystrix/blob/master/hystrix-core/src/main/java/com/netflix/hystrix/HystrixCommandProperties.java#L69

The comment is "// Whether circuit breaker should be enabled.", so this makes me think it doesn't have to do with the circuits closed/open state, but if the circuit breaker should be used or not

Makes sense

lance · 2017-04-03T18:03:38Z

lib/hystrix-formatter.js

+  json.propertyValue_circuitBreakerEnabled = true;
+  json.propertyValue_executionIsolationStrategy = 'THREAD';
+  json.propertyValue_executionIsolationThreadTimeoutInMilliseconds = 300;
+  json.propertyValue_executionTimeoutInMilliseconds = 300;


Use the circuit's options here as well.

lance · 2017-04-03T18:04:02Z

lib/hystrix-formatter.js

+  json.propertyValue_executionIsolationSemaphoreMaxConcurrentRequests = 10;
+  json.propertyValue_fallbackIsolationSemaphoreMaxConcurrentRequests = 10;
+  json.propertyValue_metricsRollingStatisticalWindowInMilliseconds = 10000;
+  json.propertyValue_requestCacheEnabled = true;


And here :)

lance · 2017-04-03T18:07:12Z

lib/hystrix-stats.js

+
+  // Register the instance of a circuit stats to listen for a snapshot event
+  // during that event, call the hystrixSnapshotListener to do the stream stuff
+  register (circuitInstance) {


Is there any reason to have this separated out? Couldn't the user just pass the circuit to the constructor, and this code could be executed there, no?

currently the class is exported as a new instance, so you wouldn't be able to pass it along since the constructor is called while you require it.

I think i do like that better though, having the CircuitBreakers constructor calling new HystrixStats() and registering then, let me change that

coveralls · 2017-04-03T20:26:02Z

Coverage decreased (-0.6%) to 98.79% when pulling 4a3b7f7 on lholmquist:status-stream-data into 3619678 on bucharest-gold:master.

coveralls · 2017-04-04T19:07:57Z

Coverage decreased (-0.6%) to 98.79% when pulling 241509e on lholmquist:status-stream-data into 3619678 on bucharest-gold:master.

coveralls · 2017-04-05T14:00:36Z

Coverage decreased (-0.6%) to 98.79% when pulling 48bfb97 on lholmquist:status-stream-data into 3619678 on bucharest-gold:master.

coveralls · 2017-04-05T14:23:19Z

Coverage decreased (-0.2%) to 99.194% when pulling 9ef1339 on lholmquist:status-stream-data into 3619678 on bucharest-gold:master.

coveralls · 2017-04-05T14:23:19Z

Coverage decreased (-0.2%) to 99.194% when pulling 9ef1339 on lholmquist:status-stream-data into 3619678 on bucharest-gold:master.

coveralls · 2017-04-05T15:33:27Z

Coverage decreased (-0.2%) to 99.194% when pulling 9f883a8 on lholmquist:status-stream-data into 3619678 on bucharest-gold:master.

coveralls · 2017-04-05T17:12:03Z

Coverage decreased (-0.2%) to 99.194% when pulling 3352783 on lholmquist:status-stream-data into 3619678 on bucharest-gold:master.

coveralls · 2017-04-05T17:18:34Z

Coverage decreased (-0.2%) to 99.194% when pulling c5c5261 on lholmquist:status-stream-data into 3619678 on bucharest-gold:master.

coveralls · 2017-04-05T17:25:39Z

Coverage decreased (-0.2%) to 99.194% when pulling 8080ddc on lholmquist:status-stream-data into 3619678 on bucharest-gold:master.

lance · 2017-04-05T19:08:51Z

README.md

@@ -231,3 +231,11 @@ you create the breaker. E.g.
 // Force opossum to use native JS promises
 const breaker = circuitBreaker(readFile, { Promise: Promise });


I'm debating removing Fidelity from opossum, fwiw.

A stream is now provided using the circuit.hystrixStats.getHystrixStream method. This stream can be easily turned into a SSE stream for use with a Hystrix Dashboard.

coveralls · 2017-04-05T19:20:30Z

Coverage decreased (-0.2%) to 99.194% when pulling fd8246a on lholmquist:status-stream-data into 3b3e309 on bucharest-gold:master.

lance reviewed Apr 1, 2017

View reviewed changes

lholmquist force-pushed the status-stream-data branch 2 times, most recently from a151bfd to 1284d9b Compare April 3, 2017 15:04

lholmquist force-pushed the status-stream-data branch 2 times, most recently from 235d027 to 6401e0b Compare April 3, 2017 17:39

lance requested changes Apr 3, 2017

View reviewed changes

lance added the in progress label Apr 4, 2017

lance assigned lholmquist Apr 4, 2017

lholmquist force-pushed the status-stream-data branch from c5c5261 to 8080ddc Compare April 5, 2017 17:22

lholmquist changed the title ~~[WIP] first pass at adding in streaming hystrix data. will squash later~~ Addition of streaming hystrix data. Apr 5, 2017

lholmquist added needs-review in progress and removed in progress needs-review labels Apr 5, 2017

lance reviewed Apr 5, 2017

View reviewed changes

lance approved these changes Apr 5, 2017

View reviewed changes

lholmquist added 2 commits April 5, 2017 15:18

feat: Addition of Hystrix Mertrics Stream. GH-ISSUE nodeshift#39

2d44df6

A stream is now provided using the circuit.hystrixStats.getHystrixStream method. This stream can be easily turned into a SSE stream for use with a Hystrix Dashboard.

feat: Add an example on how to use the Hystrix Metrics

fd8246a

lholmquist force-pushed the status-stream-data branch from 8080ddc to fd8246a Compare April 5, 2017 19:18

lholmquist merged commit fd8246a into nodeshift:master Apr 5, 2017

ghost removed the needs-review label Apr 5, 2017

		@@ -231,3 +231,11 @@ you create the breaker. E.g.
		// Force opossum to use native JS promises
		const breaker = circuitBreaker(readFile, { Promise: Promise });

Addition of streaming hystrix data. #41

Addition of streaming hystrix data. #41

Conversation

lholmquist commented Mar 31, 2017 • edited Loading

coveralls commented Mar 31, 2017 • edited Loading

lance left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Apr 3, 2017 • edited Loading

coveralls commented Apr 3, 2017 • edited Loading

coveralls commented Apr 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Apr 3, 2017 • edited Loading

coveralls commented Apr 4, 2017 • edited Loading

coveralls commented Apr 5, 2017 • edited Loading

coveralls commented Apr 5, 2017

coveralls commented Apr 5, 2017

coveralls commented Apr 5, 2017 • edited Loading

coveralls commented Apr 5, 2017 • edited Loading

coveralls commented Apr 5, 2017 • edited Loading

coveralls commented Apr 5, 2017 • edited Loading

Choose a reason for hiding this comment

coveralls commented Apr 5, 2017 • edited Loading

lholmquist commented Mar 31, 2017 •

edited

Loading

coveralls commented Mar 31, 2017 •

edited

Loading

coveralls commented Apr 3, 2017 •

edited

Loading

coveralls commented Apr 3, 2017 •

edited

Loading

coveralls commented Apr 3, 2017 •

edited

Loading

coveralls commented Apr 3, 2017 •

edited

Loading

coveralls commented Apr 4, 2017 •

edited

Loading

coveralls commented Apr 5, 2017 •

edited

Loading

coveralls commented Apr 5, 2017 •

edited

Loading

coveralls commented Apr 5, 2017 •

edited

Loading

coveralls commented Apr 5, 2017 •

edited

Loading

coveralls commented Apr 5, 2017 •

edited

Loading

coveralls commented Apr 5, 2017 •

edited

Loading