feat: refactor Prometheus and Hystrix metrics

[lance]

Instead of continuing to add different metrics modules to the
circuit breaker API, we can extract the PrometheusMetrics and
HystrixStats classes as their own modules. All they need is a
handle to the circuit itself so they can listen for events and/or
monitor the circuit stats. This allows us to continue maintaining
things like Hystrics that are deprecated upstream, without polluting
our circuit breaker implementation.

Fixes: #347
Fixes: #343
Related: #342

[lance]

This is a work in progress. There are a couple of things to note about this change.

• It does not modify the API to provide a plugin interface. Instead, it removes API specific functions and properties related to any specific third party metrics tool - e.g. Prometheus and Hystrix.
• Prometheus and Hystrix metrics instances do not have to conform to any specific interface. It is up to the author, but we probably want to document some "good practices" - e.g. accept an array of circuits in the constructor.
• I believe that the functionality of the metrics implementations remains the same with one exception. ** Previously, the default Node.js metrics that prom-client gathers by default is now prefixed with opossum_ instead of circuit_${circuit.name}_. This probably makes more sense anyway, because the Node.js metrics maintained by prom-client are not specific to any individual circuit, but rather to the entire process which may have multiple circuits.
• Before landing - or at least before a new version is released with this capability - we should probably extract the PrometheusMetrics and HystrixStats modules into an opossum-metrics repository and have these be maintained and versioned independent of the opossum.

/cc @ttessarolo @Sumeshkumar

[ttessarolo]

@lance Just to share my view.

I've recently developed a Microservices Framework for NodeJs (to be open sourced when test and docs will be done...) thus I've seen in action how powerful are two concepts:

• pluggability
• hooks

Thus to give you just a raw example on what I see for opossum (that I use in my framework):

const circuitBreaker = require("opossum");

const breaker = circuitBreaker(hanlderFunc, breakerOpts);
breaker.plug(require("opossum-histryx-plugin"), {...options...});
breaker.plug(require("opossum-prometheus-plugin"), {...options...});
breaker.plug(require("opossum-redis-cache-plugin"), {...options...});

// Then you have hooks (existing hooks for circuit status are still there)
breaker.on("fallback", fallbackFunc);
breaker.on("stats-emit"); // opossum-histryx-plugin + opossum-prometheus-plugin are registered to this hook
breaker.on("beforeCache", beforeCacheFunc); // chek if to cache --> return false === noCache
breaker.on("cache"); // opossum-redis-cache-plugin is registered to this hook
breaker.on("afterCache", beforeCacheFunc); // verify cache

// Start the Circuit --> activate plugings
breaker.exec();

The only doubt is if you really need to let plug-ins to be registered (and configured) for every breaker instance or, instead, if you just want to provide a generic plugin registration for every breaker instace:

const circuitBreaker = require("opossum");

circuitBreaker.plug(require("opossum-histryx-plugin"), {...options...});
circuitBreaker.plug(require("opossum-prometheus-plugin"), {...options...});
circuitBreaker.plug(require("opossum-redis-cache-plugin"), {...options...});

The first option is more powerful, but maybe not so usefull.

That's very quickly my view.
As I said you can use npm Avvio to handle plug-in logic, and let plug-in register to hooks, cache handling included.

[lholmquist]

Before landing - or at least before a new version is released with this capability - we should probably extract the PrometheusMetrics and HystrixStats modules into an opossum-metrics repository and have these be maintained and versioned independent of the opossum.

Are you thinking these would be in the same repo(published as 1 module) or should be separately published modules in their own repos. I think i like the second option

[helio-frota]

Before landing - or at least before a new version is released with this capability - we should probably extract the PrometheusMetrics and HystrixStats modules into an opossum-metrics repository and have these be maintained and versioned independent of the opossum.

Sounds good.

Are you thinking these would be in the same repo(published as 1 module) or should be separately published modules in their own repos. I think i like the second option

+1

[lance]

@ttessarolo thanks for providing that insight. What I am having a hard time understanding is why the circuit needs to have any awareness of these plugins. The stats plugins such as Prometheus and Hystrix have everything they need by just listening for the snapshot event. What kind of behavior do you see the circuit actually taking care of other than just emitting the stats? What is the specific, tangible benefit of doing it the way you suggest?

[lholmquist]

I think this looks pretty good. I think the removal of the specific stat frameworks from the Circuit Breaker implementation code makes sense and by just passing in an array of circuits to the specific stats class should work well.

It would be nice to have an example of the new way of doing Prometheus/Hystrix in the Readme

I don't really have a strong opinion on if those stat modules should be broken out into their own modules(on npm) first or this PR land first. i guess it doesn't really matter as long as it happens before a new release

[ttessarolo]

What kind of behavior do you see the circuit actually taking care of other than just emitting the stats? What is the specific, tangible benefit of doing it the way you suggest?

Well, not an evident so clear tangible benefit. But for instance if you want to let devs implement a cache plugin (eg. using Redis) you want to be sure that the plugin is up-and-running before invoke it.
Plus we can share the plugin functionalities inside the contest of the handler function the eg. you can reference to redis inside the handler using this.redis.set() wher this is opossum itself.

And definitely opening opossum to plugin wide the development to scenarios not yet imagined.

[lholmquist]

umm, something crazy happened with the commit history

[lholmquist]

I think all this looks pretty good. Moving the stats out of the Circuit Breaker implementation makes sense.

So i think next steps here once this gets in, is to the remove each stat(Hystrix and Prometheus) and put them into their own module

[lance]

@lholmquist @helio-frota @danbev I have moved the prometheus and hystrix metrics into their own modules in the @nodeshift org. Can y'all give this one more looksee? I'm eager to merge it and get this over with! :)

[lance]

@ttessarolo I've decided to go ahead and keep this simpler for now and avoid the plugin pattern. I think that most or all of what you are asking for can be taken care of in user code. Additionally (though I have not tested it) doing it this way means that these metrics can be used on older versions of opossum, since no plugin knowledge is required.

[mcollina]

Do you think it would be possible to remove the global circuit set? This ended up causing memory leaks in tests several times.

How about moving the logic in the factory, and exposing the constructor as well?

[lance]

@mcollina yes I will do those things for 4.0 but to keep it clean I would like to do these as separate independent pull requests.

[mcollina]

Awesome, thanks!

LGTM