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

fix(sdk-metrics): merge uncollected delta accumulations #3667

Merged
merged 2 commits into from
Mar 14, 2023

Conversation

legendecas
Copy link
Member

@legendecas legendecas commented Mar 10, 2023

Which problem is this PR solving?

Fixes #3664

Short description of the changes

When two metric readers are registered and an async instrument is created, two async metric storages are created for each metric reader.

When one metric reader initiates collection on the async instrument, both async metric storages recorded the observed values. However, only one async metric storage that corresponds to the initiating metric reader is collected.

Next, when another metric reader initiates collection, both async metric storages recorded the observed values. But the async metric storage that corresponds to the reader has uncollected active delta accumulations. Those delta accumulations should be merged.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

  • DeltaMetricProcessor.batchCumulate should merge uncollected active delta.
  • Regression test: packages/sdk-metrics/test/regression/two-metric-readers-async-instrument.test.ts.

Checklist:

  • Followed the style guidelines of this project
  • Unit tests have been added
  • Documentation has been updated

@legendecas legendecas force-pushed the metrics/cumulative-batch branch from 20e8415 to e805525 Compare March 10, 2023 08:48
@legendecas legendecas marked this pull request as ready for review March 10, 2023 08:49
@legendecas legendecas requested a review from a team March 10, 2023 08:49
@legendecas legendecas added the sdk:metrics Issues and PRs related to the Metrics SDK label Mar 10, 2023
@codecov
Copy link

codecov bot commented Mar 10, 2023

Codecov Report

Merging #3667 (1c5a497) into main (d82a098) will decrease coverage by 0.22%.
The diff coverage is 100.00%.

❗ Current head 1c5a497 differs from pull request most recent head 6ba09a2. Consider uploading reports for the commit 6ba09a2 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3667      +/-   ##
==========================================
- Coverage   93.93%   93.71%   -0.22%     
==========================================
  Files         256      277      +21     
  Lines        7152     8451    +1299     
  Branches     1472     1755     +283     
==========================================
+ Hits         6718     7920    +1202     
- Misses        434      531      +97     
Impacted Files Coverage Δ
...ages/sdk-metrics/src/state/DeltaMetricProcessor.ts 100.00% <100.00%> (ø)

... and 26 files with indirect coverage changes

@legendecas legendecas force-pushed the metrics/cumulative-batch branch from e805525 to 41d3d1e Compare March 10, 2023 09:01
Copy link
Member

@pichlermarc pichlermarc left a comment

Choose a reason for hiding this comment

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

Looks good, thank you for taking care of it. 🙂

@legendecas legendecas merged commit 2276977 into open-telemetry:main Mar 14, 2023
@legendecas legendecas deleted the metrics/cumulative-batch branch March 14, 2023 16:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sdk:metrics Issues and PRs related to the Metrics SDK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

*two* MetricReaders on a MeterProvider interfere with each other's async metric data point values
2 participants