feat: add option for user-provided EventEmitter to control buckets

[gjethwani]

Hello! Me again with another optional optimization.

My motivation for submitting the linked PR above was that our app uses multiple circuit breakers from opossum and the timer load was too heavy on memory. Disabling the snapshots helped, but we were still seeing our memory climb, indicating that the bucket-interval timers were still a problem.

That had me thinking: the bucket-interval triggers at the same time and does the same thing across all the breakers: it rotates the buckets. What if we could have one central timer in the app that controls all of the buckets? We would significantly decrease our timer load.

That is the motivation for this PR. I added an option for a user-provided EventEmitter that would handle the rotating of the buckets. The idea here is that you could do something like this in your app

const bucketController = new EventEmitter();
setInterval(() => {
   bucketController.emit('rotate')
}, 1000)

And then pass bucketController into opossum. Opossum would listen for that event and then rotate the buckets.

As a note, we forked the library in our service and tried this and it had a tremendous effect on memory. The effect was pretty much tenfold for us.

Thanks for hearing me out and let me know if you have any questions!

[coveralls]

Pull Request Test Coverage Report for Build 6169531621

• 19 of 19 (100.0%) changed or added relevant lines in 2 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.07%) to 98.331%

---

Totals
Change from base Build 5879653887:	0.07%
Covered Lines:	373
Relevant Lines:	374

---

💛 - Coveralls

[gjethwani]

Corresponding types PR
Let me know if I should change the version at the top of the types PR to 8.2 👍

[lholmquist]

@gjethwani thanks for the PR, i just getting back from PTO, so i'll make sure to take a look at this, this week

[lholmquist]

@gjethwani This looks really great.

The only thing i wonder is if we can make it slightly easier to call for the end user, so instead of having to create the event emitter and then use that, i wonder if we could do something like this:

// create a new circuit and tell it that you want to rotate the buckets manually
const breaker = new CircuitBreaker(passFail, {
    rotateBuckets: false // not married to the name,  but just something to say you are going to do it manually
    rollingCountTimeout: 10
  });

// Rotate the buckets manually for a single circuit
breaker.rotate();


// Maybe for multiple circuits, we could have the rotate on the CircuitBreaker class?
// I think we can get all the circuits, maybe?

// Rotates the buckets manually for a single circuit
CircuitBreaker.rotate(); 

Thoughts?

I'm fine with just merging what we have now, and refining later

[lholmquist]

one other thing, if you disable a circuit, will the emitter still listen for the rotate event?

[gjethwani]

I like your suggestion of making it easier for the end user, but the purpose of the Event Emitter here would be so that you could control multiple breakers across your app at the same time. Imports might get tricky if we try to call a function on the actual circuit breaker object on the central interval. I figured it's easier to just pass the event emitter into every single circuit breaker instance created. This would only be for highly specialized use cases where you're trying to optimize for memory with a large number of breakers. Keen to hear your thoughts on this and whether you think there's another way

[gjethwani]

one other thing, if you disable a circuit, will the emitter still listen for the rotate event?

In it's current state, yes it will still listen if the circuit is disabled

[lholmquist]

I like your suggestion of making it easier for the end user, but the purpose of the Event Emitter here would be so that you could control multiple breakers across your app at the same time. Imports might get tricky if we try to call a function on the actual circuit breaker object on the central interval. I figured it's easier to just pass the event emitter into every single circuit breaker instance created. This would only be for highly specialized use cases where you're trying to optimize for memory with a large number of breakers. Keen to hear your thoughts on this and whether you think there's another way

I think we can go with what you have, i don't have strong feelings with what i wrote 😄

[lholmquist]

In it's current state, yes it will still listen if the circuit is disabled

We probably want to stop listening if the circuit is disabled.

If the circuit is then re-enabled, we would probably need to re-listen

[gjethwani]

@lholmquist sounds reasonable. i will make that change this week

[gjethwani]

@lholmquist Was this a typo? If yes, I took the liberty of correcting it :)

[lholmquist]

🤣 most likely. damn copy/paste

[gjethwani]

Hi @lholmquist I made the changes you requested and added the relevant tests. They are passing locally and for most of the pipelines, but I can't tell why the current pipeline is not passing. As far as I can tell, it performs the same steps as the 18.x pipeline. Is this something you could shed some light on?

[lholmquist]

very weird, i'll look into it

[lholmquist]

The workflow that it is failing on, is when running the tests in "headless" for the browser. For some reason only 1 listener is getting added to the event emitter with the browser version. let me look into it a little more

[lholmquist]

created this PR gjethwani#1 that should fix this

[gjethwani]

Much appreciated @lholmquist ! I merged your PR and it looks like all checks are passing. Could I have your review please :)

[lholmquist]

Thanks for this and staying with it through the process!

[lholmquist]

released as 8.1.1

[gjethwani]

Sure, thanks for your feedback!