search

yammer / circuitbox

Circuit breaker built with large Ruby apps in mind.
Other
704 stars 59 forks source link

Track timer only if notifier needs to track it #168

Closed ritikesh closed 3 years ago

ritikesh commented 3 years ago

I do not want to compute time differences if I'm using a Notifier::Null notifier. This was the behaviour earlier and got replaced with #159.

matthewshafer commented 3 years ago

Thanks for opening the PR!

When I was taking a look at the changes I thought it might make things a little cleaner if the notifiers handled the timing themselves. That way disabling the timing of the circuit isn't specifically tied to using the null notifier. If I were to make that change I think it would fulfill what you are looking to do and also make it possible for someone to skip timing in their own notifiers. I can look at implementing that this weekend and then release another prerelease sometime next week if that sounds good?

ritikesh commented 3 years ago

Sounds good. I thought about that but wanted to refrain from making too many changes to get this done quickly(dirtily) 😅 Let me know if you want me to help with the refactor in this PR itself. Glad to help.

matthewshafer commented 3 years ago

Just wanted to give an update. I'm mostly done with moving over to have the notifiers do timing, just working on the naming of some things. The changes I have so far would just take advantage of ActiveSupport::Notifications emitting start and finish times, circuit box wouldn't do any timing itself anymore. This seems to me like it would fulfill the request to not have runtime metrics run when the timer is the null timer. The null timer's method for tracking this (currently metric_gauge but I'm going to rename it) would just block.call (or yield).

ritikesh commented 3 years ago

Thanks @matthewshafer for the updates. Looking forward to the next prerelease version!

matthewshafer commented 3 years ago

I finally opened a PR #169 with the changes. I've held off for a while because I've been waiting for one of my coworkers to take a look since it'll change how we measure things in our applications. Hopefully they are able to take a look in the next day or two and then I can merge this.

ritikesh commented 3 years ago

closing this in favour of #169