mirage / prometheus

OCaml library for reporting metrics to a Prometheus server
Apache License 2.0
49 stars 27 forks source link

Register lwt pre-collectors and collectors #41

Closed killian-delarue closed 2 years ago

killian-delarue commented 2 years ago

We are currently exporting metrics from the tezos node. There are functions that needs to be registered as pre-collectors into a collector registry. Unfortunatly, these functions must return a type unit Lwt.t. It would be useful to be able to register such a function.

A proof of concept is avaible on this branch with a use case. The modified version of Prometheus is vendored in vendors/prometheus and vendors/prometheus-app.

This poc adds to CollectorRegistry register_lwt and register_pre_collect_lwt, to register the lwt collectors and pre-collectors, and collect_lwt to read the current value of the metrics that have been registered with lwt collectors.

samoht commented 2 years ago

Could you open a PR with the diff between vendor/prometheus and this repository? It's a bit hard to see the changes you had to make to adapt it to Lwt otherwise. (or at least point to a separate commit on tezos/tezos which only your changes).

killian-delarue commented 2 years ago

I made a commit with only the changes. I will also make a PR out of it.

talex5 commented 2 years ago

Having async collectors sounds reasonable.

However, with effects being added to OCaml 5.0, we really want to remove Lwt from the main prometheus module completely, so I'm nervous about adding a new API knowing we'll be removing it again shortly! With effects, the existing register_pre_collect will work fine.

vect0r-vicall commented 2 years ago

Indeed, I understand your concern. In the case of Octez, I'm not sure that we will be able to support OCaml 5.0 in a short term perspective, and Lwt will be a dependency during a while. To me, and as we needed it "rapidly", we have the following options:

samoht commented 2 years ago

Do you think that would be possible to add a separate prometheus.lwt module?

talex5 commented 2 years ago

Do you think that would be possible to add a separate prometheus.lwt module?

I think it's OK to add the lwt collector to the main prometheus library for now. Just be aware that it will likely become deprecated once 5.0 is out, and will disappear a while after that. This library doesn't change much, so having OCaml 4.x stuck on an older release probably won't be a problem.

So the plan would be:

  1. Add lwt collector function now.
  2. When 5.0 comes out, deprecate Lwt functions in prometheus. Update prometheus_app to Eio (using Lwt_eio to ensure deprecated functions continue to work).
  3. Remove deprecated functions. prometheus itself no longer depends on any async library (lwt or eio). prometheus_app no longer requires lwt_eio.
killian-delarue commented 2 years ago

So is it Ok if I make a PR out of the commit I linked ?

samoht commented 2 years ago

Yes please :-)