Merge branch 'anders/diameter/statistics/OTP-9608' into maint

* anders/diameter/statistics/OTP-9608:
  Improve statistics test cases
  Statistics fixes

5 files changed, 181 insertions, 206 deletions

diff --git a/lib/diameter/src/base/diameter_config.erl b/lib/diameter/src/base/diameter_config.erl
index 9253af0de2..eb06045ace 100644
--- a/lib/diameter/src/base/diameter_config.erl
+++ b/lib/diameter/src/base/diameter_config.erl
@@ -1,7 +1,7 @@
 %%
 %% %CopyrightBegin%
 %%
-%% Copyright Ericsson AB 2010-2011. All Rights Reserved.
+%% Copyright Ericsson AB 2010-2012. All Rights Reserved.
 %%
 %% The contents of this file are subject to the Erlang Public License,
 %% Version 1.1, (the "License"); you may not use this file except in
@@ -519,6 +519,7 @@ rm(SvcName, L) ->
     Refs = lists:map(fun(#transport{ref = R}) -> R end, L),
     case stop_transport(SvcName, Refs) of
         ok ->
+            diameter_stats:flush(Refs),
             lists:foreach(fun delete_object/1, L);
         {error, _} = No ->
             No
diff --git a/lib/diameter/src/base/diameter_peer_fsm.erl b/lib/diameter/src/base/diameter_peer_fsm.erl
index ca2e8c56cb..302540e76b 100644
--- a/lib/diameter/src/base/diameter_peer_fsm.erl
+++ b/lib/diameter/src/base/diameter_peer_fsm.erl
@@ -1,7 +1,7 @@
 %%
 %% %CopyrightBegin%
 %%
-%% Copyright Ericsson AB 2010-2011. All Rights Reserved.
+%% Copyright Ericsson AB 2010-2012. All Rights Reserved.
 %%
 %% The contents of this file are subject to the Erlang Public License,
 %% Version 1.1, (the "License"); you may not use this file except in
@@ -131,11 +131,10 @@
 %% specified on the transport in question. Check here that the list is
 %% still non-empty.
 
-start({_, Ref} = Type, Opts, #diameter_service{applications = Apps} = Svc) ->
+start({_,_} = Type, Opts, #diameter_service{applications = Apps} = Svc) ->
     [] /= Apps orelse ?ERROR({no_apps, Type, Opts}),
     T = {self(), Type, Opts, Svc},
     {ok, Pid} = diameter_peer_fsm_sup:start_child(T),
-    diameter_stats:reg(Pid, Ref),
     Pid.
 
 start_link(T) ->
@@ -157,6 +156,7 @@ init(T) ->
 i({WPid, T, Opts, #diameter_service{capabilities = Caps} = Svc}) ->
     putr(?DWA_KEY, dwa(Caps)),
     {M, Ref} = T,
+    diameter_stats:reg(Ref),
     {[Ts], Rest} = proplists:split(Opts, [capabilities_cb]),
     putr(?CB_KEY, {Ref, [F || {_,F} <- Ts]}),
     erlang:monitor(process, WPid),
diff --git a/lib/diameter/src/base/diameter_service.erl b/lib/diameter/src/base/diameter_service.erl
index ced6fe605c..dc74c57334 100644
--- a/lib/diameter/src/base/diameter_service.erl
+++ b/lib/diameter/src/base/diameter_service.erl
@@ -43,8 +43,7 @@
          subscriptions/0,
          services/0,
          services/1,
-         whois/1,
-         flush_stats/1]).
+         whois/1]).
 
 %% test/debug
 -export([call_module/3,
@@ -390,15 +389,6 @@ whois(SvcName) ->
             undefined
     end.
 
-%%% ---------------------------------------------------------------------------
-%%% # flush_stats/1
-%%%
-%%% Output: list of {{SvcName, Alias, Counter}, Value}
-%%% ---------------------------------------------------------------------------
-
-flush_stats(TPid) ->
-    diameter_stats:flush(TPid).
-
 %% ===========================================================================
 %% ===========================================================================
 
@@ -2857,11 +2847,10 @@ complete(Pre) ->
     end.
 
 info_stats(#state{peerT = PeerT}) ->
-    Peers = ets:select(PeerT, [{#peer{ref = '$1', conn = '$2', _ = '_'},
-                                [{'is_pid', '$2'}],
-                                [['$1', '$2']]}]),
-    diameter_stats:read(lists:append(Peers)).
-%% TODO: include peer identities in return value
+    MatchSpec = [{#peer{ref = '$1', conn = '$2', _ = '_'},
+                  [{'is_pid', '$2'}],
+                  [['$1', '$2']]}],
+    diameter_stats:read(lists:append(ets:select(PeerT, MatchSpec))).
 
 info_transport(#state{peerT = PeerT, connT = ConnT}) ->
     dict:fold(fun it/3,
diff --git a/lib/diameter/src/base/diameter_stats.erl b/lib/diameter/src/base/diameter_stats.erl
index 71479afa95..70727d068e 100644
--- a/lib/diameter/src/base/diameter_stats.erl
+++ b/lib/diameter/src/base/diameter_stats.erl
@@ -1,7 +1,7 @@
 %%
 %% %CopyrightBegin%
 %%
-%% Copyright Ericsson AB 2010-2011. All Rights Reserved.
+%% Copyright Ericsson AB 2010-2012. All Rights Reserved.
 %%
 %% The contents of this file are subject to the Erlang Public License,
 %% Version 1.1, (the "License"); you may not use this file except in
@@ -22,14 +22,13 @@
 %%
 
 -module(diameter_stats).
--compile({no_auto_import, [monitor/2]}).
 
 -behaviour(gen_server).
 
--export([reg/1, reg/2,
-         incr/1, incr/2, incr/3,
+-export([reg/2, reg/1,
+         incr/3, incr/1,
          read/1,
-         flush/0, flush/1]).
+         flush/1]).
 
 %% supervisor callback
 -export([start_link/0]).
@@ -48,123 +47,105 @@
 
 -include("diameter_internal.hrl").
 
-%% ets table containing stats. reg(Pid, Ref) inserts a {Pid, Ref},
-%% incr(Counter, X, N) updates the counter keyed at {Counter, X}, and
-%% Pid death causes counters keyed on {Counter, Pid} to be deleted and
-%% added to those keyed on {Counter, Ref}.
+%% ets table containing 2-tuple stats. reg(Pid, Ref) inserts a {Pid,
+%% Ref}, incr(Counter, X, N) updates the counter keyed at {Counter,
+%% X}, and Pid death causes counters keyed on {Counter, Pid} to be
+%% deleted and added to those keyed on {Counter, Ref}.
 -define(TABLE, ?MODULE).
 
 %% Name of registered server.
 -define(SERVER, ?MODULE).
 
-%% Entries in the table.
--define(REC(Key, Value), {Key, Value}).
-
 %% Server state.
 -record(state, {id = now()}).
 
 -type counter() :: any().
--type contrib() :: any().
-
-%%% ---------------------------------------------------------------------------
-%%% # reg(Pid, Contrib)
-%%%
-%%% Description: Register a process as a contributor of statistics
-%%%              associated with a specified term. Statistics can be
-%%%              contributed by specifying either Pid or Contrib as
-%%%              the second argument to incr/3. Statistics contributed
-%%%              by Pid are folded into the corresponding entry for
-%%%              Contrib when the process dies.
-%%%
-%%%              Contrib can be any term but should not be a pid
-%%%              passed as the first argument to reg/2. Subsequent
-%%%              registrations for the same Pid overwrite the association
-%%% ---------------------------------------------------------------------------
-
--spec reg(pid(), contrib())
-   -> true.
+-type ref() :: any().
+
+%% ---------------------------------------------------------------------------
+%% # reg(Pid, Ref)
+%%
+%% Register a process as a contributor of statistics associated with a
+%% specified term. Statistics can be contributed by specifying either
+%% Pid or Ref as the second argument to incr/3. Statistics contributed
+%% by Pid are folded into the corresponding entry for Ref when the
+%% process dies.
+%% ---------------------------------------------------------------------------
+
+-spec reg(pid(), ref())
+   -> boolean().
 
-reg(Pid, Contrib)
+reg(Pid, Ref)
   when is_pid(Pid) ->
-    call({reg, Pid, Contrib}).
+    call({reg, Pid, Ref}).
 
--spec reg(contrib())
+-spec reg(ref())
    -> true.
 
 reg(Ref) ->
     reg(self(), Ref).
 
-%%% ---------------------------------------------------------------------------
-%%% # incr(Counter, Contrib, N)
-%%%
-%%% Description: Increment a counter for the specified contributor.
-%%%
-%%%              Contrib will typically be an argument passed to reg/2
-%%%              but there's nothing that requires this. In particular,
-%%%              if Contrib is a pid that hasn't been registered then
-%%%              counters are unaffected by the death of the process.
-%%% ---------------------------------------------------------------------------
-
--spec incr(counter(), contrib(), integer())
-   -> integer().
+%% ---------------------------------------------------------------------------
+%% # incr(Counter, Ref, N)
+%%
+%% Increment a counter for the specified contributor.
+%%
+%% Ref will typically be an argument passed to reg/2 but there's
+%% nothing that requires this. Only registered pids can contribute
+%% counters however, otherwise incr/3 is a no-op.
+%% ---------------------------------------------------------------------------
 
-incr(Ctr, Contrib, N) ->
-    update_counter({Ctr, Contrib}, N).
+-spec incr(counter(), ref(), integer())
+   -> integer() | false.
 
-incr(Ctr, N)
+incr(Ctr, Ref, N)
   when is_integer(N) ->
-    incr(Ctr, self(), N);
-
-incr(Ctr, Contrib) ->
-    incr(Ctr, Contrib, 1).
+    update_counter({Ctr, Ref}, N).
 
 incr(Ctr) ->
     incr(Ctr, self(), 1).
 
-%%% ---------------------------------------------------------------------------
-%%% # read(Contribs)
-%%%
-%%% Description: Retrieve counters for the specified contributors.
-%%% ---------------------------------------------------------------------------
+%% ---------------------------------------------------------------------------
+%% # read(Refs)
+%%
+%% Retrieve counters for the specified contributors.
+%% ---------------------------------------------------------------------------
 
--spec read([contrib()])
-   -> [{contrib(), [{counter(), integer()}]}].
+-spec read([ref()])
+   -> [{ref(), [{counter(), integer()}]}].
 
-read(Contribs) ->
-    lists:foldl(fun(?REC({T,C}, N), D) -> orddict:append(C, {T,N}, D) end,
+read(Refs) ->
+    read(Refs, false).
+
+read(Refs, B) ->
+    MatchSpec = [{{{'_', '$1'}, '_'},
+                  [?ORCOND([{'=:=', '$1', {const, R}}
+                            || R <- Refs])],
+                  ['$_']}],
+    L = ets:select(?TABLE, MatchSpec),
+    B andalso delete(L),
+    lists:foldl(fun({{C,R}, N}, D) -> orddict:append(R, {C,N}, D) end,
                 orddict:new(),
-                ets:select(?TABLE, [{?REC({'_', '$1'}, '_'),
-                                     [?ORCOND([{'=:=', '$1', {const, C}}
-                                               || C <- Contribs])],
-                                     ['$_']}])).
-
-%%% ---------------------------------------------------------------------------
-%%% # flush(Contrib)
-%%%
-%%% Description: Retrieve and delete statistics for the specified
-%%%              contributor.
-%%%
-%%%              If Contrib is a pid registered with reg/2 then statistics
-%%%              for both and its associated contributor are retrieved.
-%%% ---------------------------------------------------------------------------
-
--spec flush(contrib())
-   -> [{counter(), integer()}].
+                L).
+
+%% ---------------------------------------------------------------------------
+%% # flush(Refs)
+%%
+%% Retrieve and delete statistics for the specified contributors.
+%% ---------------------------------------------------------------------------
+
+-spec flush([ref()])
+   -> [{ref(), {counter(), integer()}}].
                    
-flush(Contrib) ->
+flush(Refs) ->
     try
-        call({flush, Contrib})
+        call({flush, Refs})
     catch
         exit: _ ->
             []
     end.
 
-flush() ->
-    flush(self()).
-
-%%% ---------------------------------------------------------
-%%% EXPORTED INTERNAL FUNCTIONS
-%%% ---------------------------------------------------------
+%% ===========================================================================
 
 start_link() ->
     ServerName = {local, ?SERVER},
@@ -179,18 +160,16 @@ state() ->
 uptime() ->
     call(uptime).
 
-%%% ----------------------------------------------------------
-%%% # init(_)
-%%%
-%%% Output: {ok, State}
-%%% ----------------------------------------------------------
+%% ----------------------------------------------------------
+%% # init/1
+%% ----------------------------------------------------------
 
 init([]) ->
     ets:new(?TABLE, [named_table, ordered_set, public]),
     {ok, #state{}}.
 
 %% ----------------------------------------------------------
-%% handle_call(Request, From, State)
+%% # handle_call/3
 %% ----------------------------------------------------------
 
 handle_call(state, _, State) ->
@@ -199,31 +178,31 @@ handle_call(state, _, State) ->
 handle_call(uptime, _, #state{id = Time} = State) ->
     {reply, diameter_lib:now_diff(Time), State};
 
-handle_call({reg, Pid, Contrib}, _From, State) ->
-    monitor(not ets:member(?TABLE, Pid), Pid),
-    {reply, insert(?REC(Pid, Contrib)), State};
+handle_call({incr, T}, _, State) ->
+    {reply, update_counter(T), State};
 
-handle_call({flush, Contrib}, _From, State) ->
-    {reply, fetch(Contrib), State};
+handle_call({reg, Pid, Ref}, _From, State) ->
+    B = ets:insert_new(?TABLE, {Pid, Ref}),
+    B andalso erlang:monitor(process, Pid),
+    {reply, B, State};
+
+handle_call({flush, Refs}, _From, State) ->
+    {reply, read(Refs, true), State};
 
 handle_call(Req, From, State) ->
     ?UNEXPECTED([Req, From]),
     {reply, nok, State}.
 
 %% ----------------------------------------------------------
-%% handle_cast(Request, State)
+%% # handle_cast/2
 %% ----------------------------------------------------------
 
-handle_cast({incr, Rec}, State) ->
-    update_counter(Rec),
-    {noreply, State};
-
 handle_cast(Msg, State) ->
     ?UNEXPECTED([Msg]),
     {noreply, State}.
 
 %% ----------------------------------------------------------
-%% handle_info(Request, State)
+%% # handle_info/2
 %% ----------------------------------------------------------
 
 handle_info({'DOWN', _MRef, process, Pid, _}, State) ->
@@ -235,91 +214,62 @@ handle_info(Info, State) ->
     {noreply, State}.
 
 %% ----------------------------------------------------------
-%% terminate(Reason, State)
+%% # terminate/2
 %% ----------------------------------------------------------
 
 terminate(_Reason, _State) ->
     ok.
 
 %% ----------------------------------------------------------
-%% code_change(OldVsn, State, Extra)
+%% # code_change/3
 %% ----------------------------------------------------------
 
 code_change(_OldVsn, State, _Extra) ->
     {ok, State}.
 
-%%% ---------------------------------------------------------
-%%% INTERNAL FUNCTIONS
-%%% ---------------------------------------------------------
-
-%% monitor/2
-
-monitor(true, Pid) ->
-    erlang:monitor(process, Pid);
-monitor(false = No, _) ->
-    No.
+%% ===========================================================================
 
 %% down/1
 
 down(Pid) ->
-    L = ets:match_object(?TABLE, ?REC({'_', Pid}, '_')),
-    [?REC(_, Ref) = T] = lookup(Pid),
+    down(lookup(Pid), ets:match_object(?TABLE, {{'_', Pid}, '_'})).
+
+down([{_, Ref} = T], L) ->
     fold(Ref, L),
-    delete_object(T),
+    delete([T|L]);
+down([], L) -> %% flushed
     delete(L).
 
-%% Fold Pid-based entries into Ref-based ones.
+%% Fold pid-based entries into ref-based ones.
 fold(Ref, L) ->
-    lists:foreach(fun(?REC({K, _}, V)) -> update_counter({{K, Ref}, V}) end,
-                  L).
-
-delete(Objs) ->
-    lists:foreach(fun delete_object/1, Objs).
-
-%% fetch/1
-
-fetch(X) ->
-    MatchSpec = [{?REC({'_', '$1'}, '_'),
-                  [?ORCOND([{'==', '$1', {const, T}} || T <- [X | ref(X)]])],
-                  ['$_']}],
-    L = ets:select(?TABLE, MatchSpec),
-    delete(L),
-    D = lists:foldl(fun sum/2, dict:new(), L),
-    dict:to_list(D).
-
-sum({{Ctr, _}, N}, Dict) ->
-    dict:update(Ctr, fun(V) -> V+N end, N, Dict).
-
-ref(Pid)
-  when is_pid(Pid) ->
-    ets:select(?TABLE, [{?REC(Pid, '$1'), [], ['$1']}]);
-ref(_) ->
-    [].
+    lists:foreach(fun({{K, _}, V}) -> update_counter({{K, Ref}, V}) end, L).
 
 %% update_counter/2
 %%
-%% From an arbitrary request process. Cast to the server process to
-%% insert a new element if the counter doesn't exists so that two
-%% processes don't do so simultaneously.
+%% From an arbitrary process. Call to the server process to insert a
+%% new element if the counter doesn't exists so that two processes
+%% don't insert simultaneously.
 
 update_counter(Key, N) ->
     try
         ets:update_counter(?TABLE, Key, N)
     catch
         error: badarg ->
-            cast({incr, ?REC(Key, N)})
+            call({incr, {Key, N}})
     end.
 
 %% update_counter/1
 %%
-%% From the server process.
+%% From the server process, when update_counter/2 failed due to a
+%% non-existent entry.
 
-update_counter(?REC(Key, N) = T) ->
+update_counter({{_Ctr, Ref} = Key, N} = T) ->
     try
         ets:update_counter(?TABLE, Key, N)
     catch
         error: badarg ->
-            insert(T)
+            (not is_pid(Ref) orelse ets:member(?TABLE, Ref))
+                andalso begin insert(T), N end
     end.
 
 insert(T) ->
@@ -328,13 +278,8 @@ insert(T) ->
 lookup(Key) ->
     ets:lookup(?TABLE, Key).
 
-delete_object(T) ->
-    ets:delete_object(?TABLE, T).
-
-%% cast/1
-
-cast(Msg) ->
-    gen_server:cast(?SERVER, Msg).
+delete(Objs) ->
+    lists:foreach(fun({K,_}) -> ets:delete(?TABLE, K) end, Objs).
 
 %% call/1
 
diff --git a/lib/diameter/test/diameter_stats_SUITE.erl b/lib/diameter/test/diameter_stats_SUITE.erl
index e7807fd360..4a93d4f748 100644
--- a/lib/diameter/test/diameter_stats_SUITE.erl
+++ b/lib/diameter/test/diameter_stats_SUITE.erl
@@ -1,7 +1,7 @@
 %%
 %% %CopyrightBegin%
 %%
-%% Copyright Ericsson AB 2010-2011. All Rights Reserved.
+%% Copyright Ericsson AB 2010-2012. All Rights Reserved.
 %%
 %% The contents of this file are subject to the Erlang Public License,
 %% Version 1.1, (the "License"); you may not use this file except in
@@ -30,11 +30,12 @@
          end_per_suite/1]).
 
 %% testcases
--export([an/1,
-         twa/1]).
+-export([reg/1,
+         incr/1,
+         read/1,
+         flush/1]).
 
 -define(stat, diameter_stats).
--define(util, diameter_util).
 
 %% ===========================================================================
 
@@ -49,8 +50,10 @@ groups() ->
     [{all, [], tc()}].
 
 tc() ->
-    [an,
-     twa].
+    [reg,
+     incr,
+     read,
+     flush].
 
 init_per_suite(Config) ->
     ok = diameter:start(),
@@ -61,25 +64,62 @@ end_per_suite(_Config) ->
 
 %% ===========================================================================
 
-an(_) ->
-    Ref = {'_', make_ref()},
+reg(_) ->
+    Ref = '$1',
     true = ?stat:reg(Ref),
-    true = ?stat:reg(Ref),  %% duplicate
-    ok = ?stat:incr(x),
-    ok = ?stat:incr(x, Ref),
-    ok = ?stat:incr(y, 2),
-    ok = ?stat:incr(y, Ref),
-    %% Flushing a pid flushes even stats on the registered reference.
-    [{x,2},{y,3}] = lists:sort(?stat:flush()),
-    [] = ?stat:flush(Ref),
-    [] = ?stat:flush().
-
-twa(_) ->
+    false = ?stat:reg(Ref).  %% duplicate
+
+incr(_) ->    
+    Ref = '_',
+    Ctr = x,
+    false = ?stat:incr(Ctr),      %% not registered,
+    1 = ?stat:incr(Ctr, Ref, 1),  %% only pids need register
+    true = ?stat:reg(Ref),
+    spawn(fun() ->
+                  true = ?stat:reg(Ref),
+                  2 = ?stat:incr(Ctr, self(), 2)
+          end),
+    ok = fold(Ctr, Ref, 3),  %% folded
+    ?stat:flush([self(), Ref]).
+
+read(_) ->
+    Ref = make_ref(),
+    C1 = {a,b},
+    C2 = {b,a},
+    true = ?stat:reg(Ref),
+    1 = ?stat:incr(C1),
+    1 = ?stat:incr(C2),
+    2 = ?stat:incr(C1),
+    7 = ?stat:incr(C1, Ref, 7),
+    Self = self(),
+    [{Ref,  [{C1,7}]}, {Self, [{C1,2}, {C2,1}]}]
+        = lists:sort(?stat:read([self(), Ref, make_ref()])),
+    [] = ?stat:read([]),
+    [] = ?stat:read([make_ref()]),
+    ?stat:flush([self(), Ref, make_ref()]).
+
+flush(_) ->
     Ref = make_ref(),
-    ok = ?stat:incr(x, 8),
-    ok = ?stat:incr(x, Ref, 7),
-    %% Flushing a reference doesn't affect registered pids.
-    [{x,7}] = ?stat:flush(Ref),
-    [] = ?stat:flush(Ref),
-    [{x,8}] = ?stat:flush(),
-    [] = ?stat:flush().
+    Ctr = '_',
+    true = ?stat:reg(Ref),
+    1 = ?stat:incr(Ctr),
+    3 = ?stat:incr(Ctr, self(), 2),
+    2 = ?stat:incr(Ctr, Ref, 2),
+    Self = self(),
+    [{Self, [{Ctr, 3}]}] = ?stat:flush([self()]),
+    1 = ?stat:incr(Ctr),
+    [{Ref,  [{Ctr, 2}]}] = ?stat:flush([Ref]),
+    [{Self, [{Ctr, 1}]}] = ?stat:flush([self()]),
+    [] = ?stat:flush([self(), Ref]).
+
+%% ===========================================================================
+
+%% Keep incremented until a fold results in the specified value.
+fold(Ctr, Ref, N) ->
+    case ?stat:incr(Ctr, Ref, 0) of
+        N ->
+            ok;
+        M when M < N ->
+            erlang:yield(),
+            fold(Ctr, Ref, N)
+    end.