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.

Sign up for GitHub

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

Jump to bottom

Register lwt collectors and pre-collectors #43

Merged
merged 1 commit into from
Jun 15, 2022
Merged

Register lwt collectors and pre-collectors #43

merged 1 commit into from
Jun 15, 2022

Conversation

killian-delarue
Copy link
Contributor

This pull request adds the possibility to register lwt functions as collectors and pre-collectors.

Fixes #41.

talex5
talex5 reviewed Jun 1, 2022
View reviewed changes
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.

Thanks!

It would be good to add a couple of async collectors to the tests too, to check this is working.

Rebasing on master will probably fix the CI problems.

src/prometheus.ml Outdated Show resolved Hide resolved
src/prometheus.mli Outdated Show resolved Hide resolved
src/prometheus.ml Outdated Show resolved Hide resolved
@killian-delarue killian-delarue requested a review from talex5 June 3, 2022 09:34
talex5
talex5 reviewed Jun 6, 2022
View reviewed changes
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.

The CI tests are still failing. As I said before, you need to rebase to fix this.

I'd still be interested to know what you're planning to use this for. It would be useful for the changelog to explain why this was added.

app/prometheus_app.ml Outdated Show resolved Hide resolved
src/prometheus.ml Outdated Show resolved Hide resolved
tests/test.ml Outdated Show resolved Hide resolved
tests/test.ml Outdated Show resolved Hide resolved
talex5
talex5 reviewed Jun 10, 2022
View reviewed changes
src/prometheus.ml Outdated Show resolved Hide resolved
@killian-delarue killian-delarue requested a review from talex5 June 13, 2022 11:59
talex5
talex5 approved these changes Jun 14, 2022
View reviewed changes
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.

Thanks! I pushed a few minor changes (such as checking that a metric isn't registered as both lwt and non-lwt). If you're happy with this, I think it's ready for merging.

@talex5 talex5 merged commit 792db79 into mirage:master Jun 15, 2022
talex5 added a commit to talex5/opam-repository that referenced this pull request Jun 16, 2022
@talex5
[new release] prometheus and prometheus-app (1.2)
9ccb62e 
CHANGES:

- Add lwt collectors and pre-collectors (@killian-delarue, mirage/prometheus#43).
  Note that this is a temporary feature while we wait for OCaml 5 to be released,
  when this can be replaced by the use of effects.

- Fix deprecations in Fmt 0.8.10 (@MisterDA, mirage/prometheus#36).

- General build updates, upstream deprecations, etc (@talex5, mirage/prometheus#33 mirage/prometheus#34 mirage/prometheus#35 mirage/prometheus#40 mirage/prometheus#42).
@talex5 talex5 mentioned this pull request Jun 16, 2022
Merged
talex5 added a commit to talex5/opam-repository that referenced this pull request Jun 16, 2022
@talex5
[new release] prometheus and prometheus-app (1.2)
c753eac 
CHANGES:

- Add lwt collectors and pre-collectors (@killian-delarue, mirage/prometheus#43).
  Note that this is a temporary feature while we wait for OCaml 5 to be released,
  when this can be replaced by the use of effects.

- Fix deprecations in Fmt 0.8.10 (@MisterDA, mirage/prometheus#36).

- General build updates, upstream deprecations, etc (@talex5, mirage/prometheus#33 mirage/prometheus#34 mirage/prometheus#35 mirage/prometheus#40 mirage/prometheus#42).
talex5 added a commit to talex5/opam-repository that referenced this pull request Jun 16, 2022
@talex5
[new release] prometheus and prometheus-app (1.2)
3159503 
CHANGES:

- Add lwt collectors and pre-collectors (@killian-delarue, mirage/prometheus#43).
  Note that this is a temporary feature while we wait for OCaml 5 to be released,
  when this can be replaced by the use of effects.

- Fix deprecations in Fmt 0.8.10 (@MisterDA, mirage/prometheus#36).

- General build updates, upstream deprecations, etc (@talex5, mirage/prometheus#33 mirage/prometheus#34 mirage/prometheus#35 mirage/prometheus#40 mirage/prometheus#42).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@talex5 talex5 talex5 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Register lwt pre-collectors and collectors
2 participants
@killian-delarue @talex5