search

boundary / folsom

Expose Erlang Events and Metrics
Apache License 2.0
586 stars 166 forks source link

Implement new metric to calculate min/max/mean statistics #66

Closed avasenin closed 10 years ago

avasenin commented 10 years ago

Implement new metric to calculate min/max/mean for one minute interval. This metric is used to analyze latency for systems with huge amount of metrics. We have more than 2000 latency metrics and usual histograms doesn't suit for us.

joewilliams commented 10 years ago

Any thoughts @russelldb?

russelldb commented 10 years ago

I need a bit of time to read through it. I'm also wondering if the results are comparable. I mean if we switch to this method of accounting will be graphs for the last year look radically different all of a sudden?

My other immediate concern is that the size of the sliding window can't be changed, and there is no documentation / comments in the code.

Those things aside: less memory / greater efficiency would be nice. if you can wait for me to review tomorrow, then please do.

Can we have both metrics included, rather than replace one with the other for some deprecation period?

russelldb commented 10 years ago

Actually, looked back at the old code, I didn't have any docs / comments either so scratch that complaint.

Looks good to me, I'd just like some time tomorrow to test a little as part of riak.

joewilliams commented 10 years ago

Thanks @russelldb it can definitely wait for your review. Additionally, yes, we would either need to support both metrics or decide a deprecation strategy.

russelldb commented 10 years ago

Pinging @uwiger . In case he has an opinion.

uwiger commented 10 years ago

My suggestion was to complement bear:get_statistics/1 with a bear:get_statistics(Values, Keys), in order to calculate a subset (e.g. [arithmetic_mean, min, max]) of the values. The implementation does its best to avoid unnecessary calculations, and is considerably faster when only things like mean, min, max and percentiles are needed.

https://github.com/Feuerlabs/bear/blob/uw-stats-subset/src/bear.erl#L98

joewilliams commented 10 years ago

@uwiger a PR has been started allowing this sort of functionality https://github.com/boundary/bear/pull/4 but hasn't been completed. I would happily merge a PR based on your branch with tests and etc.

uwiger commented 10 years ago

@joewilliams I added some eunit test code for the get_statistics_subset/2 function, but my repos is forked off the basho version. Not sure how I should best go about crafting a pull request against boundary/bear...

uwiger commented 10 years ago

https://github.com/boundary/bear/pull/13

joewilliams commented 10 years ago

@uwiger Merged!

joewilliams commented 10 years ago

@avasenin Please start a new PR for the changes you'd like to have merged based on @uwiger contribution to bear. I believe the changes you would like should be much simpler now and you could likely use an existing metric rather than create a new one.

avasenin commented 10 years ago

@joewilliams actually I'm a bit confused because I don't understand how I could use the changes which are made by @uwiger. I agree that get_statistics_subset works much faster but it still requires to store histograms in ETS and we still need to have one ETS table per metric. Furthermore, get_statistics_subset optimizes the get_historgram_statistics method which is called not so much often (only when we push metrics to graphics engine).

The main idea of this new metric is to use reactive way to calculate statistics. We don't store any history of values and always recalculate statistics on each notify. This approach is very limited by the number of supported statistics functions but it works on the one ETS table and doesn't store so much values in memory and it fast.

I will glad to hear any ideas how I could rework my PR to pass your review.

joewilliams commented 10 years ago

@avasenin I think I would rather see this as a type of histogram rather than a completely new metric. Do you think it would be possible to create a new "slide_simple" histogram type that would fulfill your requirements?

uwiger commented 10 years ago

@avasenin, FYI, we (Feuerlabs) have just released a metrics package called Exometer. It is reminiscent of Folsom (and is folsom-compatible via a plugin module). If you take a look at our histogram implementation, it might be closer to what you want. We have been running some comparative tests, which indicate that our histograms use considerably less memory than the folsom histograms, and are also quite cheap to read.

@joewilliams, anything you see in exometer that you'd like to steal to folsom, please go ahead. We have certainly peeked at folsom. ;-)