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 generics to dispatch updates in ValueMap #2004

Conversation

stormshield-fabs
Copy link
Contributor

@stormshield-fabs stormshield-fabs commented Aug 8, 2024

Fixes #1517 (incorrect cumulative aggregation for observable counters).

This is a follow-up to #1992 as I cannot push to the original branch.

It re-introduces the ideas from #1644 : the ValueMap with an O (operation) parameter so that observations can be performed with the correct semantics (Increment for synchronous counters, Assign for observable ones).

I've accounted for the pending review comments on the initial PR and refactored the measure method a bit to limit right-ward drift.

Edit to include the additional gains with this change:

Additionally, the previous implementation of the Observable relied on the assumption that the ValueMap is drained after ever collect. This is an area that could be tweaked in the future. This PR removed that dependency anyway, so this is better future proof.

@stormshield-fabs stormshield-fabs requested a review from a team August 8, 2024 14:03
Copy link

codecov bot commented Aug 8, 2024

Codecov Report

Attention: Patch coverage is 90.40000% with 12 lines in your changes missing coverage. Please review.

Project coverage is 74.8%. Comparing base (6a16baf) to head (398fdba).

Files Patch % Lines
opentelemetry-sdk/src/metrics/internal/mod.rs 30.0% 7 Missing ⚠️
opentelemetry-sdk/src/metrics/internal/sum.rs 95.0% 4 Missing ⚠️
opentelemetry-sdk/src/metrics/mod.rs 97.0% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##            main   #2004     +/-   ##
=======================================
- Coverage   75.1%   74.8%   -0.3%     
=======================================
  Files        122     122             
  Lines      20642   20678     +36     
=======================================
- Hits       15503   15473     -30     
- Misses      5139    5205     +66     

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

Co-authored-by: Utkarsh Umesan Pillai <[email protected]>
Copy link
Member

@cijothomas cijothomas left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks for the extra cleanups.

@cijothomas cijothomas merged commit 2bba6b0 into open-telemetry:main Aug 9, 2024
25 checks passed
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.

ObservableCounter aggregation bug for cumulative
4 participants