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

Use Extrema type for Histogram min/max #3550

Merged
merged 19 commits into from Jan 26, 2023
Merged

Conversation

MrAlias
Copy link
Contributor

@MrAlias MrAlias commented Dec 19, 2022

Resolve #3201

Prevent two allocations for every HistogramDataPoint that is created by replacing the Min and Max pointer fields with a new Extrema type. Meaning, this

type HistogramDataPoint struct {
	Min *float64
	Max *float64
}

becomes

type HistogramDataPoint struct {
	Min Extrema
	Max Extrema
}

with Extrema declared as:

// Extrema are the minimum or maximum values of a data set.
type Extrema struct {
	// value is the extrema value.
	value float64

	// valid is true if Value has been assigned a value. It is false if Value
	// is the zero-value.
	valid bool
}

See this gist for a comparison of the performance among the approaches proposed in #3201.

@MrAlias MrAlias added pkg:SDK Related to an SDK package area:metrics Part of OpenTelemetry Metrics labels Dec 19, 2022
@MrAlias MrAlias changed the title Use Extrema type for Histogram min/max PoC: Use Extrema type for Histogram min/max Dec 19, 2022
@codecov
Copy link

codecov bot commented Dec 19, 2022

Codecov Report

Merging #3550 (6436bb3) into main (c0fb8de) will increase coverage by 0.0%.
The diff coverage is 100.0%.

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #3550   +/-   ##
=====================================
  Coverage   79.4%   79.4%           
=====================================
  Files        170     170           
  Lines      12628   12645   +17     
=====================================
+ Hits       10030   10047   +17     
  Misses      2388    2388           
  Partials     210     210           
Impacted Files Coverage Δ
...s/otlp/otlpmetric/internal/transform/metricdata.go 100.0% <100.0%> (ø)
sdk/metric/internal/histogram.go 100.0% <100.0%> (ø)
sdk/metric/metricdata/data.go 57.1% <100.0%> (+57.1%) ⬆️
sdk/metric/metricdata/metricdatatest/assertion.go 84.1% <100.0%> (+0.4%) ⬆️
...dk/metric/metricdata/metricdatatest/comparisons.go 97.5% <100.0%> (+<0.1%) ⬆️
semconv/internal/v2/http.go 100.0% <100.0%> (ø)

@MrAlias
Copy link
Contributor Author

MrAlias commented Jan 12, 2023

We should add benchmarks here to show qualitative values of different proposals.

@MrAlias
Copy link
Contributor Author

MrAlias commented Jan 12, 2023

We should add benchmarks here to show qualitative values of different proposals.

https://gist.github.com/MrAlias/fefe010bbee8671e65d4540af44911f5

@MrAlias MrAlias added this to the Metric v0.35.0 milestone Jan 13, 2023
@MrAlias MrAlias changed the title PoC: Use Extrema type for Histogram min/max Use Extrema type for Histogram min/max Jan 13, 2023
@MrAlias MrAlias marked this pull request as ready for review January 13, 2023 00:45
@MrAlias MrAlias merged commit 7f4d76a into open-telemetry:main Jan 26, 2023
@MrAlias MrAlias deleted the extrema branch January 26, 2023 18:50
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 pkg:SDK Related to an SDK package
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Consider using math.NaN() values for optional Sum, Min, and Max fields in metricdata.Histogram
5 participants