Optimise handling of integers and floats for counters, gauges, summaries

NelsonVides · NelsonVides · commit 09b95f8e4e40 · 2025-03-17T15:58:34.000+01:00
The point is that these tables keep separate positions in the tuple for
integers and for floats, as on the integer position we can use the
optimised `ets:update_counter/3,4`, and probabilistically integers are
the most common input.

The issue is that under some circumstances, integer values would be
appended to the float position, which, while correct, misses the
opportunity for the optimisation.

Note too that gauges could take the value `undefined` to effectively
block changes until explicitly set. This was implemented, but not
tested, and in some edge-cases (the integer/float optimisation) it would
be buggy.
diff --git a/src/metrics/prometheus_counter.erl b/src/metrics/prometheus_counter.erl
@@ -87,6 +87,10 @@ inc(Caller) ->
 -behaviour(prometheus_metric).
 -behaviour(prometheus_collector).
 
+%% Records will be triplets `{Key, ISum, FSum}`
+%%  where `ISum` are integer sums, and `FSum` are float sums
+%% Keys use the scheduler index, because counters by definition aggregate
+%%  even after parallel updates. Hence we can run updates without locks.
 -define(TABLE, ?PROMETHEUS_COUNTER_TABLE).
 -define(ISUM_POS, 2).
 -define(FSUM_POS, 3).
@@ -180,23 +184,23 @@ Raises:
     Registry :: prometheus_registry:registry(),
     Name :: prometheus_metric:name(),
     LabelValues :: prometheus_metric:labels(),
-    Value :: non_neg_integer().
+    Value :: non_neg_integer() | float().
 inc(Registry, Name, LabelValues, Value) when is_integer(Value), Value >= 0 ->
     Key = key(Registry, Name, LabelValues),
     Spec = {?ISUM_POS, Value},
     try
         ets:update_counter(?TABLE, Key, Spec)
     catch
         error:badarg ->
-            insert_metric(Registry, Name, LabelValues, Value, fun inc/4)
+            insert_metric_int(Registry, Name, LabelValues, Value, fun inc/4)
     end,
     ok;
-inc(Registry, Name, LabelValues, Value) when is_number(Value), Value >= 0 ->
+inc(Registry, Name, LabelValues, Value) when is_float(Value), Value >= 0 ->
     Key = key(Registry, Name, LabelValues),
     Spec = [{{Key, '$1', '$2'}, [], [{{{Key}, '$1', {'+', '$2', Value}}}]}],
     case ets:select_replace(?TABLE, Spec) of
         0 ->
-            insert_metric(Registry, Name, LabelValues, Value, fun inc/4);
+            insert_metric_float(Registry, Name, LabelValues, Value, fun inc/4);
         1 ->
             ok
     end;
@@ -349,9 +353,16 @@ collect_metrics(Name, {CLabels, Labels, Registry}) ->
 deregister_select(Registry, Name) ->
     [{{{Registry, Name, '_', '_'}, '_', '_'}, [], [true]}].
 
-insert_metric(Registry, Name, LabelValues, Value, ConflictCB) ->
-    prometheus_metric:check_mf_exists(?TABLE, Registry, Name, LabelValues),
+insert_metric_int(Registry, Name, LabelValues, Value, ConflictCB) ->
+    Counter = {key(Registry, Name, LabelValues), Value, 0},
+    insert_metric(Registry, Name, LabelValues, Value, ConflictCB, Counter).
+
+insert_metric_float(Registry, Name, LabelValues, Value, ConflictCB) ->
     Counter = {key(Registry, Name, LabelValues), 0, Value},
+    insert_metric(Registry, Name, LabelValues, Value, ConflictCB, Counter).
+
+insert_metric(Registry, Name, LabelValues, Value, ConflictCB, Counter) ->
+    prometheus_metric:check_mf_exists(?TABLE, Registry, Name, LabelValues),
     case ets:insert_new(?TABLE, Counter) of
         %% some sneaky process already inserted
         false ->
diff --git a/src/metrics/prometheus_gauge.erl b/src/metrics/prometheus_gauge.erl
@@ -15,9 +15,13 @@ Gauge is a metric that represents a single numerical value that can arbitrarily
 A Gauge is typically used for measured values like temperatures or current memory usage,
 but also \"counts\" that can go up and down, like the number of running processes.
 
+It can take the value of undefined when the value is not known.
+In that case, increments and decrements will not be possible
+and explicitly setting to a new value will be necessary.
+
 Example use cases for Gauges:
 
-* Inprogress requests
+* In-progress requests
 * Number of items in a queue
 * Free memory
 * Total memory
@@ -95,6 +99,8 @@ track_checked_out_sockets(CheckoutFun) ->
 -behaviour(prometheus_metric).
 -behaviour(prometheus_collector).
 
+%% Records will be triplets `{Key, ISum, FSum}`
+%%  where `IGauge` are integer gauges, and `FGauge` are float gauges
 -define(TABLE, ?PROMETHEUS_GAUGE_TABLE).
 -define(IGAUGE_POS, 2).
 -define(FGAUGE_POS, 3).
@@ -183,23 +189,33 @@ Raises:
     Registry :: prometheus_registry:registry(),
     Name :: prometheus_metric:name(),
     LabelValues :: prometheus_metric:labels(),
-    Value :: number().
-set(Registry, Name, LabelValues, Value) ->
-    Update =
-        case Value of
-            _ when is_number(Value) ->
-                [{?IGAUGE_POS, 0}, {?FGAUGE_POS, Value}];
-            undefined ->
-                [{?IGAUGE_POS, undefined}, {?FGAUGE_POS, undefined}];
-            _ ->
-                erlang:error({invalid_value, Value, "set accepts only numbers and 'undefined'"})
-        end,
+    Value :: undefined | number().
+set(Registry, Name, LabelValues, Value) when is_integer(Value) ->
+    Update = [{?IGAUGE_POS, Value}, {?FGAUGE_POS, 0}],
     case ets:update_element(?TABLE, {Registry, Name, LabelValues}, Update) of
         false ->
-            insert_metric(Registry, Name, LabelValues, Value, fun set/4);
+            insert_metric_int(Registry, Name, LabelValues, Value, fun set/4);
         true ->
             ok
-    end.
+    end;
+set(Registry, Name, LabelValues, Value) when is_float(Value) ->
+    Update = [{?IGAUGE_POS, 0}, {?FGAUGE_POS, Value}],
+    case ets:update_element(?TABLE, {Registry, Name, LabelValues}, Update) of
+        false ->
+            insert_metric_float(Registry, Name, LabelValues, Value, fun set/4);
+        true ->
+            ok
+    end;
+set(Registry, Name, LabelValues, undefined) ->
+    Update = [{?IGAUGE_POS, undefined}, {?FGAUGE_POS, undefined}],
+    case ets:update_element(?TABLE, {Registry, Name, LabelValues}, Update) of
+        false ->
+            insert_metric_undefined(Registry, Name, LabelValues, undefined, fun set/4);
+        true ->
+            ok
+    end;
+set(_, _, _, Value) ->
+    erlang:error({invalid_value, Value, "set accepts only numbers and 'undefined'"}).
 
 ?DOC(#{equiv => inc(default, Name, [], 1)}).
 -spec inc(prometheus_metric:name()) -> ok.
@@ -236,19 +252,21 @@ Raises:
     LabelValues :: prometheus_metric:labels(),
     Value :: number().
 inc(Registry, Name, LabelValues, Value) when is_integer(Value) ->
+    Key = key(Registry, Name, LabelValues),
+    Spec = {?IGAUGE_POS, Value},
     try
-        ets:update_counter(?TABLE, {Registry, Name, LabelValues}, {?IGAUGE_POS, Value})
+        ets:update_counter(?TABLE, Key, Spec)
     catch
         error:badarg ->
-            maybe_insert_metric_for_inc(Registry, Name, LabelValues, Value)
+            maybe_insert_metric_for_inc_int(Registry, Name, LabelValues, Value)
     end,
     ok;
-inc(Registry, Name, LabelValues, Value) when is_number(Value) ->
+inc(Registry, Name, LabelValues, Value) when is_float(Value) ->
     Key = key(Registry, Name, LabelValues),
-    Spec = [{{Key, '$1', '$2'}, [], [{{{Key}, '$1', {'+', '$2', Value}}}]}],
+    Spec = [{{Key, '$1', '$2'}, [{is_integer, '$1'}], [{{{Key}, '$1', {'+', '$2', Value}}}]}],
     case ets:select_replace(?TABLE, Spec) of
         0 ->
-            insert_metric(Registry, Name, LabelValues, Value, fun inc/4);
+            maybe_insert_metric_for_inc_float(Registry, Name, LabelValues, Value);
         1 ->
             ok
     end;
@@ -522,20 +540,40 @@ collect_metrics(Name, {CLabels, Labels, Registry, DU}) ->
 key(Registry, Name, LabelValues) ->
     {Registry, Name, LabelValues}.
 
-maybe_insert_metric_for_inc(Registry, Name, LabelValues, Value) ->
+maybe_insert_metric_for_inc_int(Registry, Name, LabelValues, Value) ->
+    case ets:lookup(?TABLE, {Registry, Name, LabelValues}) of
+        [{_Key, undefined, undefined}] ->
+            erlang:error({invalid_operation, 'inc/dec', "Can't inc/dec undefined"});
+        _ ->
+            insert_metric_int(Registry, Name, LabelValues, Value, fun inc/4)
+    end.
+
+maybe_insert_metric_for_inc_float(Registry, Name, LabelValues, Value) ->
     case ets:lookup(?TABLE, {Registry, Name, LabelValues}) of
         [{_Key, undefined, undefined}] ->
             erlang:error({invalid_operation, 'inc/dec', "Can't inc/dec undefined"});
         _ ->
-            insert_metric(Registry, Name, LabelValues, Value, fun inc/4)
+            insert_metric_float(Registry, Name, LabelValues, Value, fun inc/4)
     end.
 
 deregister_select(Registry, Name) ->
     [{{{Registry, Name, '_'}, '_', '_'}, [], [true]}].
 
-insert_metric(Registry, Name, LabelValues, Value, ConflictCB) ->
+insert_metric_int(Registry, Name, LabelValues, Value, ConflictCB) ->
+    Counter = {key(Registry, Name, LabelValues), Value, 0},
+    insert_metric(Registry, Name, LabelValues, Value, ConflictCB, Counter).
+
+insert_metric_float(Registry, Name, LabelValues, Value, ConflictCB) ->
+    Counter = {key(Registry, Name, LabelValues), 0, Value},
+    insert_metric(Registry, Name, LabelValues, Value, ConflictCB, Counter).
+
+insert_metric_undefined(Registry, Name, LabelValues, Value, ConflictCB) ->
+    Counter = {key(Registry, Name, LabelValues), undefined, undefined},
+    insert_metric(Registry, Name, LabelValues, Value, ConflictCB, Counter).
+
+insert_metric(Registry, Name, LabelValues, Value, ConflictCB, Counter) ->
     prometheus_metric:check_mf_exists(?TABLE, Registry, Name, LabelValues),
-    case ets:insert_new(?TABLE, {{Registry, Name, LabelValues}, 0, Value}) of
+    case ets:insert_new(?TABLE, Counter) of
         %% some sneaky process already inserted
         false ->
             ConflictCB(Registry, Name, LabelValues, Value);
@@ -546,7 +584,7 @@ insert_metric(Registry, Name, LabelValues, Value, ConflictCB) ->
 load_all_values(Registry, Name) ->
     ets:match(?TABLE, {{Registry, Name, '$1'}, '$2', '$3'}).
 
-sum(_IValue, undefined) ->
+sum(undefined, _FValue) ->
     undefined;
 sum(IValue, FValue) ->
     IValue + FValue.
diff --git a/src/metrics/prometheus_summary.erl b/src/metrics/prometheus_summary.erl
@@ -15,6 +15,10 @@ Example use cases for Summaries:
 * Request size;
 * Response size.
 
+This keeps track of the number of events, and the sum of their values.
+This allows you to calculate the average value of each event,
+and with enough datapoints, to keep track of its average.
+
 Example:
 
 ```erlang
@@ -72,20 +76,11 @@ observe_response(Size) ->
 -behaviour(prometheus_collector).
 
 -define(TABLE, ?PROMETHEUS_SUMMARY_TABLE).
+-define(COUNTER_POS, 2).
 -define(ISUM_POS, 3).
 -define(FSUM_POS, 4).
--define(COUNTER_POS, 2).
 -define(WIDTH, 16).
 
--type conflict_cb() :: fun(
-    (
-        prometheus_registry:registry(),
-        prometheus_metric:name(),
-        prometheus_metric:labels(),
-        number()
-    ) -> ok
-).
-
 ?DOC("""
 Creates a summary using `Spec`.
 
@@ -173,15 +168,15 @@ observe(Registry, Name, LabelValues, Value) when is_integer(Value) ->
         ets:update_counter(?TABLE, Key, Spec)
     catch
         error:badarg ->
-            insert_metric(Registry, Name, LabelValues, Value, fun observe/4)
+            insert_metric_int(Registry, Name, LabelValues, Value, fun observe/4)
     end,
     ok;
 observe(Registry, Name, LabelValues, Value) when is_number(Value) ->
     Key = key(Registry, Name, LabelValues),
     Spec = [{{Key, '$1', '$2', '$3'}, [], [{{{Key}, {'+', '$1', 1}, '$2', {'+', '$3', Value}}}]}],
     case ets:select_replace(?TABLE, Spec) of
         0 ->
-            insert_metric(Registry, Name, LabelValues, Value, fun observe/4);
+            insert_metric_float(Registry, Name, LabelValues, Value, fun observe/4);
         1 ->
             ok
     end;
@@ -429,15 +424,17 @@ raise_error_if_quantile_label_found("quantile") ->
 raise_error_if_quantile_label_found(Label) ->
     Label.
 
--spec insert_metric(Registry, Name, LabelValues, Value, ConflictCB) -> ok when
-    Registry :: prometheus_registry:registry(),
-    Name :: prometheus_metric:name(),
-    LabelValues :: prometheus_metric:labels(),
-    Value :: number(),
-    ConflictCB :: conflict_cb().
-insert_metric(Registry, Name, LabelValues, Value, ConflictCB) ->
+insert_metric_int(Registry, Name, LabelValues, Value, ConflictCB) ->
+    Counter = {key(Registry, Name, LabelValues), 1, Value, 0},
+    insert_metric(Registry, Name, LabelValues, Value, ConflictCB, Counter).
+
+insert_metric_float(Registry, Name, LabelValues, Value, ConflictCB) ->
+    Counter = {key(Registry, Name, LabelValues), 1, 0, Value},
+    insert_metric(Registry, Name, LabelValues, Value, ConflictCB, Counter).
+
+insert_metric(Registry, Name, LabelValues, Value, ConflictCB, Counter) ->
     prometheus_metric:check_mf_exists(?TABLE, Registry, Name, LabelValues),
-    case ets:insert_new(?TABLE, {key(Registry, Name, LabelValues), 1, 0, Value}) of
+    case ets:insert_new(?TABLE, Counter) of
         %% some sneaky process already inserted
         false ->
             ConflictCB(Registry, Name, LabelValues, Value);
diff --git a/test/eunit/metric/prometheus_gauge_tests.erl b/test/eunit/metric/prometheus_gauge_tests.erl
@@ -9,6 +9,7 @@ prometheus_format_test_() ->
         fun test_registration/1,
         fun test_errors/1,
         fun test_set/1,
+        fun test_set_undefined/1,
         fun test_inc/1,
         fun test_dec/1,
         fun test_set_to_current_time/1,
@@ -44,6 +45,7 @@ test_registration(_) ->
 
 test_errors(_) ->
     prometheus_gauge:new([{name, with_label}, {labels, [label]}, {help, ""}]),
+    prometheus_gauge:new([{name, no_labels}, {help, ""}]),
 
     %% basic name/labels/help validations test
     [
@@ -69,10 +71,19 @@ test_errors(_) ->
             {invalid_value, "qwe", "inc accepts only numbers"},
             prometheus_gauge:inc(pool_size, [], "qwe")
         ),
+        ?_assertError(
+            {invalid_value, <<"qwe">>, "dec accepts only numbers"},
+            prometheus_gauge:dec(no_labels, <<"qwe">>)
+        ),
         ?_assertError(
             {invalid_value, "qwe", "dec accepts only numbers"},
             prometheus_gauge:dec(pool_size, [], "qwe")
         ),
+        ?_assertError(
+            {invalid_value, "qwe", "dec accepts only numbers"},
+            prometheus_gauge:dec(default, pool_size, [], "qwe")
+        ),
+
         ?_assertError(
             {invalid_value, "qwe", "track_inprogress accepts only functions"},
             prometheus_gauge:track_inprogress(pool_size, "qwe")
@@ -157,18 +168,40 @@ test_errors(_) ->
 
 test_set(_) ->
     prometheus_gauge:new([{name, pool_size}, {labels, [client]}, {help, ""}]),
+    prometheus_gauge:set(pool_size, [redis], undefined),
+    ValueUndefined = prometheus_gauge:value(pool_size, [redis]),
+    prometheus_gauge:set(pool_size, [float_pool], 92.3),
+    ValueFloat = prometheus_gauge:value(pool_size, [float_pool]),
     prometheus_gauge:set(pool_size, [mongodb], 100),
-    Value = prometheus_gauge:value(pool_size, [mongodb]),
-    prometheus_gauge:set(pool_size, [mongodb], 105),
     Value1 = prometheus_gauge:value(pool_size, [mongodb]),
+    prometheus_gauge:set(pool_size, [mongodb], 105),
+    Value2 = prometheus_gauge:value(pool_size, [mongodb]),
     prometheus_gauge:reset(pool_size, [mongodb]),
     RValue = prometheus_gauge:value(pool_size, [mongodb]),
     [
-        ?_assertEqual(100, Value),
-        ?_assertEqual(105, Value1),
+        ?_assertEqual(undefined, ValueUndefined),
+        ?_assertEqual(92.3, ValueFloat),
+        ?_assertEqual(100, Value1),
+        ?_assertEqual(105, Value2),
         ?_assertEqual(0, RValue)
     ].
 
+test_set_undefined(_) ->
+    prometheus_gauge:new([{name, pool_size}, {labels, [client]}, {help, ""}]),
+    prometheus_gauge:set(pool_size, [redis], undefined),
+    ValueUndefined = prometheus_gauge:value(pool_size, [redis]),
+    [
+        ?_assertError(
+            {invalid_operation, 'inc/dec', "Can't inc/dec undefined"},
+            prometheus_gauge:inc(pool_size, [redis], 1)
+        ),
+        ?_assertError(
+            {invalid_operation, 'inc/dec', "Can't inc/dec undefined"},
+            prometheus_gauge:inc(pool_size, [redis], 0.8)
+        ),
+        ?_assertEqual(undefined, ValueUndefined)
+    ].
+
 test_inc(_) ->
     prometheus_gauge:new([{name, pool_size}, {labels, [client]}, {help, ""}]),
     prometheus_gauge:new([{name, temperature}, {help, ""}]),
