Move the Lwt logic out of the prometheus core - #65
Conversation
|
Since we're breaking interfaces here, I intend to look at #46 after as well to get the prometheus-reporter changes in. Once that's done, it should be straightforward to add eio, miou, or any other concurrency library as additions. This'll need testing in a unikernel to be sure it works; I've only done that by inspection of dependencies. |
talex5
left a comment
talex5
left a comment
There was a problem hiding this comment.
One problem here is that you need to upgrade all libraries using prometheus at once, which could be a problem.
An alternative would be to add the separate lwt package first with the new API, but have it simply call the corresponding functions in the main prometheus package (which would still be using lwt).
Then libraries can be updated incrementally to the new API without breaking compatibility. Then we can break the original API in another release.
One annoyance is that we'd want to provide the new synchronous functions at the same time (so people upgrading can decide whether they need the Lwt version or not), but the old recording functions don't have _lwt in their names. But maybe we just need to be creative about names (e.g. replace time with time_fn or something).
On the other hand, maybe not that many people are using the library and it doesn't matter?
| module Cohttp (S : Cohttp_lwt.S.Server) : sig | ||
| val callback : | ||
| S.conn -> | ||
| Cohttp.Request.t -> | ||
| Cohttp_lwt.Body.t -> (Cohttp.Response.t * Cohttp_lwt.Body.t) Lwt.t | ||
| end | ||
| (** A Cohttp callback that serves {!CollectorRegistry.default} at [/metrics]. *) |
There was a problem hiding this comment.
Having the server here is bad - anything that wants to report Lwt metrics will pull in a cohttp dependency, even if the metrics aren't being used.
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 `_fn` 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>
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 `_fn` 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>
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 `_fn` 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>
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>
2 participants
Split off the Lwt-specific parts of the library so that defining and recording metrics no longer pulls in a concurrency library. Metric recording is synchronous anyway (it updates an in-memory scoreboard) so the core does not need Lwt.
This is the first half of #60, so that the interface-breaking move can be reviewed separately.