Add API to set default value of labeled metrics - #203
Conversation
|
Ping :) |
|
Woops, part of the delay was my holidays, part I didn't notice! Thanks a lot for the contribution, noted as a TODO for next week 💪🏽 |
|
In the meantime, note CI please 😄 I updated the master branch to run CI, feel free to rebase on top of the latest master and remove the merge commits 👌🏽 |
NelsonVides
left a comment
There was a problem hiding this comment.
A few comments, and thanks again for the PR and sorry for the delay :)
| ?DOC("Sets the default metric function for the module."). | ||
| -callback set_default(Registry, Name) -> dynamic() when | ||
| Registry :: prometheus_registry:registry(), | ||
| Name :: name(). | ||
| ?DOC("Sets the default (zero/initial) state for the metric series identified by Registry, Name and LabelValues."). | ||
| -callback set_default(Registry :: prometheus_registry:registry(), | ||
| Name :: name(), | ||
| LabelValues :: label_values()) -> | ||
| dynamic(). |
There was a problem hiding this comment.
An issue with this is that it is a breaking change and according to SemVer I'd have to release v7, when it doesn't need to be. I think that set_default/2 might continue to exist nevertheless, and by default call set_default/3 with an empty list of labels. That way this PR only adds a new callback without removing an existing one, therefore not breaking any behaviour.
| LabelValues :: prometheus_metric:label_values() | ||
| ) -> ok. | ||
| set_default(Registry, Name, LabelValues) -> | ||
| set(Registry, Name, LabelValues, undefined). |
There was a problem hiding this comment.
This actually looks like an old hidden bug... all other set_default changes use ets:insert_new/ while only here we will do ets:insert/ and potentially override already existing values 🫣
Could you try a fix with a regression test for this one? 🙏🏽
| Idempotent = prometheus_counter:set_default(default, http_requests_total, [get]), | ||
| AfterIdempotent = prometheus_counter:value(http_requests_total, [get]), |
There was a problem hiding this comment.
We should be also calling set and then checking that set_default would not override an already existing value here
This PR adds a capability to initialize a metric with labels with a default value for specific label values. This is not done automatically, as label values are not known at the moment of metric creation, so it's up to the application whether these should be set to initial default value or not.