Skip to content

Fix histogram buckets for storage latency metrics #7672

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.

Sign up for GitHub

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

Open
Samriddha9619 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix histogram buckets for storage latency metrics #7672

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
114 changes: 73 additions & 41 deletions internal/metrics/metrics.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"reflect"
"strconv"
"strings"
"time"
)

// MustInit initializes the passed in metrics and initializes its fields using the passed in factory.
Expand All @@ -36,58 +37,54 @@ func Init(m any, factory Factory, globalTags map[string]string) error {
factory = NullFactory
}

counterPtrType := reflect.TypeOf((*Counter)(nil)).Elem()
Copy link
Member

@yurishkuro yurishkuro Jan 9, 2026

Choose a reason for hiding this comment

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

Why does this function need refactoring? If there was a bug in it please create a unit test first that demonstrates the bug.

Also, overall there are no tests in this PR - if you changing something there must be a reason and that reason could be captured in a unit test

gaugePtrType := reflect.TypeOf((*Gauge)(nil)).Elem()
timerPtrType := reflect.TypeOf((*Timer)(nil)).Elem()
histogramPtrType := reflect.TypeOf((*Histogram)(nil)).Elem()

v := reflect.ValueOf(m).Elem()
t := v.Type()
for i := 0; i < t.NumField(); i++ {
tags := make(map[string]string)
maps.Copy(tags, globalTags)
var buckets []float64

field := t.Field(i)
metric := field.Tag.Get("metric")
if metric == "" {
return fmt.Errorf("Field %s is missing a tag 'metric'", field.Name)
}
if tagString := field.Tag.Get("tags"); tagString != "" {
for tagPair := range strings.SplitSeq(tagString, ",") {
tag := strings.Split(tagPair, "=")
if len(tag) != 2 {
return fmt.Errorf(
"Field [%s]: Tag [%s] is not of the form key=value in 'tags' string [%s]",
field.Name, tagPair, tagString)
tagPairs := strings.Split(tagString, ",")
for _, pair := range tagPairs {
kv := strings.Split(pair, "=")
if len(kv) == 2 {
tags[kv[0]] = kv[1]
}
tags[tag[0]] = tag[1]
}
}
if bucketString := field.Tag.Get("buckets"); bucketString != "" {
switch {
case field.Type.AssignableTo(timerPtrType):
// TODO: Parse timer duration buckets
return fmt.Errorf(
"Field [%s]: Buckets are not currently initialized for timer metrics",
field.Name)
case field.Type.AssignableTo(histogramPtrType):
bucketValues := strings.Split(bucketString, ",")
for _, bucket := range bucketValues {
b, err := strconv.ParseFloat(bucket, 64)
if err != nil {
return fmt.Errorf(
"Field [%s]: Bucket [%s] could not be converted to float64 in 'buckets' string [%s]",
field.Name, bucket, bucketString)
}
buckets = append(buckets, b)
help := field.Tag.Get("help")
Copy link
Member

@yurishkuro yurishkuro Jan 9, 2026

Choose a reason for hiding this comment

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

why are you changing this to unconditional reads where the previous code was actually checking if a particular field was defined in the annotation and was not empty?


var buckets []float64
var durationBuckets []time.Duration
var err error

bucketTag := field.Tag.Get("buckets")

switch {
case field.Type.AssignableTo(timerPtrType):
if bucketTag != "" {
durationBuckets, err = parseBuckets(bucketTag, time.ParseDuration)
if err != nil {
return fmt.Errorf("Field [%s]: %w", field.Name, err)
}
}
case field.Type.AssignableTo(histogramPtrType):
if bucketTag != "" {
buckets, err = parseBuckets(bucketTag, func(s string) (float64, error) {
return strconv.ParseFloat(s, 64)
})
if err != nil {
return fmt.Errorf("Field [%s]: %w", field.Name, err)
}
default:
return fmt.Errorf(
"Field [%s]: Buckets should only be defined for Timer and Histogram metric types",
field.Name)
}
default:
}
help := field.Tag.Get("help")

var obj any
switch {
case field.Type.AssignableTo(counterPtrType):
Expand All @@ -103,11 +100,11 @@ func Init(m any, factory Factory, globalTags map[string]string) error {
Help: help,
})
case field.Type.AssignableTo(timerPtrType):
// TODO: Add buckets once parsed (see TODO above)
obj = factory.Timer(TimerOptions{
Name: metric,
Tags: tags,
Help: help,
Name: metric,
Tags: tags,
Help: help,
Buckets: durationBuckets,
})
case field.Type.AssignableTo(histogramPtrType):
obj = factory.Histogram(HistogramOptions{
Expand All @@ -116,12 +113,47 @@ func Init(m any, factory Factory, globalTags map[string]string) error {
Help: help,
Buckets: buckets,
})
case field.Type.Kind() == reflect.Ptr && field.Type.Elem().Kind() == reflect.Struct:
nestedMetrics := reflect.New(field.Type.Elem()).Interface()
// Explicitly using NSOptions here
if err := Init(nestedMetrics, factory.Namespace(NSOptions{
Name: metric,
Tags: tags,
}), nil); err != nil {
return err
}
obj = nestedMetrics
default:
return fmt.Errorf(
"Field %s is not a pointer to timer, gauge, or counter",
"Field %s is not a pointer to timer, gauge, counter, histogram or struct",
field.Name)
}
v.Field(i).Set(reflect.ValueOf(obj))
if obj != nil {
v.Field(i).Set(reflect.ValueOf(obj))
}
}
return nil
}

func parseBuckets[T any](tag string, parse func(string) (T, error)) ([]T, error) {
if tag == "" {
return nil, nil
}
parts := strings.Split(tag, ",")
results := make([]T, 0, len(parts))
for _, part := range parts {
val, err := parse(strings.TrimSpace(part))
if err != nil {
return nil, fmt.Errorf("bucket [%s] could not be parsed: %w", part, err)
}
results = append(results, val)
}
return results, nil
}

var (
counterPtrType = reflect.TypeOf((*Counter)(nil)).Elem()
gaugePtrType = reflect.TypeOf((*Gauge)(nil)).Elem()
timerPtrType = reflect.TypeOf((*Timer)(nil)).Elem()
histogramPtrType = reflect.TypeOf((*Histogram)(nil)).Elem()
)
13 changes: 12 additions & 1 deletion internal/metrics/otelmetrics/factory.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,18 @@ func (f *otelFactory) Histogram(opts metrics.HistogramOptions) metrics.Histogram

func (f *otelFactory) Timer(opts metrics.TimerOptions) metrics.Timer {
name := f.subScope(opts.Name)
timer, err := f.meter.Float64Histogram(name, metric.WithUnit("s"))

histogramOpts := []metric.Float64HistogramOption{metric.WithUnit("s")}

if len(opts.Buckets) > 0 {
boundaries := make([]float64, len(opts.Buckets))
for i, d := range opts.Buckets {
boundaries[i] = d.Seconds()
}
histogramOpts = append(histogramOpts, metric.WithExplicitBucketBoundaries(boundaries...))
}

timer, err := f.meter.Float64Histogram(name, histogramOpts...)
if err != nil {
log.Printf("Error creating OTEL timer: %v", err)
return metrics.NullTimer
Expand Down
4 changes: 2 additions & 2 deletions internal/storage/v1/api/spanstore/spanstoremetrics/write_metrics.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@ type WriteMetrics struct {
Attempts metrics.Counter `metric:"attempts"`
Inserts metrics.Counter `metric:"inserts"`
Errors metrics.Counter `metric:"errors"`
LatencyOk metrics.Timer `metric:"latency-ok"`
LatencyErr metrics.Timer `metric:"latency-err"`
LatencyOk metrics.Timer `metric:"latency-ok" buckets:"5ms,10ms,25ms,50ms,100ms,250ms,500ms,1s,2500ms,5s,10s"`
LatencyErr metrics.Timer `metric:"latency-err" buckets:"5ms,10ms,25ms,50ms,100ms,250ms,500ms,1s,2500ms,5s,10s"`
}

// NewWriter takes a metrics scope and creates a metrics struct
Expand Down