Skip to content

Conversation

@ulrikstrid
Copy link

This is a pretty early PR that just moves the code around a bit to not depend on cohttp.
Tests seems to pass on my machine at least.

Closes #39

@ulrikstrid ulrikstrid force-pushed the ulrikstrid--pure-implementation branch from 724d7a3 to df232b1 Compare June 21, 2022 13:38
Copy link
Contributor

@talex5 talex5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable. I'm not keen on the name prometheus-app-pure though (especially as it has the side-effect of registering collectors!). Maybe prometheus-reporter?

@ulrikstrid ulrikstrid force-pushed the ulrikstrid--pure-implementation branch from df232b1 to 5acd350 Compare September 21, 2022 12:15
@ulrikstrid
Copy link
Author

Sorry for the super late update here @talex5. I have now rebased, changed the name to prometheus-reporter and changed the description in the opam file.


The `prometheus-reporter.unix` ocamlfind library provides the `Prometheus_reporter_unix` module,
which includes a cmdliner option.
See the `examples/example.ml` program for an example, which can be run as:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing text here?

@@ -0,0 +1,18 @@
(** Report metrics for Prometheus.
See: https://prometheus.io/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was See: {{:https://prometheus.io/}https://prometheus.io/} previously.

@@ -0,0 +1,55 @@
(** Report metrics for Prometheus.
See: https://prometheus.io/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should also be a link.

Comment on lines +12 to +20
- This extends [Prometheus_reporter] with support for cmdliner option parsing, a server pre-configured
for Unix, and a start-time metric that uses [Unix.gettimeofday].
*)

type config = int option

val opts : config Cmdliner.Term.t
(** [opts] is the extra command-line options to offer Prometheus
monitoring. *)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might make more sense to keep the cmdliner stuff for cohttp in app. That would also avoid exposing the type (which is likely to change - see #51).

process_cpu_seconds_total;
]
end
include Prometheus_reporter
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also be able to include it in the mli, to avoid duplication.

@smorimoto
Copy link

I would love to see a combination of Prometheus and Dream!

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 this pull request may close these issues.

Split out the non-Cohttp parts of prometheus-app

3 participants