Skip to content
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

Add WithExplicitBucketBoundaries Histogram option to the metric api #4603

Merged
merged 9 commits into from
Oct 26, 2023

Conversation

dashpole
Copy link
Contributor

@dashpole dashpole commented Oct 10, 2023

Part of #4094

I split the API changes into its own PR so we can discuss them independently. There are a few choices we have:

One option or two (for int vs float)

Single Option (proposed):

  func WithExplicitBucketBoundaries(bounds []float64) HistogramOption
  type HistogramOption interface {
  	Int64HistogramOption
  	Float64HistogramOption
  }

VS

 func WithFloat64HistogramBoundaries(boundaries []float64) Float64HistogramOption
 func WithInt64HistogramBoundaries(boundaries []float64) Int64HistogramOption

[]float64 vs ...float64

UX of ...float64 (proposed)

  histogram, err := meter.Float64Histogram(
	"task.duration",
	metric.WithExplicitBucketBoundaries(.005, .01, .025, .05, .075, .1, .25, .5, .75, 1, 2.5, 5, 7.5, 10),
  )

UX of []float64:

 histogram, err := meter.Float64Histogram(
   "task.duration",
   metric.WithExplicitBucketBoundaries([]float64{.005, .01, .025, .05, .075, .1, .25, .5, .75, 1, 2.5, 5, 7.5, 10}),
)

@dashpole dashpole added area:metrics Part of OpenTelemetry Metrics pkg:API Related to an API package enhancement New feature or request labels Oct 10, 2023
@dashpole dashpole force-pushed the explicit_bucket_advisory branch from df9521e to 3972b9f Compare October 10, 2023 19:30
@codecov
Copy link

codecov bot commented Oct 10, 2023

Codecov Report

Merging #4603 (34366d5) into main (7e6da12) will increase coverage by 0.0%.
Report is 1 commits behind head on main.
The diff coverage is 96.0%.

Impacted file tree graph

@@          Coverage Diff           @@
##            main   #4603    +/-   ##
======================================
  Coverage   81.4%   81.4%            
======================================
  Files        225     225            
  Lines      17728   17830   +102     
======================================
+ Hits       14431   14529    +98     
- Misses      2998    3001     +3     
- Partials     299     300     +1     
Files Coverage Δ
metric/instrument.go 100.0% <100.0%> (ø)
metric/syncfloat64.go 100.0% <100.0%> (ø)
metric/syncint64.go 100.0% <100.0%> (ø)
sdk/metric/metricdata/metricdatatest/assertion.go 90.0% <100.0%> (+0.7%) ⬆️
sdk/metric/metricdata/data.go 44.4% <0.0%> (-5.6%) ⬇️
...dk/metric/metricdata/metricdatatest/comparisons.go 91.3% <96.2%> (+0.6%) ⬆️

@dashpole dashpole force-pushed the explicit_bucket_advisory branch 3 times, most recently from 9871661 to 67f421e Compare October 10, 2023 19:39
@dashpole dashpole marked this pull request as ready for review October 10, 2023 19:51
metric/instrument.go Show resolved Hide resolved
@dashpole dashpole force-pushed the explicit_bucket_advisory branch from f98fb01 to 983a7c7 Compare October 12, 2023 15:12
@dashpole dashpole force-pushed the explicit_bucket_advisory branch from 983a7c7 to 8fb33f3 Compare October 12, 2023 15:13
@pellared
Copy link
Member

Just to double-check. Can we merge this PR? 😉

@MrAlias
Copy link
Contributor

MrAlias commented Oct 17, 2023

Just to double-check. Can we merge this PR? 😉

@MadVikingGod ^

@MrAlias MrAlias added this to the v1.20.0 milestone Oct 24, 2023
@MadVikingGod MadVikingGod merged commit 0f5565a into open-telemetry:main Oct 26, 2023
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:metrics Part of OpenTelemetry Metrics enhancement New feature or request pkg:API Related to an API package
Projects
Development

Successfully merging this pull request may close these issues.

5 participants