opentelemetry-rust icon indicating copy to clipboard operation
opentelemetry-rust copied to clipboard

Add tests to confirm observable counter aggregation bug

Open cijothomas opened this issue 1 year ago • 2 comments

Observable counter has an aggregation bug. Most likely same issue exists for ObservableUpDownCounter as well. This PR just adds a test that will fail, and hence ignored for now. Will look at the fix separately.

cijothomas avatar Feb 08 '24 08:02 cijothomas

Codecov Report

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

Comparison is base (b35bf38) 62.4% compared to head (14c871a) 62.5%.

Files Patch % Lines
opentelemetry-sdk/src/metrics/mod.rs 55.0% 90 Missing :warning:
Additional details and impacted files
@@           Coverage Diff           @@
##            main   #1516     +/-   ##
=======================================
+ Coverage   62.4%   62.5%   +0.1%     
=======================================
  Files        144     144             
  Lines      19891   20091    +200     
=======================================
+ Hits       12426   12576    +150     
- Misses      7465    7515     +50     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Feb 08 '24 08:02 codecov[bot]

We can also consider the use of parameterized tests for scenarios where only the inputs and expected outputs differ, to reduce redundant test code. Rust doesn't support it directly, but there are external crates providing this option e.g., - https://crates.io/crates/parameterized_test , or we can create a custom macro as suggested here - https://stackoverflow.com/questions/34662713/how-can-i-create-parameterized-tests-in-rust/

Not related to this PR, for future :)

lalitb avatar Feb 15 '24 18:02 lalitb