-
Notifications
You must be signed in to change notification settings - Fork 8
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(metrics) histograms with user-defined bins #608
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #608 +/- ##
===================================================
+ Coverage 90.73673% 90.83281% +0.09608%
===================================================
Files 52 52
Lines 11076 11203 +127
===================================================
+ Hits 10050 10176 +126
- Misses 1026 1027 +1
... and 27 files with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more. |
5d7227c
to
bbfdd98
Compare
afae383
to
9fc9b7f
Compare
81d4024
to
4cdb29e
Compare
4cdb29e
to
2f0d443
Compare
This feature is only available through the FFI. If a user provides a list of numbers when defining a histogram, those numbers will be used as the upper-bounds of the histogram's bins. For instance, the following code: ``` local shm = require "resty.wasmx.shm" shm.metrics:define("h", shm.metrics.HISTOGRAM, { bins = { 1, 3, 5 } }) ``` Creates a histogram with bins `[0, 1]`, `(1, 3]`, `(3, 5]` and `(5, Inf+]`
2f0d443
to
353bc71
Compare
ngx_wa_metrics_histogram_t **h; | ||
|
||
if (bins) { | ||
if (cn_bins > NGX_WA_METRICS_BINS_MAX) { |
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.
One last thing before merge: a logarithmic histogram can contain up to 18 bins as per the existing design, but this specific condition and the extra bin on line 197 mean that a custom binning histogram can contain up to 19 bins (18 defined + the MAX_UINT32 bin). Is this analysis correct? If so, is the behavior correct or could the number of bins difference cause problems?
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.
Yeah, that was a deliberate decision.
Although it is an inconsistent application of the limit of bins, this seemed like an implementation detail that the FFI layer could be spared from; since the limit of bins is rather arbitrary.
In retrospective, though, I admit that having a check for #bins > HISTOGRAM_MAX_BINS - 1
accompanied by a comment in shm.lua
seems like a low price to pay to achieve consistence between logarithmic and standard histograms.
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.
I made this change in the merge PR. The change makes the documentation easier to read and understand: "all histograms can only have 18 bins"
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 addressing it in the merge PR.
This feature is only available through the FFI.
If a user provides a list of numbers when defining a histogram, those numbers will be used as the upper-bounds of the histogram's bins.
For instance, the following code:
Creates a histogram with bins
[0, 1]
,(1, 3]
,(3, 5]
and(5, Inf+]
.