Merge pull request #183 from prometheus-erl/improvements

Author: NelsonVides

src/metrics/prometheus_gauge.erl

@@ -226,14 +226,14 @@ inc(Name) ->
 If the second argument is a list, equivalent to [inc(default, Name, LabelValues, 1)](`inc/4`)
 otherwise equivalent to [inc(default, Name, [], Value)](`inc/4`).
 """).
--spec inc(prometheus_metric:name(), prometheus_metric:label_values() | non_neg_integer()) -> ok.
+-spec inc(prometheus_metric:name(), prometheus_metric:label_values() | number()) -> ok.
 inc(Name, LabelValues) when is_list(LabelValues) ->
     inc(default, Name, LabelValues, 1);
 inc(Name, Value) ->
     inc(default, Name, [], Value).
 
 ?DOC(#{equiv => inc(default, Name, LabelValues, Value)}).
--spec inc(prometheus_metric:name(), prometheus_metric:label_values(), non_neg_integer()) -> ok.
+-spec inc(prometheus_metric:name(), prometheus_metric:label_values(), number()) -> ok.
 inc(Name, LabelValues, Value) ->
     inc(default, Name, LabelValues, Value).
 
@@ -484,10 +484,12 @@ Raises:
     LabelValues :: prometheus_metric:label_values().
 value(Registry, Name, LabelValues) ->
     MF = prometheus_metric:check_mf_exists(?TABLE, Registry, Name, LabelValues),
-    DU = prometheus_metric:mf_duration_unit(MF),
     case ets:lookup(?TABLE, {Registry, Name, LabelValues}) of
-        [{_Key, IValue, FValue}] -> prometheus_time:maybe_convert_to_du(DU, sum(IValue, FValue));
-        [] -> undefined
+        [{_Key, IValue, FValue}] ->
+            DU = prometheus_metric:mf_duration_unit(MF),
+            prometheus_time:maybe_convert_to_du(DU, sum(IValue, FValue));
+        [] ->
+            undefined
     end.
 
 -spec values(prometheus_registry:registry(), prometheus_metric:name()) ->

src/metrics/prometheus_histogram.erl

@@ -241,27 +241,24 @@ Raises:
     Count :: integer().
 observe_n(Registry, Name, LabelValues, Value, Count) when is_integer(Value), is_integer(Count) ->
     Key = key(Registry, Name, LabelValues),
-    case ets:lookup(?TABLE, Key) of
-        [Metric] ->
-            BucketPosition = calculate_histogram_bucket_position(Metric, Value),
-            ets:update_counter(
-                ?TABLE,
-                Key,
-                [
-                    {?ISUM_POS, Value * Count},
-                    {?BUCKETS_START + BucketPosition, Count}
-                ]
-            );
-        [] ->
+    try ets:lookup_element(?TABLE, Key, ?BOUNDS_POS) of
+        Buckets ->
+            BucketPosition = prometheus_buckets:position(Buckets, Value),
+            Spec = [{?ISUM_POS, Value * Count}, {?BUCKETS_START + BucketPosition, Count}],
+            ets:update_counter(?TABLE, Key, Spec)
+    catch
+        error:badarg ->
             insert_metric(Registry, Name, LabelValues, Value, Count, fun observe_n/5)
     end,
     ok;
 observe_n(Registry, Name, LabelValues, Value, Count) when is_float(Value), is_integer(Count) ->
     Key = key(Registry, Name, LabelValues),
-    case ets:lookup(?TABLE, Key) of
-        [Metric] ->
-            fobserve_impl(Key, Metric, Value, Count);
-        [] ->
+    try ets:lookup_element(?TABLE, Key, ?BOUNDS_POS) of
+        Buckets ->
+            BucketPosition = prometheus_buckets:position(Buckets, Value),
+            fobserve_impl(Key, Buckets, BucketPosition, Value, Count)
+    catch
+        error:badarg ->
             insert_metric(Registry, Name, LabelValues, Value, Count, fun observe_n/5)
     end;
 observe_n(_Registry, _Name, _LabelValues, Value, Count) when is_number(Value) ->
@@ -445,7 +442,6 @@ Raises:
     LabelValues :: prometheus_metric:label_values().
 value(Registry, Name, LabelValues) ->
     MF = prometheus_metric:check_mf_exists(?TABLE, Registry, Name, LabelValues),
-
     RawValues = [
         ets:lookup(?TABLE, {Registry, Name, LabelValues, Scheduler})
      || Scheduler <- schedulers_seq()
@@ -542,11 +538,6 @@ insert_metric(Registry, Name, LabelValues, Value, Count, CB) ->
     insert_placeholders(Registry, Name, LabelValues),
     CB(Registry, Name, LabelValues, Value, Count).
 
-fobserve_impl(Key, Metric, Value, Count) ->
-    Buckets = metric_buckets(Metric),
-    BucketPos = calculate_histogram_bucket_position(Metric, Value),
-    fobserve_impl(Key, Buckets, BucketPos, Value, Count).
-
 fobserve_impl(Key, Buckets, BucketPos, Value, Count) ->
     ets:select_replace(?TABLE, generate_select_replace(Key, Buckets, BucketPos, Value, Count)).
 
@@ -558,33 +549,33 @@ insert_placeholders(Registry, Name, LabelValues) ->
         prometheus_time:maybe_convert_to_native(DU, Bucket)
     end,
     BoundCounters = lists:duplicate(length(MFBuckets), 0),
+    Buckets = list_to_tuple(lists:map(Fun, MFBuckets)),
     MetricSpec =
-        [key(Registry, Name, LabelValues), lists:map(Fun, MFBuckets), 0, 0] ++
-            BoundCounters,
+        [key(Registry, Name, LabelValues), Buckets, 0, 0 | BoundCounters],
     ets:insert_new(?TABLE, list_to_tuple(MetricSpec)).
 
-calculate_histogram_bucket_position(Metric, Value) ->
-    Buckets = metric_buckets(Metric),
-    prometheus_buckets:position(Buckets, Value).
-
 generate_select_replace(Key, Bounds, BucketPos, Value, Count) ->
     BoundPlaceholders = gen_query_bound_placeholders(Bounds),
-    HistMatch = list_to_tuple([Key, '$2', '$3', '$4'] ++ BoundPlaceholders),
+    HistMatch = list_to_tuple([Key, '$2', '$3', '$4' | BoundPlaceholders]),
     BucketUpdate =
         lists:sublist(BoundPlaceholders, BucketPos) ++
-            [{'+', gen_query_placeholder(?BUCKETS_START + BucketPos), Count}] ++
-            lists:nthtail(BucketPos + 1, BoundPlaceholders),
-    HistUpdate = list_to_tuple([{Key}, '$2', '$3', {'+', '$4', Value * Count}] ++ BucketUpdate),
+            [
+                {'+', gen_query_placeholder(?BUCKETS_START + BucketPos), Count}
+                | lists:nthtail(BucketPos + 1, BoundPlaceholders)
+            ],
+    HistUpdate = list_to_tuple([{Key}, '$2', '$3', {'+', '$4', Value * Count} | BucketUpdate]),
     [{HistMatch, [], [{HistUpdate}]}].
 
-buckets_seq(Buckets) ->
-    lists:seq(?BUCKETS_START, ?BUCKETS_START + length(Buckets) - 1).
+buckets_seq(Buckets) when is_list(Buckets) ->
+    lists:seq(?BUCKETS_START, ?BUCKETS_START + length(Buckets) - 1);
+buckets_seq(Buckets) when is_tuple(Buckets) ->
+    lists:seq(?BUCKETS_START, ?BUCKETS_START + tuple_size(Buckets) - 1).
 
 generate_update_spec(Buckets) ->
     [{Index, 0} || Index <- buckets_seq(Buckets)].
 
 gen_query_placeholder(Index) ->
-    list_to_atom("$" ++ integer_to_list(Index)).
+    list_to_atom([$$ | integer_to_list(Index)]).
 
 gen_query_bound_placeholders(Buckets) ->
     [gen_query_placeholder(Index) || Index <- buckets_seq(Buckets)].
@@ -593,9 +584,9 @@ augment_counters([Start | Counters]) ->
     augment_counters(Counters, [Start], Start).
 
 augment_counters([], LAcc, _CAcc) ->
-    LAcc;
+    lists:reverse(LAcc);
 augment_counters([Counter | Counters], LAcc, CAcc) ->
-    augment_counters(Counters, LAcc ++ [CAcc + Counter], CAcc + Counter).
+    augment_counters(Counters, [CAcc + Counter | LAcc], CAcc + Counter).
 
 metric_buckets(Metric) ->
     element(?BOUNDS_POS, Metric).
@@ -606,7 +597,7 @@ reduce_buckets_counters(Metrics) ->
             sub_tuple_to_list(
                 Metric,
                 ?BUCKETS_START,
-                ?BUCKETS_START + length(metric_buckets(Metric))
+                ?BUCKETS_START + tuple_size(metric_buckets(Metric))
             )
          || Metric <- Metrics
         ],
@@ -636,17 +627,17 @@ create_histogram_metric(CLabels, Labels, DU, Bounds, LabelValues, [ISum, FSum |
 
 load_all_values(Registry, Name, Bounds) ->
     BoundPlaceholders = gen_query_bound_placeholders(Bounds),
-    QuerySpec = [{Registry, Name, '$1', '_'}, '_', '$3', '$4'] ++ BoundPlaceholders,
+    QuerySpec = [{Registry, Name, '$1', '_'}, '_', '$3', '$4' | BoundPlaceholders],
     ets:match(?TABLE, list_to_tuple(QuerySpec)).
 
 deregister_select(Registry, Name, Buckets) ->
     BoundCounters = lists:duplicate(length(Buckets), '_'),
-    MetricSpec = [{Registry, Name, '_', '_'}, '_', '_', '_'] ++ BoundCounters,
+    MetricSpec = [{Registry, Name, '_', '_'}, '_', '_', '_' | BoundCounters],
     [{list_to_tuple(MetricSpec), [], [true]}].
 
 delete_metrics(Registry, Buckets) ->
     BoundCounters = lists:duplicate(length(Buckets), '_'),
-    MetricSpec = [{Registry, '_', '_', '_'}, '_', '_', '_'] ++ BoundCounters,
+    MetricSpec = [{Registry, '_', '_', '_'}, '_', '_', '_' | BoundCounters],
     ets:match_delete(?TABLE, list_to_tuple(MetricSpec)).
 
 sub_tuple_to_list(Tuple, Pos, Size) when Pos < Size ->

src/prometheus.erl

@@ -70,7 +70,12 @@ stop(_State) ->
 ?DOC(false).
 -spec start() -> ok | {error, term()}.
 start() ->
-    application:start(?MODULE).
+    case application:ensure_all_started(?MODULE) of
+        {ok, _} ->
+            ok;
+        Error ->
+            Error
+    end.
 
 ?DOC(false).
 -spec stop() -> ok | {error, term()}.

src/prometheus_buckets.erl

@@ -130,9 +130,14 @@ linear(_Start, _Step, Count) when Count < 1 ->
 linear(Start, Step, Count) ->
     linear(Start, Step, Count, []).
 
--spec position(buckets(), number()) -> pos_integer().
+?DOC("""
+Find the first index that is greater than or equal to the given value.
+""").
+-spec position(buckets() | tuple(), number()) -> pos_integer().
 position(Buckets, Value) when is_list(Buckets), is_number(Value) ->
-    find_position(Buckets, Value, 0).
+    find_position(Buckets, Value, 0);
+position(Buckets, Value) when is_tuple(Buckets), 1 < tuple_size(Buckets), is_number(Value) ->
+    find_position_in_tuple(Buckets, Value, 1, tuple_size(Buckets)).
 
 linear(_Current, _Step, 0, Acc) ->
     lists:reverse(Acc);
@@ -155,7 +160,8 @@ try_to_maintain_integer_bounds(Bound) when is_float(Bound) ->
         false -> Bound
     end.
 
--spec find_position(buckets(), number(), non_neg_integer()) -> pos_integer().
+%% Find the first index that is greater than or equal to the given value.
+-spec find_position(buckets(), number(), non_neg_integer()) -> non_neg_integer().
 find_position([], _Value, _Pos) ->
     0;
 find_position([Bound | L], Value, Pos) ->
@@ -165,3 +171,20 @@ find_position([Bound | L], Value, Pos) ->
         false ->
             find_position(L, Value, Pos + 1)
     end.
+
+%% Find the first index that is greater than or equal to the given value.
+-spec find_position_in_tuple(tuple(), number(), non_neg_integer(), pos_integer()) ->
+    non_neg_integer().
+find_position_in_tuple(Tuple, Value, Low, High) when Low =< High ->
+    Mid = Low + (High - Low) div 2,
+    case element(Mid, Tuple) of
+        Element when Element < Value ->
+            find_position_in_tuple(Tuple, Value, Mid + 1, High);
+        Element when Value =< Element ->
+            find_position_in_tuple(Tuple, Value, Low, Mid - 1)
+    end;
+find_position_in_tuple(Tuple, _Value, Low, _High) ->
+    case tuple_size(Tuple) < Low of
+        true -> 0;
+        false -> Low - 1
+    end.

src/prometheus_time.erl

@@ -155,14 +155,16 @@ maybe_convert_to_native(DU, Value) ->
         _ -> to_native(Value, DU)
     end.
 
--spec maybe_convert_to_du(duration_unit(), infinity | number()) -> infinity | number().
+-spec maybe_convert_to_du(undefined | duration_unit(), undefined | infinity | number()) ->
+    undefined | infinity | number().
+maybe_convert_to_du(undefined, Value) ->
+    Value;
+maybe_convert_to_du(_, undefined) ->
+    undefined;
 maybe_convert_to_du(_, infinity) ->
     infinity;
 maybe_convert_to_du(DU, Value) ->
-    case DU of
-        undefined -> Value;
-        _ -> from_native(Value, DU)
-    end.
+    from_native(Value, DU).
 
 %%====================================================================
 %% Private Parts

test/eunit/metric/prometheus_gauge_tests.erl

@@ -16,6 +16,7 @@ prometheus_format_test_() ->
         fun test_track_inprogress/1,
         fun test_set_duration_seconds/1,
         fun test_set_duration_milliseconds/1,
+        fun test_get_value_from_undefined_with_float/1,
         fun test_deregister/1,
         fun test_remove/1,
         fun test_default_value/1,
@@ -345,6 +346,26 @@ test_set_duration_milliseconds(_) ->
         ?_assertMatch(true, 0 < ValueE andalso ValueE < 100)
     ].
 
+test_get_value_from_undefined_with_float(_) ->
+    prometheus_gauge:new([{name, gauge_seconds}, {labels, [client]}, {help, ""}]),
+    prometheus_gauge:inc(gauge_seconds, [redis], 2),
+    prometheus_gauge:inc(gauge_seconds, [redis], 1.2),
+    Value = prometheus_gauge:value(gauge_seconds, [redis]),
+    prometheus_gauge:set(gauge_seconds, [redis], undefined),
+    ValueUndefined = prometheus_gauge:value(gauge_seconds, [redis]),
+    [
+        ?_assertError(
+            {invalid_operation, 'inc/dec', "Can't inc/dec undefined"},
+            prometheus_gauge:inc(gauge_seconds, [redis], 1)
+        ),
+        ?_assertError(
+            {invalid_operation, 'inc/dec', "Can't inc/dec undefined"},
+            prometheus_gauge:inc(gauge_seconds, [redis], 0.8)
+        ),
+        ?_assertEqual(3.0e-9, Value),
+        ?_assertEqual(undefined, ValueUndefined)
+    ].
+
 test_deregister(_) ->
     prometheus_gauge:new([
         {name, pool_size},

test/eunit/prometheus_buckets_tests.erl

@@ -2,6 +2,26 @@
 
 -include_lib("eunit/include/eunit.hrl").
 
+find_position_test_() ->
+    Buckets = prometheus_buckets:linear(1, 1, 10),
+    [
+        ?_assertEqual(0, prometheus_buckets:position(Buckets, 100))
+        | [
+            ?_assertEqual(N - 1, prometheus_buckets:position(Buckets, N))
+         || N <- lists:seq(1, 10)
+        ]
+    ].
+
+find_position_in_tuple_test_() ->
+    TupleBuckets = list_to_tuple(prometheus_buckets:linear(1, 1, 10)),
+    [
+        ?_assertEqual(0, prometheus_buckets:position(TupleBuckets, 100))
+        | [
+            ?_assertEqual(N - 1, prometheus_buckets:position(TupleBuckets, N))
+         || N <- lists:seq(1, 10)
+        ]
+    ].
+
 linear_errors_test() ->
     ?assertError(
         {invalid_value, 0, "Buckets count should be positive"},