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

Delete bound instruments #5157

Merged
merged 2 commits into from
Feb 3, 2023

Conversation

jack-berg
Copy link
Member

@jack-berg jack-berg commented Jan 30, 2023

A while back there was an issue and PR to remove bound instruments. It's been over a year and there's been no movement on bound instruments in the spec, and they didn't end up being related to the batch callback API, which was added. Removing them saves ~1500 lines of code and gets rid of some of the trickiest code in the metrics SDK (tracking references to the bound instruments using high performance lock free techniques and bitshifting). After the optimizations I've been doing lately (#5154, #5142) the performance impact that bound instruments might have is small:

  • Bound instruments prevent needing to do a lookup in ConcurrentHashMap<Attributes,AggregatorHandle> since the caller holds on to a reference to a specific AggregatorHandle.
  • If temporality is delta, bound instruments prevent reallocating new AggregatorHandle after collection. But we can get around this by adding an object pool of AggregatorHandles, and reusing handles after collection.
  • If temporality is cumulative, there is no memory allocation advantage after Reuse AggregatorHandle with cumulative temporality #5142.

I think it's unlikely that the performance benefit of saving a lookup of Attributes in a map would warrant the increased API surface area and reduced ergonomics of bound instruments.

Deleting bound instruments results in a modest performance improvement on the hot path since there's less book keeping tracking whether bound storage handles have references. See the before and after.

@codecov
Copy link

codecov bot commented Jan 30, 2023

Codecov Report

Base: 90.99% // Head: 91.07% // Increases project coverage by +0.08% 🎉

Coverage data is based on head (1d308d6) compared to base (9cfdf67).
Patch coverage: 90.32% of modified lines in pull request are covered.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #5157      +/-   ##
============================================
+ Coverage     90.99%   91.07%   +0.08%     
+ Complexity     4881     4863      -18     
============================================
  Files           545      548       +3     
  Lines         14498    14360     -138     
  Branches       1383     1375       -8     
============================================
- Hits          13193    13079     -114     
+ Misses          910      890      -20     
+ Partials        395      391       -4     
Impacted Files Coverage Δ
...io/opentelemetry/sdk/metrics/SdkDoubleCounter.java 100.00% <ø> (ø)
.../opentelemetry/sdk/metrics/SdkDoubleHistogram.java 100.00% <ø> (ø)
...ntelemetry/sdk/metrics/SdkDoubleUpDownCounter.java 100.00% <ø> (ø)
...a/io/opentelemetry/sdk/metrics/SdkLongCounter.java 100.00% <ø> (ø)
...io/opentelemetry/sdk/metrics/SdkLongHistogram.java 100.00% <ø> (ø)
...pentelemetry/sdk/metrics/SdkLongUpDownCounter.java 100.00% <ø> (ø)
.../metrics/internal/aggregator/AggregatorHandle.java 88.23% <ø> (-5.32%) ⬇️
...rics/internal/state/AsynchronousMetricStorage.java 98.33% <ø> (ø)
...sdk/metrics/internal/state/EmptyMetricStorage.java 0.00% <0.00%> (ø)
...etry/sdk/metrics/internal/state/MetricStorage.java 0.00% <ø> (ø)
... and 22 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@jack-berg jack-berg marked this pull request as ready for review February 3, 2023 17:15
@jack-berg jack-berg requested a review from a team February 3, 2023 17:15
@jack-berg jack-berg merged commit 249d097 into open-telemetry:main Feb 3, 2023
@anuraaga
Copy link
Contributor

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants