goodeggs / librato-node

A Node.js client for Librato (http://librato.com/)
MIT License
37 stars 26 forks source link

Counter never reset after flush, keeps incrementing #52

Open michaelnatkin opened 7 years ago

michaelnatkin commented 7 years ago

I added a little console.log and reduced the flush period just to demonstrate the problem:

{"name":"chefsteps-chatbot-facebook-messenger","hostname":"cs-mbp-11-4.local","pid":71247,"level":30,"methodName":"increment","args":["develop.chatbot-fb-messenger.message-sent"],"msg":"Librato debugging","time":"2017-02-02T23:53:15.924Z","v":0}
****** SENDING TO LIBRATO: {"counters":[{"name":"develop.chatbot-fb-messenger.message-received","value":1},{"name":"develop.chatbot-fb-messenger.message-sent","value":1}],"gauges":[{"value":1,"name":"develop.chatbot-fb-messenger.score"}]}
****** SENDING TO LIBRATO: {"counters":[{"name":"develop.chatbot-fb-messenger.message-received","value":1},{"name":"develop.chatbot-fb-messenger.message-sent","value":1}],"gauges":[]}
****** SENDING TO LIBRATO: {"counters":[{"name":"develop.chatbot-fb-messenger.message-received","value":1},{"name":"develop.chatbot-fb-messenger.message-sent","value":1}],"gauges":[]}
****** SENDING TO LIBRATO: {"counters":[{"name":"develop.chatbot-fb-messenger.message-received","value":1},{"name":"develop.chatbot-fb-messenger.message-sent","value":1}],"gauges":[]}
****** SENDING TO LIBRATO: {"counters":[{"name":"develop.chatbot-fb-messenger.message-received","value":1},{"name":"develop.chatbot-fb-messenger.message-sent","value":1}],"gauges":[]}
michaelnatkin commented 7 years ago

Fix is to add @cache = {} to CounterCache#flushTo

michaelnatkin commented 7 years ago

@bobzoller see above

michaelnatkin commented 7 years ago

... or maybe it should have remained as a gauge?

michaelnatkin commented 7 years ago

Yes, I think the correct thing is it should have stayed a gauge. Although it sounds right, increment shouldn't use counters, because counters are meant only for tracking the derivative of a monotonically increasing value (which you've done) and can't be used with server-side aggregation (which makes them useless for the normal increment case).

https://www.librato.com/docs/kb/faq/glossary/whats_a_counter/ https://www.librato.com/docs/kb/faq/app_questions/count_events/ https://github.com/librato/librato-metrics/issues/105

bobzoller commented 7 years ago

we went around on this back in #27 ... my intention is to treat librato-rack as the best practice, which is why increment was changed to use counters. at the time, I also bought into the case that was made for not deleting/resetting counters. FWIW, despite a somewhat painful migration, this has worked fine for our use cases.