Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Register lwt pre-collectors and collectors #41

Closed
killian-delarue opened this issue May 11, 2022 · 8 comments · Fixed by #43
Closed

Register lwt pre-collectors and collectors #41

killian-delarue opened this issue May 11, 2022 · 8 comments · Fixed by #43

Comments

@killian-delarue
Copy link
Contributor

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
Copy link
Member

samoht commented May 12, 2022

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
Copy link
Contributor Author

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

@talex5
Copy link
Contributor

talex5 commented May 13, 2022

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
Copy link

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:

  • vendor prometheus with the Lwt collectors on our side,
  • introduce the Lwt collectors into prometheus and constraint our dependencies to this particular version, so that you can switch to effects afterwards

@samoht
Copy link
Member

samoht commented May 18, 2022

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

@talex5
Copy link
Contributor

talex5 commented May 20, 2022

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
Copy link
Contributor Author

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

@samoht
Copy link
Member

samoht commented May 24, 2022

Yes please :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants