Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Add a 'spiral' one minute sliding window count/meter #1

Closed
wants to merge 4 commits into from

2 participants

@russelldb
Owner

Sorry about the name.

@rzezeski rzezeski was assigned
@rzezeski rzezeski commented on the diff
src/folsom_metrics_spiral.erl
((21 lines not shown))
+%%% @doc A total count, and sliding window count of events over the last
+%%% minute.
+%%% @end
+%%%------------------------------------------------------------------
+
+-module(folsom_metrics_spiral).
+
+-export([new/1,
+ update/2,
+ trim/2,
+ get_value/1,
+ get_values/1
+ ]).
+
+%% size of the window in seconds
+-define(WINDOW, 60).
@rzezeski
rzezeski added a note

Should spiral be hardcoded to minute?

@russelldb Owner

Yes, otherwise there is a load of front matter code to add to folsom_metrics, folsom_ets etc. We only ever use a 1 minute window…theoretically others may need configurable windows, but I didn't see now as the time to add a maybe feature.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@rzezeski rzezeski commented on the diff
src/folsom_metrics_spiral.erl
((27 lines not shown))
+
+-export([new/1,
+ update/2,
+ trim/2,
+ get_value/1,
+ get_values/1
+ ]).
+
+%% size of the window in seconds
+-define(WINDOW, 60).
+
+-include("folsom.hrl").
+
+new(Name) ->
+ Spiral = #spiral{},
+ Pid = folsom_sample_slide_sup:start_slide_server(?MODULE,
@rzezeski
rzezeski added a note

Just a question, would it make any sense to use the folsom_sample_slide:new API and store the slide record? Or is this just a facade on slide? Excuse me, I have a general ignorance of these metrics. I'm trying not to follow rabbit holes while reviewing this.

@russelldb Owner

The slide sample stores the readings, all of them. The spiral just stores a count. So at most 90 integers in memory vs. as many integers in memory per second as there are events. So on a 10k puts a second cluster that is a lot of '1s' over just incrementing a counter. I thought of using slide, but it just seemed wasteful to store a great number of events. when all we want is the number of events in a minute.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@rzezeski rzezeski commented on the diff
src/folsom_metrics_spiral.erl
((36 lines not shown))
+-define(WINDOW, 60).
+
+-include("folsom.hrl").
+
+new(Name) ->
+ Spiral = #spiral{},
+ Pid = folsom_sample_slide_sup:start_slide_server(?MODULE,
+ Spiral#spiral.tid,
+ ?WINDOW),
+ ets:insert_new(Spiral#spiral.tid, {count, 0}),
+ ets:insert(?SPIRAL_TABLE, {Name, Spiral#spiral{server=Pid}}).
+
+update(Name, Value) ->
+ #spiral{tid=Tid} = get_value(Name),
+ Moment = folsom_utils:now_epoch(),
+ ets:insert_new(Tid, {Moment, 0}),
@rzezeski
rzezeski added a note

I guess that the use of insert_new versus insert is to protect against overwriting a value when there are multiple values for a single moment. However, folsom_utils:now_epoch uses erlang:now which guarantees unique values. I wonder, in general, about the use of now versus os:timestamp but that is tangental to this PR.

In practice, I'm not sure the use of insert_new + now hurts anything and I suppose it future-proofs the function for the future if something like os:timestamp were to be used. I just wanted to point this out. I don't think there is any action to take.

@russelldb Owner

No, insert_new is 'cos you can't increment a non existing counter. But insert would zero the counter for the current moment if it already existed, insert_new means "only create this key if it doesn't exist already" and then update it. now_epoch returns the number of seconds only (it doesn't guarantee a unique value). Basically, we store a counter per second, and increment that counter for every event in the given moment.

Your concerns about now() are well founded, I need to discuss changing folsom with Joe Williams in this regard.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@rzezeski rzezeski commented on the diff
src/folsom_metrics_spiral.erl
((40 lines not shown))
+new(Name) ->
+ Spiral = #spiral{},
+ Pid = folsom_sample_slide_sup:start_slide_server(?MODULE,
+ Spiral#spiral.tid,
+ ?WINDOW),
+ ets:insert_new(Spiral#spiral.tid, {count, 0}),
+ ets:insert(?SPIRAL_TABLE, {Name, Spiral#spiral{server=Pid}}).
+
+update(Name, Value) ->
+ #spiral{tid=Tid} = get_value(Name),
+ Moment = folsom_utils:now_epoch(),
+ ets:insert_new(Tid, {Moment, 0}),
+ ets:update_counter(Tid, Moment, Value),
+ ets:update_counter(Tid, count, Value).
+
+get_value(Name) ->
@rzezeski
rzezeski added a note

It might make more sense to call this get_spiral.

@russelldb Owner

agree, but as far as the idioms of folsom goes, it is always get_value.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/folsom_metrics_meter.erl
@@ -52,7 +52,7 @@ new(Name) ->
{Name, #meter{one = OneMin,
five = FiveMin,
fifteen = FifteenMin,
- start_time = folsom_utils:now_epoch_micro()}}).
@rzezeski
rzezeski added a note

Why did you change this to second resolution? Does this not have an effect on things besides spiral. Once again, excuse my ignorance of all this.

@russelldb Owner

dammit, that is a mistake. All changes to meter are a mistake. Will roll those back at once. Dunno how I missed that.

@rzezeski
rzezeski added a note

No biggie, this is why we review :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@rzezeski

I saw an error while running the tests. I haven't dug into the cause yet.

.Shrinking..........................(19 times)
[{set,{var,1},{call,slide_uniform_eqc,new_histo,[]}},
 {set,{var,4},
      {call,slide_uniform_eqc,update,[{call,erlang,element,[2,{var,1}]},0]}},
 {set,{var,8},{call,slide_uniform_eqc,update,[{var,4},0]}},
 {set,{var,12},{call,slide_uniform_eqc,update,[{var,8},0]}},
 {set,{var,13},{call,slide_uniform_eqc,update,[{var,12},0]}},
 {set,{var,14},{call,slide_uniform_eqc,trim,[{var,13},60]}}]
eq: failed
{postcondition,{"after trim",
                {"model",
                 [{{1000,1},0},{{1000,2},0},{{1000,3},0},{{1000,4},0}]},
                {"sample",
                 [{{1005,1},0},{{1005,2},0},{{1005,3},0},{{1005,4},0}]}}} /= ok
slide_uniform_eqc:110: prop_window_test_...*failed*
::error:{assertion_failed,[{module,slide_uniform_eqc},
                         {line,110},
                         {expression,"eqc : quickcheck ( eqc : numtests ( ? NUMTESTS , ? QC_OUT ( prop_window ( ) ) ) )"},
                         {expected,true},
                         {value,false}]}
  in function slide_uniform_eqc:'-prop_window_test_/0-fun-3-'/0                                                                                                           [29/476]
  output:<<"History: [{{state,1000,undefined,undefined,[]},
           {#Ref<0.0.0.13241>,
            {slide_uniform,60,5,1073195,{1341,510641,212453},<0.2258.0>}}},
          {{state,1000,
                  {slide_uniform,60,5,1073195,{1341,510641,212453},<0.2258.0>},
                  #Ref<0.0.0.13241>,[]},
           []},
          {{state,1000,
                  {slide_uniform,60,5,1073195,{1341,510641,212453},<0.2258.0>},
                  #Ref<0.0.0.13241>,[]},
           1009},
          {{state,1009,
                  {slide_uniform,60,5,1073195,{1341,510641,212453},<0.2258.0>},
                  #Ref<0.0.0.13241>,[]},
           {slide_uniform,60,5,1073195,{1341,510641,212453},<0.2258.0>}},
          {{state,1009,
                  {slide_uniform,60,5,1073195,{1341,510641,212453},<0.2258.0>}">>...

WARNING: Deleting data for module folsom_utils imported from
["/Users/rzezeski/work/folsom/.eunit/folsom_utils.coverdata",
 "/Users/rzezeski/work/folsom/.eunit/folsom_utils_meck_original.coverdata"]

=INFO REPORT==== 5-Jul-2012::13:50:42 ===                                                                                                                                  [6/476]
    application: folsom
    exited: stopped
    type: temporary
=======================================================
  Failed: 1.  Skipped: 0.  Passed: 9.
.One or more tests were cancelled.
...........(21 times)
Reason: 
{'EXIT',{{not_mocked,folsom_utils},
         [{meck,gen_server,3},
          {slide_statem_eqc,initial_state,0},
          {eqc_statem,f500_0,3},
          {eqc_statem,run_commands,2},
          {slide_statem_eqc,'-prop_window/0-fun-2-',1},
          {eqc,'-f777_0/2-fun-4-',3},
          {eqc_gen,'-f321_0/2-fun-0-',5},
          {eqc_gen,f186_0,2}]}}
[{set,{var,1},{call,slide_statem_eqc,new_histo,[]}},
 {set,{var,20},
      {call,slide_statem_eqc,get_values,[{call,erlang,element,[2,{var,1}]}]}}]
Analysis includes data from imported files
["/Users/rzezeski/work/folsom/.eunit/folsom_utils.coverdata",
 "/Users/rzezeski/work/folsom/.eunit/folsom_utils_meck_original.coverdata"]
Analysis includes data from imported files
["/Users/rzezeski/work/folsom/.eunit/folsom_utils.coverdata",
 "/Users/rzezeski/work/folsom/.eunit/folsom_utils_meck_original.coverdata"]
Cover analysis: /Users/rzezeski/work/folsom/.eunit/index.html
ERROR: One or more eunit tests failed.
@russelldb
Owner

Reverted meter. Sorry 'bout that.

@rzezeski

+1 to merge.

@russelldb russelldb closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jul 2, 2012
  1. @russelldb

    Add a 'spiral' one minute sliding window count/meter

    russelldb authored
    Sorry about the name.
Commits on Jul 5, 2012
  1. @russelldb

    Add a 'spiral' one minute sliding window count/meter

    russelldb authored
    Sorry about the name.
  2. @russelldb
  3. @russelldb

    Revert meter

    russelldb authored
This page is out of date. Refresh to see the latest.
View
9 include/folsom.hrl
@@ -6,6 +6,7 @@
-define(METER_READER_TABLE, folsom_meter_readers).
-define(HISTORY_TABLE, folsom_histories).
-define(DURATION_TABLE, folsom_durations).
+-define(SPIRAL_TABLE, folsom_spirals).
-define(DEFAULT_LIMIT, 5).
-define(DEFAULT_SIZE, 1028). % mimic codahale's metrics
@@ -14,6 +15,14 @@
-define(DEFAULT_INTERVAL, 5000).
-define(DEFAULT_SAMPLE_TYPE, uniform).
+-record(spiral, {
+ tid = folsom_metrics_histogram_ets:new(folsom_spiral,
+ [ordered_set,
+ {write_concurrency, true},
+ public]),
+ server
+ }).
+
-record(slide, {
window = ?DEFAULT_SLIDING_WINDOW,
reservoir = folsom_metrics_histogram_ets:new(folsom_slide,
View
20 src/folsom_ets.erl
@@ -140,6 +140,8 @@ get_values(Name, meter_reader) ->
folsom_metrics_meter_reader:get_values(Name);
get_values(Name, duration) ->
folsom_metrics_duration:get_values(Name);
+get_values(Name, spiral) ->
+ folsom_metrics_spiral:get_values(Name);
get_values(_, Type) ->
{error, Type, unsupported_metric_type}.
@@ -181,6 +183,10 @@ maybe_add_handler(meter_reader, Name, false) ->
true = folsom_metrics_meter_reader:new(Name),
true = ets:insert(?FOLSOM_TABLE, {Name, #metric{type = meter_reader}}),
ok;
+maybe_add_handler(spiral, Name, false) ->
+ true = folsom_metrics_spiral:new(Name),
+ true = ets:insert(?FOLSOM_TABLE, {Name, #metric{type = spiral}}),
+ ok;
maybe_add_handler(Type, _, false) ->
{error, Type, unsupported_metric_type};
maybe_add_handler(_, Name, true) ->
@@ -251,6 +257,13 @@ delete_metric(Name, meter) ->
delete_metric(Name, meter_reader) ->
true = ets:delete(?METER_READER_TABLE, Name),
true = ets:delete(?FOLSOM_TABLE, Name),
+ ok;
+delete_metric(Name, spiral) ->
+ #spiral{tid=Tid, server=Pid} = folsom_metrics_spiral:get_value(Name),
+ folsom_sample_slide_server:stop(Pid),
+ true = ets:delete(Tid),
+ ets:delete(?SPIRAL_TABLE, Name),
+ ets:delete(?FOLSOM_TABLE, Name),
ok.
delete_histogram(Name, #histogram{type = uniform, sample = #uniform{reservoir = Reservoir}}) ->
@@ -344,6 +357,13 @@ notify(Name, Value, duration, false) ->
add_handler(duration, Name),
folsom_metrics_duration:update(Name, Value),
ok;
+notify(Name, Value, spiral, true) ->
+ folsom_metrics_spiral:update(Name, Value),
+ ok;
+notify(Name, Value, spiral, false) ->
+ add_handler(spiral, Name),
+ folsom_metrics_spiral:update(Name, Value),
+ ok;
notify(_, _, Type, _) ->
{error, Type, unsupported_metric_type}.
View
4 src/folsom_metrics.erl
@@ -39,6 +39,7 @@
new_duration/2,
new_duration/3,
new_duration/4,
+ new_spiral/1,
delete_metric/1,
notify/1,
notify/2,
@@ -103,6 +104,9 @@ new_duration(Name, SampleType, SampleSize) ->
new_duration(Name, SampleType, SampleSize, Alpha) ->
folsom_ets:add_handler(duration, Name, SampleType, SampleSize, Alpha).
+new_spiral(Name) ->
+ folsom_ets:add_handler(spiral, Name).
+
delete_metric(Name) ->
folsom_ets:delete_handler(Name).
View
74 src/folsom_metrics_spiral.erl
@@ -0,0 +1,74 @@
+%%%
+%%% Copyright 2012, Basho Technologies, Inc. All Rights Reserved.
+%%%
+%%% Licensed under the Apache License, Version 2.0 (the "License");
+%%% you may not use this file except in compliance with the License.
+%%% You may obtain a copy of the License at
+%%%
+%%% http://www.apache.org/licenses/LICENSE-2.0
+%%%
+%%% Unless required by applicable law or agreed to in writing, software
+%%% distributed under the License is distributed on an "AS IS" BASIS,
+%%% WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+%%% See the License for the specific language governing permissions and
+%%% limitations under the License.
+%%%
+
+
+%%%-------------------------------------------------------------------
+%%% File: folsom_metrics_spiral.erl
+%%% @author Russell Brown <russelldb@basho.com>
+%%% @doc A total count, and sliding window count of events over the last
+%%% minute.
+%%% @end
+%%%------------------------------------------------------------------
+
+-module(folsom_metrics_spiral).
+
+-export([new/1,
+ update/2,
+ trim/2,
+ get_value/1,
+ get_values/1
+ ]).
+
+%% size of the window in seconds
+-define(WINDOW, 60).
@rzezeski
rzezeski added a note

Should spiral be hardcoded to minute?

@russelldb Owner

Yes, otherwise there is a load of front matter code to add to folsom_metrics, folsom_ets etc. We only ever use a 1 minute window…theoretically others may need configurable windows, but I didn't see now as the time to add a maybe feature.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+
+-include("folsom.hrl").
+
+new(Name) ->
+ Spiral = #spiral{},
+ Pid = folsom_sample_slide_sup:start_slide_server(?MODULE,
@rzezeski
rzezeski added a note

Just a question, would it make any sense to use the folsom_sample_slide:new API and store the slide record? Or is this just a facade on slide? Excuse me, I have a general ignorance of these metrics. I'm trying not to follow rabbit holes while reviewing this.

@russelldb Owner

The slide sample stores the readings, all of them. The spiral just stores a count. So at most 90 integers in memory vs. as many integers in memory per second as there are events. So on a 10k puts a second cluster that is a lot of '1s' over just incrementing a counter. I thought of using slide, but it just seemed wasteful to store a great number of events. when all we want is the number of events in a minute.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ Spiral#spiral.tid,
+ ?WINDOW),
+ ets:insert_new(Spiral#spiral.tid, {count, 0}),
+ ets:insert(?SPIRAL_TABLE, {Name, Spiral#spiral{server=Pid}}).
+
+update(Name, Value) ->
+ #spiral{tid=Tid} = get_value(Name),
+ Moment = folsom_utils:now_epoch(),
+ ets:insert_new(Tid, {Moment, 0}),
@rzezeski
rzezeski added a note

I guess that the use of insert_new versus insert is to protect against overwriting a value when there are multiple values for a single moment. However, folsom_utils:now_epoch uses erlang:now which guarantees unique values. I wonder, in general, about the use of now versus os:timestamp but that is tangental to this PR.

In practice, I'm not sure the use of insert_new + now hurts anything and I suppose it future-proofs the function for the future if something like os:timestamp were to be used. I just wanted to point this out. I don't think there is any action to take.

@russelldb Owner

No, insert_new is 'cos you can't increment a non existing counter. But insert would zero the counter for the current moment if it already existed, insert_new means "only create this key if it doesn't exist already" and then update it. now_epoch returns the number of seconds only (it doesn't guarantee a unique value). Basically, we store a counter per second, and increment that counter for every event in the given moment.

Your concerns about now() are well founded, I need to discuss changing folsom with Joe Williams in this regard.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ ets:update_counter(Tid, Moment, Value),
+ ets:update_counter(Tid, count, Value).
+
+get_value(Name) ->
@rzezeski
rzezeski added a note

It might make more sense to call this get_spiral.

@russelldb Owner

agree, but as far as the idioms of folsom goes, it is always get_value.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ [{Name, Spiral}] = ets:lookup(?SPIRAL_TABLE, Name),
+ Spiral.
+
+trim(Tid, _Window) ->
+ Oldest = oldest(),
+ ets:select_delete(Tid, [{{'$1','_'}, [{is_integer, '$1'}, {'<', '$1', Oldest}], ['true']}]).
+
+get_values(Name) ->
+ Oldest = oldest(),
+ #spiral{tid=Tid} = get_value(Name),
+ [{count, Count}] = ets:lookup(Tid, count),
+ One =lists:sum(ets:select(Tid, [{{'$1','$2'},[{is_integer, '$1'}, {'>=', '$1', Oldest}],['$2']}])),
+
+ [{count, Count}, {one, One}].
+
+oldest() ->
+ folsom_utils:now_epoch() - ?WINDOW.
+
+
View
3  src/folsom_sup.erl
@@ -107,7 +107,8 @@ create_tables() ->
{?METER_TABLE, [set, named_table, public, {write_concurrency, true}]},
{?METER_READER_TABLE, [set, named_table, public, {write_concurrency, true}]},
{?HISTORY_TABLE, [set, named_table, public, {write_concurrency, true}]},
- {?DURATION_TABLE, [ordered_set, named_table, public, {write_concurrency, true}]}
+ {?DURATION_TABLE, [ordered_set, named_table, public, {write_concurrency, true}]},
+ {?SPIRAL_TABLE, [set, named_table, public, {write_concurrency, true}]}
],
[maybe_create_table(ets:info(Name), Name, Opts) || {Name, Opts} <- Tables],
ok.
View
20 test/folsom_erlang_checks.erl
@@ -63,6 +63,8 @@ create_metrics() ->
ok = folsom_metrics:new_duration(duration),
+ ok = folsom_metrics:new_spiral(spiral),
+
?debugFmt("ensuring meter tick is registered with gen_server~n", []),
ok = ensure_meter_tick_exists(),
@@ -75,7 +77,10 @@ create_metrics() ->
{state, List} = folsom_meter_timer_server:dump(),
2 = length(List),
- 12 = length(folsom_metrics:get_metrics()),
+ %% check a server got started for the spiral metric
+ 1 = length(supervisor:which_children(folsom_sample_slide_sup)),
+
+ 13 = length(folsom_metrics:get_metrics()),
?debugFmt("~n~nmetrics: ~p~n", [folsom_metrics:get_metrics()]).
@@ -129,7 +134,9 @@ populate_metrics() ->
[ folsom_metrics:notify({meter_reader, Item}) || Item <- [1, 10, 100, 1000, 10000]],
% simulate an interval tick
- folsom_metrics_meter_reader:tick(meter_reader).
+ folsom_metrics_meter_reader:tick(meter_reader),
+
+ folsom_metrics:notify_existing_metric(spiral, 100, spiral).
check_metrics() ->
0 = folsom_metrics:get_metric_value(counter),
@@ -188,10 +195,14 @@ check_metrics() ->
%% check duration
Dur = folsom_metrics:get_metric_value(duration),
- duration_check(Dur).
+ duration_check(Dur),
+
+ %% check spiral
+ [{count, 100}, {one, 100}] = folsom_metrics:get_metric_value(spiral).
+
delete_metrics() ->
- 14= length(ets:tab2list(?FOLSOM_TABLE)),
+ 15 = length(ets:tab2list(?FOLSOM_TABLE)),
ok = folsom_metrics:delete_metric(counter),
ok = folsom_metrics:delete_metric(<<"gauge">>),
@@ -217,6 +228,7 @@ delete_metrics() ->
0 = length(ets:tab2list(?METER_READER_TABLE)),
ok = folsom_metrics:delete_metric(duration),
+ ok = folsom_metrics:delete_metric(spiral),
0 = length(ets:tab2list(?FOLSOM_TABLE)).
Something went wrong with that request. Please try again.