Add prometheus-lwt as a forward-compatible Lwt interface - #66
Conversation
|
I've pushed a commit with some changes: We'd like people to stop using the old names soon so we can remove the Lwt dependency, so I marked the old functions as deprecated now. API changes:
This should mean we end up with a nicer API after removing the old names I think. All functions in |
|
Looks good, I was in two minds about the deprecated tags since I really dislike build time spam, but these are all actionable tags with instructions on what to do for the user! |
talex5
left a comment
There was a problem hiding this comment.
Looks good to me.
I pushed some more minor changes, updating the changelog and README a bit, and removing the (wrapped false) (didn't look like it was needed).
| include METRIC | ||
| val observe : t -> float -> unit | ||
| val time : t -> (unit -> float) -> (unit -> 'a Lwt.t) -> 'a Lwt.t | ||
| val time : t -> (unit -> float) -> (unit -> 'a Lwt.t) -> 'a Lwt.t [@@deprecated] |
There was a problem hiding this comment.
This is a minor thing, but is the deprecated tag needed outside of the mli?
There was a problem hiding this comment.
It wouldn't build without it for some reason (the error is pretty confusing):
File "src/prometheus.ml", line 1:
Error (alert deprecated): time
Use observe_time instead if you don't need Lwt here, or Prometheus_lwt.HISTOGRAM.observe_time if you do.
File "src/prometheus.mli", lines 233-234, characters 2-123:
Definition
File "src/prometheus.ml", line 367, characters 2-67:
367 | val time : t -> (unit -> float) -> (unit -> 'a Lwt.t) -> 'a Lwt.t
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Expected signature
There was a problem hiding this comment.
Interesting, it doesn't fire an error when using let bindings and val signatures, only when module types are involved.
Introduce the `prometheus-lwt` package with the API that the core will keep once its Lwt dependency is removed (mirage#60 mirage#65), but implemented for now via Lwt support still present in `prometheus`. Therefore nothing breaks in this release as existing users can migrate to `Prometheus_lwt` and also to the new synchronous functions at their own pace. The actual interface break will then happen in a later release without further source changes for migrated Lwt users. Also add synchronous variants of the timing helpers to the core since the existing Lwt-typed functions occupy the unsuffixed names until the break. Suggested by @talex5 in mirage#65. Co-authored-by: Mark Elvers <mark.elvers@tunbury.org> Co-authored-by: Thomas Leonard <talex5@gmail.com>
Introduce the
prometheus-lwtpackage with the API that the core will keep once its Lwt dependency is removed (#60 #65), but implemented for now via Lwt support still present inprometheus.Therefore nothing breaks in this release as existing users can migrate to
Prometheus_lwtand also to the new synchronous_fnfunctions at their own pace. The actual interface break will then happen in a later release without further source changes for migrated Lwt users.Also add synchronous variants of the timing helpers to the core since the existing Lwt-typed functions occupy the unsuffixed names until the break.
Suggested by @talex5 in #65.