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

Attempt to write metric tests with inmemoryexp #1382

Merged

Conversation

cijothomas
Copy link
Member

@cijothomas cijothomas commented Nov 17, 2023

Attempting to write Metrics test to ensure SDK is doing the aggregation as expected. This is full of "hacks" to unblock.! Need to spend some time to understand the inner workings to fix this - If anyone can help, really appreciate it! Thanks @TommyCpp who suggested the fix for flush being stuck!

Such tests are very critical to make sure the aggregation is correct, especially considering we are considering big changes like the ones in this PR - #1379

@cijothomas cijothomas requested a review from a team November 17, 2023 19:49
Copy link

codecov bot commented Nov 17, 2023

Codecov Report

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

Comparison is base (9e2e3db) 55.6% compared to head (364d736) 57.1%.
Report is 1 commits behind head on main.

Files Patch % Lines
opentelemetry-sdk/src/metrics/mod.rs 98.7% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##            main   #1382     +/-   ##
=======================================
+ Coverage   55.6%   57.1%   +1.4%     
=======================================
  Files        145     146      +1     
  Lines      18022   18099     +77     
=======================================
+ Hits       10037   10350    +313     
+ Misses      7985    7749    -236     

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

@cijothomas cijothomas marked this pull request as draft November 18, 2023 22:25
Copy link
Contributor

@hdost hdost left a comment

Choose a reason for hiding this comment

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

Looks like a winner to me 🤘 one step closer to stability.

Copy link
Contributor

@TommyCpp TommyCpp left a comment

Choose a reason for hiding this comment

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

Good first step 👍 we can try table-driven unit tests when adding more test cases

Copy link
Member

@lalitb lalitb left a comment

Choose a reason for hiding this comment

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

LGTM. Nicely done :)


counter.add(1, &[KeyValue::new("key1", "value2")]);
counter.add(1, &[KeyValue::new("key1", "value2")]);
counter.add(1, &[KeyValue::new("key1", "value2")]);
Copy link
Member

@lalitb lalitb Nov 20, 2023

Choose a reason for hiding this comment

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

nit - attributes can be defined once, and reused - something like (not tested):

let key_value1 = KeyValue::new("key1", "value1"); 
let key_value2 = KeyValue::new("key1", "value2");

for _ in 0..5 {
    counter.add(1, &[key_value1]);  // or &[key_value1.clone()] if metrics SDK takes ownership

for _ in 0..3 {
    counter.add(1, &[key_value2]);
}

@cijothomas cijothomas marked this pull request as ready for review November 20, 2023 17:29
@cijothomas
Copy link
Member Author

This is far from ideal, but merging as is, give this works, and can be used to quickly validate that aggregations/merges are done correctly, especially when working on PRs like #1379

Expecting to improve such tests in follow ups. Thanks everyone for helping review!

@cijothomas cijothomas merged commit 28aca99 into open-telemetry:main Nov 20, 2023
15 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.

4 participants