From 45e03f163c8361a3d36bacbf3c36272d68d2cedf Mon Sep 17 00:00:00 2001 From: David Ashpole Date: Wed, 19 Jul 2023 18:03:22 +0000 Subject: [PATCH] prototype for the advice api --- metric/instrument.go | 24 +++++++++++++++++++ metric/syncfloat64.go | 10 ++++++-- metric/syncint64.go | 10 ++++++-- sdk/metric/instrument.go | 9 ++++++++ sdk/metric/meter.go | 50 +++++++++++++++++++++------------------- sdk/metric/meter_test.go | 26 +++++++++++++++++++++ sdk/metric/pipeline.go | 11 ++++++--- 7 files changed, 109 insertions(+), 31 deletions(-) diff --git a/metric/instrument.go b/metric/instrument.go index 0033c1e12d52..32ce0e7b2ccd 100644 --- a/metric/instrument.go +++ b/metric/instrument.go @@ -39,6 +39,13 @@ type InstrumentOption interface { Float64ObservableGaugeOption } +// HistogramOption applies options to histogram instruments. +// This assumes we only want to allow setting explicit bucket boundaries for histograms instruments. +type HistogramOption interface { + Int64HistogramOption + Float64HistogramOption +} + type descOpt string func (o descOpt) applyFloat64Counter(c Float64CounterConfig) Float64CounterConfig { @@ -169,6 +176,23 @@ func (o unitOpt) applyInt64ObservableGauge(c Int64ObservableGaugeConfig) Int64Ob // WithUnit sets the instrument unit. func WithUnit(u string) InstrumentOption { return unitOpt(u) } +// WithExplicitBucketBoundaries sets the instrument explicit bucket boundaries. +// Note: We could call this something closer to the spec (e.g. WithAdvice(advice)), but I used +// this to keep the prototype simpler. +func WithExplicitBucketBoundaries(bounds []float64) HistogramOption { return bucketOpt(bounds) } + +type bucketOpt []float64 + +func (o bucketOpt) applyFloat64Histogram(c Float64HistogramConfig) Float64HistogramConfig { + c.explicitBucketBoundaries = o + return c +} + +func (o bucketOpt) applyInt64Histogram(c Int64HistogramConfig) Int64HistogramConfig { + c.explicitBucketBoundaries = o + return c +} + // AddOption applies options to an addition measurement. See // [MeasurementOption] for other options that can be used as an AddOption. type AddOption interface { diff --git a/metric/syncfloat64.go b/metric/syncfloat64.go index f0b063721d81..0a4825ae6a79 100644 --- a/metric/syncfloat64.go +++ b/metric/syncfloat64.go @@ -147,8 +147,9 @@ type Float64Histogram interface { // Float64HistogramConfig contains options for synchronous counter instruments // that record int64 values. type Float64HistogramConfig struct { - description string - unit string + description string + unit string + explicitBucketBoundaries []float64 } // NewFloat64HistogramConfig returns a new [Float64HistogramConfig] with all @@ -171,6 +172,11 @@ func (c Float64HistogramConfig) Unit() string { return c.unit } +// ExplicitBucketBoundaries returns the configured explicit bucket boundaries. +func (c Float64HistogramConfig) ExplicitBucketBoundaries() []float64 { + return c.explicitBucketBoundaries +} + // Float64HistogramOption applies options to a [Float64HistogramConfig]. See // [InstrumentOption] for other options that can be used as a // Float64HistogramOption. diff --git a/metric/syncint64.go b/metric/syncint64.go index 6f508eb66d40..56667d32fc01 100644 --- a/metric/syncint64.go +++ b/metric/syncint64.go @@ -147,8 +147,9 @@ type Int64Histogram interface { // Int64HistogramConfig contains options for synchronous counter instruments // that record int64 values. type Int64HistogramConfig struct { - description string - unit string + description string + unit string + explicitBucketBoundaries []float64 } // NewInt64HistogramConfig returns a new [Int64HistogramConfig] with all opts @@ -171,6 +172,11 @@ func (c Int64HistogramConfig) Unit() string { return c.unit } +// ExplicitBucketBoundaries returns the configured explicit bucket boundaries. +func (c Int64HistogramConfig) ExplicitBucketBoundaries() []float64 { + return c.explicitBucketBoundaries +} + // Int64HistogramOption applies options to a [Int64HistogramConfig]. See // [InstrumentOption] for other options that can be used as an // Int64HistogramOption. diff --git a/sdk/metric/instrument.go b/sdk/metric/instrument.go index b4d6fa8b35cf..cc3c12dcb430 100644 --- a/sdk/metric/instrument.go +++ b/sdk/metric/instrument.go @@ -79,6 +79,11 @@ type Instrument struct { // Scope identifies the instrumentation that created the instrument. Scope instrumentation.Scope + // bucketBoundaries are the configured boundaries for histogram instruments. + // Make this non-exported because I don't think we want users setting this + // in a view. + bucketBoundaries []float64 + // Ensure forward compatibility if non-comparable fields need to be added. nonComparable // nolint: unused } @@ -90,6 +95,8 @@ func (i Instrument) empty() bool { i.Kind == zeroInstrumentKind && i.Unit == "" && i.Scope == zeroScope + // This is only used for views, so no need to take bucket boundaries + // into account here. } // matches returns whether all the non-zero-value fields of i match the @@ -101,6 +108,8 @@ func (i Instrument) matches(other Instrument) bool { i.matchesKind(other) && i.matchesUnit(other) && i.matchesScope(other) + // Note: Don't require matching the bucket boundaries, as that isn't something + // that users can set with a view. } // matchesName returns true if the Name of i is "" or it equals the Name of diff --git a/sdk/metric/meter.go b/sdk/metric/meter.go index 7e1d32be2493..808c7085415b 100644 --- a/sdk/metric/meter.go +++ b/sdk/metric/meter.go @@ -68,7 +68,7 @@ var _ metric.Meter = (*meter)(nil) func (m *meter) Int64Counter(name string, options ...metric.Int64CounterOption) (metric.Int64Counter, error) { cfg := metric.NewInt64CounterConfig(options...) const kind = InstrumentKindCounter - i, err := m.int64IP.lookup(kind, name, cfg.Description(), cfg.Unit()) + i, err := m.int64IP.lookup(kind, name, cfg.Description(), cfg.Unit(), nil) if err != nil { return i, err } @@ -82,7 +82,7 @@ func (m *meter) Int64Counter(name string, options ...metric.Int64CounterOption) func (m *meter) Int64UpDownCounter(name string, options ...metric.Int64UpDownCounterOption) (metric.Int64UpDownCounter, error) { cfg := metric.NewInt64UpDownCounterConfig(options...) const kind = InstrumentKindUpDownCounter - i, err := m.int64IP.lookup(kind, name, cfg.Description(), cfg.Unit()) + i, err := m.int64IP.lookup(kind, name, cfg.Description(), cfg.Unit(), nil) if err != nil { return i, err } @@ -96,7 +96,7 @@ func (m *meter) Int64UpDownCounter(name string, options ...metric.Int64UpDownCou func (m *meter) Int64Histogram(name string, options ...metric.Int64HistogramOption) (metric.Int64Histogram, error) { cfg := metric.NewInt64HistogramConfig(options...) const kind = InstrumentKindHistogram - i, err := m.int64IP.lookup(kind, name, cfg.Description(), cfg.Unit()) + i, err := m.int64IP.lookup(kind, name, cfg.Description(), cfg.Unit(), cfg.ExplicitBucketBoundaries()) if err != nil { return i, err } @@ -158,7 +158,7 @@ func (m *meter) Int64ObservableGauge(name string, options ...metric.Int64Observa func (m *meter) Float64Counter(name string, options ...metric.Float64CounterOption) (metric.Float64Counter, error) { cfg := metric.NewFloat64CounterConfig(options...) const kind = InstrumentKindCounter - i, err := m.float64IP.lookup(kind, name, cfg.Description(), cfg.Unit()) + i, err := m.float64IP.lookup(kind, name, cfg.Description(), cfg.Unit(), nil) if err != nil { return i, err } @@ -172,7 +172,7 @@ func (m *meter) Float64Counter(name string, options ...metric.Float64CounterOpti func (m *meter) Float64UpDownCounter(name string, options ...metric.Float64UpDownCounterOption) (metric.Float64UpDownCounter, error) { cfg := metric.NewFloat64UpDownCounterConfig(options...) const kind = InstrumentKindUpDownCounter - i, err := m.float64IP.lookup(kind, name, cfg.Description(), cfg.Unit()) + i, err := m.float64IP.lookup(kind, name, cfg.Description(), cfg.Unit(), nil) if err != nil { return i, err } @@ -186,7 +186,7 @@ func (m *meter) Float64UpDownCounter(name string, options ...metric.Float64UpDow func (m *meter) Float64Histogram(name string, options ...metric.Float64HistogramOption) (metric.Float64Histogram, error) { cfg := metric.NewFloat64HistogramConfig(options...) const kind = InstrumentKindHistogram - i, err := m.float64IP.lookup(kind, name, cfg.Description(), cfg.Unit()) + i, err := m.float64IP.lookup(kind, name, cfg.Description(), cfg.Unit(), cfg.ExplicitBucketBoundaries()) if err != nil { return i, err } @@ -451,20 +451,21 @@ func newInt64InstProvider(s instrumentation.Scope, p pipelines, c *cache[string, return &int64InstProvider{scope: s, pipes: p, resolve: newResolver[int64](p, c)} } -func (p *int64InstProvider) aggs(kind InstrumentKind, name, desc, u string) ([]aggregate.Measure[int64], error) { +func (p *int64InstProvider) aggs(kind InstrumentKind, name, desc, u string, bounds []float64) ([]aggregate.Measure[int64], error) { inst := Instrument{ - Name: name, - Description: desc, - Unit: u, - Kind: kind, - Scope: p.scope, + Name: name, + Description: desc, + Unit: u, + Kind: kind, + Scope: p.scope, + bucketBoundaries: bounds, } return p.resolve.Aggregators(inst) } // lookup returns the resolved instrumentImpl. -func (p *int64InstProvider) lookup(kind InstrumentKind, name, desc, u string) (*int64Inst, error) { - aggs, err := p.aggs(kind, name, desc, u) +func (p *int64InstProvider) lookup(kind InstrumentKind, name, desc, u string, bounds []float64) (*int64Inst, error) { + aggs, err := p.aggs(kind, name, desc, u, bounds) return &int64Inst{measures: aggs}, err } @@ -479,27 +480,28 @@ func newFloat64InstProvider(s instrumentation.Scope, p pipelines, c *cache[strin return &float64InstProvider{scope: s, pipes: p, resolve: newResolver[float64](p, c)} } -func (p *float64InstProvider) aggs(kind InstrumentKind, name, desc, u string) ([]aggregate.Measure[float64], error) { +func (p *float64InstProvider) aggs(kind InstrumentKind, name, desc, u string, bounds []float64) ([]aggregate.Measure[float64], error) { inst := Instrument{ - Name: name, - Description: desc, - Unit: u, - Kind: kind, - Scope: p.scope, + Name: name, + Description: desc, + Unit: u, + Kind: kind, + Scope: p.scope, + bucketBoundaries: bounds, } return p.resolve.Aggregators(inst) } // lookup returns the resolved instrumentImpl. -func (p *float64InstProvider) lookup(kind InstrumentKind, name, desc, u string) (*float64Inst, error) { - aggs, err := p.aggs(kind, name, desc, u) +func (p *float64InstProvider) lookup(kind InstrumentKind, name, desc, u string, bounds []float64) (*float64Inst, error) { + aggs, err := p.aggs(kind, name, desc, u, bounds) return &float64Inst{measures: aggs}, err } type int64ObservProvider struct{ *int64InstProvider } func (p int64ObservProvider) lookup(kind InstrumentKind, name, desc, u string) (int64Observable, error) { - aggs, err := p.aggs(kind, name, desc, u) + aggs, err := p.aggs(kind, name, desc, u, nil) return newInt64Observable(p.scope, kind, name, desc, u, aggs), err } @@ -532,7 +534,7 @@ func (o int64Observer) Observe(val int64, opts ...metric.ObserveOption) { type float64ObservProvider struct{ *float64InstProvider } func (p float64ObservProvider) lookup(kind InstrumentKind, name, desc, u string) (float64Observable, error) { - aggs, err := p.aggs(kind, name, desc, u) + aggs, err := p.aggs(kind, name, desc, u, nil) return newFloat64Observable(p.scope, kind, name, desc, u, aggs), err } diff --git a/sdk/metric/meter_test.go b/sdk/metric/meter_test.go index 185095e4f8d5..84090a3c826a 100644 --- a/sdk/metric/meter_test.go +++ b/sdk/metric/meter_test.go @@ -397,6 +397,32 @@ func TestMeterCreatesInstruments(t *testing.T) { }, }, }, + { + name: "SyncInt64Histogram with bounds through advice", + fn: func(t *testing.T, m metric.Meter) { + gauge, err := m.Int64Histogram("histogram", metric.WithExplicitBucketBoundaries([]float64{0, 1, 2, 3})) + assert.NoError(t, err) + + gauge.Record(context.Background(), 7) + }, + want: metricdata.Metrics{ + Name: "histogram", + Data: metricdata.Histogram[int64]{ + Temporality: metricdata.CumulativeTemporality, + DataPoints: []metricdata.HistogramDataPoint[int64]{ + { + Attributes: attribute.Set{}, + Count: 1, + Bounds: []float64{0, 1, 2, 3}, + BucketCounts: []uint64{0, 0, 0, 0, 1}, + Min: metricdata.NewExtrema[int64](7), + Max: metricdata.NewExtrema[int64](7), + Sum: 7, + }, + }, + }, + }, + }, { name: "SyncFloat64Count", fn: func(t *testing.T, m metric.Meter) { diff --git a/sdk/metric/pipeline.go b/sdk/metric/pipeline.go index 05fe29ce4f93..53cbc85d0255 100644 --- a/sdk/metric/pipeline.go +++ b/sdk/metric/pipeline.go @@ -247,7 +247,7 @@ func (i *inserter[N]) Instrument(inst Instrument) ([]aggregate.Measure[N], error } matched = true - in, id, err := i.cachedAggregator(inst.Scope, inst.Kind, stream) + in, id, err := i.cachedAggregator(inst.Scope, inst.Kind, inst.bucketBoundaries, stream) if err != nil { errs.append(err) } @@ -272,7 +272,7 @@ func (i *inserter[N]) Instrument(inst Instrument) ([]aggregate.Measure[N], error Description: inst.Description, Unit: inst.Unit, } - in, _, err := i.cachedAggregator(inst.Scope, inst.Kind, stream) + in, _, err := i.cachedAggregator(inst.Scope, inst.Kind, inst.bucketBoundaries, stream) if err != nil { errs.append(err) } @@ -306,11 +306,16 @@ type aggVal[N int64 | float64] struct { // // If the instrument defines an unknown or incompatible aggregation, an error // is returned. -func (i *inserter[N]) cachedAggregator(scope instrumentation.Scope, kind InstrumentKind, stream Stream) (meas aggregate.Measure[N], aggID uint64, err error) { +func (i *inserter[N]) cachedAggregator(scope instrumentation.Scope, kind InstrumentKind, bucketBoundaries []float64, stream Stream) (meas aggregate.Measure[N], aggID uint64, err error) { switch stream.Aggregation.(type) { case nil, aggregation.Default: // Undefined, nil, means to use the default from the reader. stream.Aggregation = i.pipeline.reader.aggregation(kind) + // override bucket boundaries with the instrument-level default boundaries + if a, ok := stream.Aggregation.(aggregation.ExplicitBucketHistogram); ok && len(bucketBoundaries) > 0 { + a.Boundaries = bucketBoundaries + stream.Aggregation = a + } } if err := isAggregatorCompatible(kind, stream.Aggregation); err != nil {