Use ValueMap for Histogram- Throughput increased by 5x

[utpilla]

Follow-up to #2012
Similar to #1833. #1989, #2017
Related to #1740

Changes

• Update Histogram to use ValueMap

Machine information

OS: Ubuntu 22.04.4 LTS (5.15.153.1-microsoft-standard-WSL2) Hardware: Intel(R) Xeon(R) Platinum 8370C CPU @ 2.80GHz, 16vCPUs, RAM: 64.0 GB

Benchmarks

There is a ~60% increase in benchmark performance. This is coming from ValueMap's optimized lookup on the hot path.

Histogram_Record_With_Three_Random_Attributes	Average time
main branch	509.21 n
PR	193.04 ns

Stress Test

Throughput has improved by 5 times. This is coming from ValueMap's reduced thread contention and optimized lookup on the hot path.

16 threads updating random trackers	Throughput
main branch	1.8 M/sec
PR	9 M/sec

Merge requirement checklist

• CONTRIBUTING guidelines followed
• Unit tests added/updated (if applicable)
• Appropriate CHANGELOG.md files updated for non-trivial, user-facing changes
• Changes in public API reviewed (if applicable)

[utpilla]

ValueMap struct takes three generic parameters now. There is scope to further refactor this. We could update AtomicTracker in a way that would allow us to get rid of Operation trait. I didn't want to move too many things around in this PR.

[codecov]

Codecov Report

Attention: Patch coverage is 69.74359% with 59 lines in your changes missing coverage. Please review.

Project coverage is 76.6%. Comparing base (108161c) to head (8b92798).

Files	Patch %	Lines
...pentelemetry-sdk/src/metrics/internal/histogram.rs	65.9%	48 Missing ⚠️
opentelemetry-sdk/src/metrics/internal/mod.rs	77.0%	11 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##            main   #2033     +/-   ##
=======================================
- Coverage   76.7%   76.6%   -0.1%     
=======================================
  Files        122     122             
  Lines      20828   20916     +88     
=======================================
+ Hits       15993   16041     +48     
- Misses      4835    4875     +40     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[utpilla]

One difference now is that we would always update the sum. Previously, this was happening only when self.record_sum is true. I think we are not required to provide record_sum as a configuration option in the first place. Currently, it is set based on this condition:

opentelemetry-rust/opentelemetry-sdk/src/metrics/pipeline.rs

Lines 477 to 482 in 108161c

	let record_sum = !matches!(
	kind,
	InstrumentKind::UpDownCounter
	| InstrumentKind::ObservableUpDownCounter
	| InstrumentKind::ObservableGauge
	);

[cijothomas]

spec:

Arithmetic sum of Measurement values in population. This SHOULD NOT be collected when used with instruments that record negative measurements (e.g. UpDownCounter or ObservableGauge).

This scenario occurs only if a View is used to aggregate UpDownCounter/Gauge as Histogram. Lets address this (if we chose to), in a future PR.

[cijothomas]

Additionally, I am unsure if SUM should be not reported, when -ve measurements are provided, which are currently not blockeable at API level for f64...

Proto wording is not super clear to me...Lets follow up later.
https://github.com/open-telemetry/opentelemetry-proto/blob/v0.9.0/opentelemetry/proto/metrics/v1/metrics.proto#L538-L543

[cijothomas]

Nice gains!
The generic/awkwardness is not a blocker as they are purely internal implementations, and can be improved without breaking.