deadtrickster / prometheus.erl

Prometheus.io client in Erlang
MIT License
341 stars 117 forks source link

mixing floats and ints result in infinite loop #61

Closed massemanet closed 6 years ago

massemanet commented 7 years ago

this snippet will send prometheus into an infinite loop.

1> application:ensure_all_started(prometheus).
{ok,[prometheus]}
2> prometheus_counter:declare([{name,nejm},{help,"help"}]).
true
3> prometheus_counter:dinc(nejm,1.0).
ok
4>  prometheus_counter:inc(nejm,1).

I realize mixing floats and ints are wrong (I did encounter this due to a bug), but infinite loops are not a great way to handle bad input.

the offending code https://github.com/deadtrickster/prometheus.erl/blob/master/src/metrics/prometheus_counter.erl#L196 assumes that the badarg implies that the key does not exist (whereas in this case the key exists but the value is of the wrong type)

deadtrickster commented 7 years ago

infinite loops are not a great way to handle bad input.

Totally agree!

Thanks for the report, will take a look shortly

deadtrickster commented 7 years ago

btw 1.0 to some extent is just 1. Maybe introduce auto conversion?

massemanet commented 7 years ago

auto conversion sounds like a good idea to me

deadtrickster commented 7 years ago

so this is a proposed fix:

if badarg was raised, before trying to insert metric I'll add prometheus_counter:value call. If it returns undefined - metric isn't initialized yet and call insert_metric, otherwise the value must be float -> exception

still thinking about auto conversion: since I use is_integer guard anyway I can add new case for Arg==trunc(Arg). The question is "is it really needed?"

massemanet commented 7 years ago

that fix ought to solve it.

that trunc looks surprising. probably better to just crash if there is a float.

perhaps lose the first function clause (duplicated error msg) and move the 'ok' inside the try (makes the code appear non-tail recursive)?

inc(Registry, Name, LabelValues, Value) when is_integer(Value), 0 =< Value ->
  try
    ets:update_counter(?TABLE,
                       key(Registry, Name, LabelValues),
                       {?SUM_POS, Value}),
    ok
  catch error:badarg ->
      insert_metric(Registry, Name, LabelValues, Value, fun inc/4)
  end;
deadtrickster commented 6 years ago

This should be fixed starting from version 4.0.0