Skip to content

Memory mode: Adding support for synchronous instruments - Counter#6182

Merged
jack-berg merged 2 commits intoopen-telemetry:mainfrom
streamnative:memory-mode-sync-instruments-counter
Jan 31, 2024
Merged

Memory mode: Adding support for synchronous instruments - Counter#6182
jack-berg merged 2 commits intoopen-telemetry:mainfrom
streamnative:memory-mode-sync-instruments-counter

Conversation

@asafm
Copy link
Copy Markdown
Contributor

@asafm asafm commented Jan 30, 2024

Epic

This is one of many PRs that implement #5105. See the complete plan here.
Specifically, this PR adds the implementation of MemoryMode for the synchronous instrument: counter.

What was done here?

This PR, relative to Exponential and Explicit Histogram PR, is rather trivial. It contains:

  • Using a reusable point for returning the result on doAggregateThenMaybeReset for both LongSumAggregator.Handle and DoubleSumAggregator.Handle.
  • Adding matching garbage collection benchmark tests
  • Adjusting and adding tests for LongSumAggregator and DoubleSumAggregator
  • Allowed to set the normalized allocate rate reduction percentage since, for both those aggregations, there was only a single allocation per Attributes, hence the reduction could not reach 99.8 as it did for the previous Exponential and Explicit Histogram aggregations.

@asafm asafm requested a review from a team January 30, 2024 09:15
This was referenced Jan 30, 2024
Closed
Open
@codecov
Copy link
Copy Markdown

codecov Bot commented Jan 30, 2024 •
edited
Loading

Codecov Report

Attention: 27 lines in your changes are missing coverage. Please review.

Comparison is base (f123d78) 90.97% compared to head (e4b856b) 90.94%.

Files Patch % Lines
...nal/data/MutableExponentialHistogramPointData.java 12.50% 0 Missing and 14 partials ⚠️
.../metrics/internal/data/MutableDoublePointData.java 14.28% 0 Missing and 6 partials ⚠️
...dk/metrics/internal/data/MutableLongPointData.java 14.28% 0 Missing and 6 partials ⚠️
...ernal/data/MutableExponentialHistogramBuckets.java 66.66% 0 Missing and 1 partial ⚠️
Additional details and impacted files 
@@             Coverage Diff              @@
##               main    #6182      +/-   ##
============================================
- Coverage     90.97%   90.94%   -0.03%     
- Complexity     5670     5672       +2     
============================================
  Files           620      620              
  Lines         16535    16555      +20     
  Branches       1682     1695      +13     
============================================
+ Hits          15042    15056      +14     
+ Misses         1023     1009      -14     
- Partials        470      490      +20

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

jack-berg
jack-berg approved these changes Jan 30, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@jack-berg jack-berg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple minor comments, but looks good. Thanks!

Comment thread ...cs/src/jmhBasedTest/java/io/opentelemetry/sdk/metrics/internal/state/TestInstrumentType.java Outdated
Comment thread ...metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/data/MutableDoublePointData.java
@asafm
Copy link
Copy Markdown
Contributor Author

asafm commented Jan 31, 2024

@jack-berg pushed all fixes

@jack-berg jack-berg merged commit 0aa223d into open-telemetry:main Jan 31, 2024
@asafm asafm deleted the memory-mode-sync-instruments-counter branch February 1, 2024 12:46
@asafm asafm mentioned this pull request Apr 1, 2024
Closed
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jack-berg jack-berg jack-berg approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@asafm @jack-berg