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 16 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
Comment on lines +132 to +133
The second metrics we use to instrument our program is a background thread
incrementing a counter every second.
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather "the second tool we use to instrument our program[…]"? The thread is not the metric, right?

Copy link
Author

Choose a reason for hiding this comment

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

correct, it's lazy writing

Copy link
Author

@lucasdicioccio lucasdicioccio Apr 5, 2024

Choose a reason for hiding this comment

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

Maybe We further instrument our program with a second metrics. This second metrics consist of a simple counter. A background thread will increment the counter every second.

wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

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

yep!

doc/cookbook/expose-prometheus/ExposePrometheus.lhs Outdated Show resolved Hide resolved
"pre-rendered" format for the MimeRender instance.

``` haskell
newtype Metrics = Metrics {toLBS :: ByteString}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
newtype Metrics = Metrics {toLBS :: ByteString}
newtype Metrics = Metrics {getMetrics :: ByteString}

Copy link
Author

Choose a reason for hiding this comment

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

maybe FormattedMetrics then so that getMetrics becomes getFormattedMetrics (if you want to have more explicit types)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah sure let's go!

newtype Metrics = Metrics {toLBS :: ByteString}

instance MimeRender PlainText Metrics where
mimeRender _ = toLBS
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
mimeRender _ = toLBS
mimeRender _ = getMetrics

doc/cookbook/expose-prometheus/ExposePrometheus.lhs Outdated Show resolved Hide resolved
lucasdicioccio and others added 14 commits April 5, 2024 12:20
@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
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