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

Add cookbook to expose Prometheus counters. #1730

Open
wants to merge 18 commits into
base: master
Choose a base branch
from

Conversation

lucasdicioccio
Copy link

@tchoutri 😗

Not sure when I'll have time to address comments, so feel free to directly edit the PR.

The code itself is inspired from what I do in prodapi.

@tchoutri tchoutri self-assigned this Mar 17, 2024
@tchoutri tchoutri self-requested a review March 17, 2024 23:22
Copy link
Author

@lucasdicioccio lucasdicioccio left a comment

Choose a reason for hiding this comment

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

i blame myself

doc/cookbook/expose-prometheus/ExposePrometheus.lhs Outdated Show resolved Hide resolved
doc/cookbook/expose-prometheus/ExposePrometheus.lhs Outdated Show resolved Hide resolved
doc/cookbook/expose-prometheus/ExposePrometheus.lhs Outdated Show resolved Hide resolved
doc/cookbook/expose-prometheus/ExposePrometheus.lhs Outdated Show resolved Hide resolved
doc/cookbook/expose-prometheus/ExposePrometheus.lhs Outdated Show resolved Hide resolved
@lucasdicioccio
Copy link
Author

@tchoutri any comment?

@tchoutri
Copy link
Contributor

tchoutri commented Apr 5, 2024

Sorry for the time, I got sidetracked by servant-quickcheck, which needed some love.

Any reason why you didn't use https://hackage.haskell.org/package/wai-middleware-prometheus for the WAI integration? I'm asking since it's part of the same family of packages as wai-middleware-prometheus.

Copy link
Contributor

@tchoutri tchoutri left a comment

Choose a reason for hiding this comment

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

Okay, quite a big review, I can understand if you'd rather let me handle it. :)

doc/cookbook/expose-prometheus/ExposePrometheus.lhs Outdated Show resolved Hide resolved
doc/cookbook/expose-prometheus/ExposePrometheus.lhs Outdated Show resolved Hide resolved
doc/cookbook/expose-prometheus/ExposePrometheus.lhs Outdated Show resolved Hide resolved
doc/cookbook/expose-prometheus/ExposePrometheus.lhs Outdated Show resolved Hide resolved
doc/cookbook/expose-prometheus/ExposePrometheus.lhs Outdated Show resolved Hide resolved
doc/cookbook/expose-prometheus/ExposePrometheus.lhs Outdated Show resolved Hide resolved
doc/cookbook/expose-prometheus/ExposePrometheus.lhs Outdated Show resolved Hide resolved
doc/cookbook/expose-prometheus/ExposePrometheus.lhs Outdated Show resolved Hide resolved
doc/cookbook/expose-prometheus/ExposePrometheus.lhs Outdated Show resolved Hide resolved
doc/cookbook/expose-prometheus/ExposePrometheus.lhs Outdated Show resolved Hide resolved
@lucasdicioccio
Copy link
Author

Any reason why you didn't use https://hackage.haskell.org/package/wai-middleware-prometheus for the WAI integration? I'm asking since it's part of the same family of packages as wai-middleware-prometheus.

Two things.

  • (a) the cost of "writing your own" is low enough (as demonstrated in this example) vs. bundling "an extra dependency"
  • (b) sometimes you would really benefit from parametrizing the metrics sets via query-params (e.g., to reduce network load), which is not feasible with the above package).

I haven't discussed (a), which is more of a taste/personal-experience judgment but we could add a mention as well (and maybe link to another cookbook to embed a middleware/serve a Raw Application).
For (b) it's a bit motivated in the writing, adding a filtering query-param in prodapi is low on my priority list but it's quite often that I wish I had a way to reduce the prometheus payload at the server-side.

@tchoutri
Copy link
Contributor

tchoutri commented Apr 5, 2024

@lucasdicioccio cool, thanks for the explanation. :)

tchoutri added a commit that referenced this pull request May 5, 2024
@tchoutri
Copy link
Contributor

@lucasdicioccio Hi! Could you please rebase your PR on current master? This will trigger a build of the cookbook. :)

@lucasdicioccio
Copy link
Author

@tchoutri do you mind if I squash the diffs?

lucasdicioccio and others added 16 commits February 4, 2025 17:01
@lucasdicioccio lucasdicioccio force-pushed the cookbook-expose-prometheus branch from 6c17dea to 9118dc3 Compare February 4, 2025 16:02
lucasdicioccio and others added 2 commits February 4, 2025 17:05
@lucasdicioccio
Copy link
Author

@lucasdicioccio Hi! Could you please rebase your PR on current master? This will trigger a build of the cookbook. :)

done; I've yet to retry the whole example, sorry for the delays

@tchoutri
Copy link
Contributor

tchoutri commented Feb 5, 2025

@lucasdicioccio thanks! I and others will take a look. :)

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.

2 participants