-
Notifications
You must be signed in to change notification settings - Fork 33
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
feat: add option to disable summary collection in routeMetrics config #96
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for contribution. Looks good, but I think we can make it even better.
README.md
Outdated
@@ -205,7 +206,7 @@ await app.register(metricsPlugin, { | |||
|
|||
### HTTP routes metrics in Prometheus | |||
|
|||
The following table shows what metrics will be available in Prometheus. Note suffixes like `_bucket`, `_sum`, `_count` are added automatically. | |||
The following table shows what metrics will be available in Prometheus. Note suffixes like `_bucket`, `_sum`, `_count` are added automatically and `summary` type metrics will only be available if `{ routeMetrics: { enableSummaries: true } }` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
summary type metrics will only be available if
{ routeMetrics: { enableSummaries: true }
Maybe it's worth to implement more flexible approach:
If enabled = { histogram: true/false, summary: true/false}
each metric is enabled if it's enabled in object provided.
If enabled = true
- both metrics enabled.
If enabled = false
- both disabled.
WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to me! I've amended the PR as per your suggestion :)
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #96 +/- ##
===========================================
- Coverage 100.00% 99.39% -0.61%
===========================================
Files 2 2
Lines 157 165 +8
Branches 60 64 +4
===========================================
+ Hits 157 164 +7
- Misses 0 1 +1 ☔ View full report in Codecov by Sentry. |
## [10.4.0](v10.3.3...v10.4.0) (2023-11-28) ### 🚀 Features * add option to disable summary collection in routeMetrics config ([#96](#96)) ([c0cb240](c0cb240))
🎉 This PR is included in version 10.4.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
As you might know, quantiles can be calculated server-side from histograms using Prometheus'
histogram_quantile
function. There are numerous reasons for doing this over summaries:This library is great, and it's been very useful for my personal use-cases, but I've found that I'm using
histogram_quantile
and completely ignoring summaries, yet they are still getting calculated and there is no option to disable them.Please consider my PR and let me know if I need to make any changes :).