-
Notifications
You must be signed in to change notification settings - Fork 4
Allow custom bucket boundaries per Histogram #300
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
base: main
Are you sure you want to change the base?
Changes from 8 commits
cc5a4f3
34f4e80
7d8817e
58028a9
7bc6f05
2b76533
fcd2d89
c501546
75eeff2
dfd64bc
e8a41e4
10edad3
f0da24d
7358d84
2cfc1fd
e1250e8
4385692
b3a0e80
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -3,6 +3,7 @@ package ctats | |||||
| import ( | ||||||
| "context" | ||||||
| "log" | ||||||
| "math" | ||||||
|
|
||||||
| "github.com/pkg/errors" | ||||||
| "go.opentelemetry.io/otel/metric" | ||||||
|
|
@@ -11,12 +12,57 @@ import ( | |||||
| "github.com/alcionai/clues/internal/node" | ||||||
| ) | ||||||
|
|
||||||
| // PresetLatencyBoundariesMs are logarithmically-spaced bucket boundaries from | ||||||
| // 1 to 60_000, suitable for measuring operation latency in milliseconds up to 60s. | ||||||
| var PresetLatencyBoundariesMs = ExponentialBoundaries(1, 60_000, 15) | ||||||
|
|
||||||
| // ExponentialBoundaries returns count boundaries spaced logarithmically between | ||||||
| // min and max (both inclusive), mirroring Prometheus's ExponentialBucketsRange: | ||||||
| // https://pkg.go.dev/github.com/prometheus/client_golang/prometheus#ExponentialBucketsRange | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We're not working with prometheus here. Are there any otel docs for the same info?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Linked to the otel doc about the Explicit Bucket Histogram Aggregation. That docs also refers to being inspired by prometheus which is the source for why we are using logarithmic spacing in the first place. |
||||||
| // | ||||||
| // Example: | ||||||
| // | ||||||
| // ExponentialBoundaries(1, 60_000, 15) | ||||||
| // // → [1 2 5 11 23 51 112 245 537 1179 2588 5679 12461 27344 60000] | ||||||
| func ExponentialBoundaries(min, max float64, count int) []float64 { | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Two thoughts on this func, now that I've slept on it: first, let's verbify the name. Second, while I don't mind the default scaling, I think it would be appropriate to allow the caller to define their own scaling factor for further control. Any value less <= 1 should use the current default.
Suggested change
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added scaling factor with the effect of skewing the boundaries towards the low end of the range. Is this what you had in mind? Are you comfortable with this function or do we want to go deeper into the maths of what is optimal? I am satisfied with roughly following the logarithmic distribution of the otel default "inspired by prometheus".
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will be good to test this against app log based calculations which are exact.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we need to be too scientific. After all, this is just a quick way for someone to get an "approximately useful" set of buckets. They can always define their own if it needs to be exact according to some range. |
||||||
| if count < 2 { | ||||||
| return []float64{min, max} | ||||||
| } | ||||||
|
|
||||||
| factor := math.Pow(max/min, 1/float64(count-1)) | ||||||
| b := make([]float64, count) | ||||||
|
|
||||||
| for i := range b { | ||||||
| b[i] = math.Round(min * math.Pow(factor, float64(i))) | ||||||
| } | ||||||
|
|
||||||
| b[count-1] = max // guarantee exact ceiling, no rounding drift | ||||||
|
|
||||||
| return b | ||||||
| } | ||||||
|
|
||||||
| type histogramCfg struct { | ||||||
| boundaries []float64 | ||||||
| } | ||||||
|
|
||||||
| type HistogramOption func(*histogramCfg) | ||||||
|
|
||||||
| // WithBoundaries sets explicit bucket boundaries on the histogram. | ||||||
| // Boundaries are passed to the OTel SDK at instrument creation time and are | ||||||
| // ignored if a matching MeterProvider View is already configured. | ||||||
| func WithBoundaries(boundaries ...float64) HistogramOption { | ||||||
| return func(c *histogramCfg) { | ||||||
| c.boundaries = boundaries | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| // getOrCreateHistogram attempts to retrieve a histogram from the | ||||||
| // context with the given ID. If it is unable to find a histogram | ||||||
| // with that ID, a new histogram is generated. | ||||||
| func getOrCreateHistogram( | ||||||
| ctx context.Context, | ||||||
| id string, | ||||||
| boundaries []float64, | ||||||
|
robertschonfeld marked this conversation as resolved.
Outdated
|
||||||
| ) (recorder, error) { | ||||||
| id = formatID(id) | ||||||
| b := fromCtx(ctx) | ||||||
|
|
@@ -36,7 +82,13 @@ func getOrCreateHistogram( | |||||
| return nil, cluerr.Stack(errNoNodeInCtx) | ||||||
| } | ||||||
|
|
||||||
| hist, err := nc.OTELMeter().Float64Histogram(id) | ||||||
| var opts []metric.Float64HistogramOption | ||||||
| if len(boundaries) > 0 { | ||||||
| opts = append(opts, metric.WithExplicitBucketBoundaries(boundaries...)) | ||||||
| } | ||||||
|
ryanfkeepers marked this conversation as resolved.
Outdated
robertschonfeld marked this conversation as resolved.
Outdated
|
||||||
|
|
||||||
| // register the histogram | ||||||
| hist, err := nc.OTELMeter().Float64Histogram(id, opts...) | ||||||
| if err != nil { | ||||||
| return nil, errors.Wrap(err, "making new histogram") | ||||||
| } | ||||||
|
|
@@ -50,17 +102,19 @@ func getOrCreateHistogram( | |||||
|
|
||||||
| // RegisterHistogram introduces a new histogram with the given unit and description. | ||||||
| // If RegisterHistogram is not called before updating a metric value, a histogram with | ||||||
| // no unit or description is created. If RegisterHistogram is called for an ID that | ||||||
| // no unit or description is created. If RegisterHistogram is called for an ID that | ||||||
| // has already been registered, it no-ops. | ||||||
| func RegisterHistogram( | ||||||
| ctx context.Context, | ||||||
| // all lowercase, period delimited id of the histogram. Ex: "http.response.status_code" | ||||||
| // all lowercase, period delimited id of the histogram. Ex: "http.response.size" | ||||||
| id string, | ||||||
| // (optional) the unit of measurement. Ex: "byte", "kB", "fnords" | ||||||
| unit string, | ||||||
| // (optional) a short description about the metric. | ||||||
| // Ex: "number of times we saw the fnords". | ||||||
| description string, | ||||||
| // (optional) histogram specific options | ||||||
| opts ...HistogramOption, | ||||||
| ) (context.Context, error) { | ||||||
| id = formatID(id) | ||||||
|
|
||||||
|
|
@@ -82,18 +136,26 @@ func RegisterHistogram( | |||||
| return ctx, errors.New("no clues in ctx") | ||||||
| } | ||||||
|
|
||||||
| opts := []metric.Float64HistogramOption{} | ||||||
| var cfg histogramCfg | ||||||
| for _, o := range opts { | ||||||
| o(&cfg) | ||||||
| } | ||||||
|
|
||||||
| var metricHistogramOpts []metric.Float64HistogramOption | ||||||
|
|
||||||
| if len(description) > 0 { | ||||||
| opts = append(opts, metric.WithDescription(description)) | ||||||
| metricHistogramOpts = append(metricHistogramOpts, metric.WithDescription(description)) | ||||||
| } | ||||||
|
|
||||||
| if len(unit) > 0 { | ||||||
| opts = append(opts, metric.WithUnit(unit)) | ||||||
| metricHistogramOpts = append(metricHistogramOpts, metric.WithUnit(unit)) | ||||||
| } | ||||||
|
|
||||||
| // register the histogram | ||||||
| hist, err := nc.OTELMeter().Float64Histogram(id, opts...) | ||||||
| if len(cfg.boundaries) > 0 { | ||||||
| metricHistogramOpts = append(metricHistogramOpts, metric.WithExplicitBucketBoundaries(cfg.boundaries...)) | ||||||
| } | ||||||
|
|
||||||
| hist, err := nc.OTELMeter().Float64Histogram(id, metricHistogramOpts...) | ||||||
| if err != nil { | ||||||
| return ctx, errors.Wrap(err, "creating histogram") | ||||||
| } | ||||||
|
|
@@ -107,17 +169,23 @@ func RegisterHistogram( | |||||
| // If a Histogram instance has been registered for that ID, the | ||||||
| // registered instance will be used. If not, a new instance | ||||||
| // will get generated. | ||||||
| func Histogram[N number](id string) histogram[N] { | ||||||
| return histogram[N]{base: base{id: formatID(id)}} | ||||||
| func Histogram[N number](id string, opts ...HistogramOption) histogram[N] { | ||||||
| hgm := histogram[N]{base: base{id: formatID(id)}} | ||||||
| for _, o := range opts { | ||||||
| o(&hgm.histogramCfg) | ||||||
| } | ||||||
|
|
||||||
| return hgm | ||||||
| } | ||||||
|
|
||||||
| // histogram provides access to the factory functions. | ||||||
| type histogram[N number] struct { | ||||||
| base | ||||||
| histogramCfg | ||||||
| } | ||||||
|
|
||||||
| func (c histogram[N]) With(kvs ...any) histogram[N] { | ||||||
| return histogram[N]{base: c.with(kvs...)} | ||||||
| return histogram[N]{base: c.with(kvs...), histogramCfg: c.histogramCfg} | ||||||
| } | ||||||
|
|
||||||
| type recorder interface { | ||||||
|
|
@@ -128,9 +196,9 @@ type noopRecorder struct{} | |||||
|
|
||||||
| func (n noopRecorder) Record(context.Context, float64, ...metric.RecordOption) {} | ||||||
|
|
||||||
| // Add increments the histogram by n. n can be negative. | ||||||
| // Record records the measurement of n in the histogram. | ||||||
|
robertschonfeld marked this conversation as resolved.
|
||||||
| func (c histogram[number]) Record(ctx context.Context, n number) { | ||||||
| hist, err := getOrCreateHistogram(ctx, c.getID()) | ||||||
| hist, err := getOrCreateHistogram(ctx, c.getID(), c.boundaries) | ||||||
| if err != nil { | ||||||
| log.Printf("err getting histogram: %+v\n", err) | ||||||
| return | ||||||
|
|
||||||
Uh oh!
There was an error while loading. Please reload this page.