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

Add more tests to Metric SDK aggregation #1600

Merged

Conversation

cijothomas
Copy link
Member

Adding more tests to test meter and instrument identify part, and whether they get merged or not.

@cijothomas cijothomas requested a review from a team March 5, 2024 07:30
Copy link

codecov bot commented Mar 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.3%. Comparing base (d5bf258) to head (d1f2ae8).

Additional details and impacted files
@@           Coverage Diff           @@
##            main   #1600     +/-   ##
=======================================
+ Coverage   67.0%   67.3%   +0.3%     
=======================================
  Files        138     138             
  Lines      19479   19654    +175     
=======================================
+ Hits       13058   13243    +185     
+ Misses      6421    6411     -10     

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

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.

Thanks for the tests.

opentelemetry-sdk/src/metrics/mod.rs Outdated Show resolved Hide resolved
opentelemetry-sdk/src/metrics/mod.rs Outdated Show resolved Hide resolved
opentelemetry-sdk/src/metrics/mod.rs Outdated Show resolved Hide resolved
opentelemetry-sdk/src/metrics/mod.rs Show resolved Hide resolved
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.

the test looks good. I'd add some more details on what we are testing here.

I personally like table driven unit tests so if we can use for loop here may save us some duplication

Comment on lines +219 to +220
// "multi_thread" tokio flavor must be used else flush won't
// be able to make progress!
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: don't think we need this for every test but not a strong opinion

Comment on lines +229 to +245
let meter1 = meter_provider.meter("test.meter1");
let meter2 = meter_provider.meter("test.meter2");
let counter1 = meter1
.u64_counter("my_counter")
.with_unit(Unit::new("my_unit"))
.with_description("my_description")
.init();

let counter2 = meter2
.u64_counter("my_counter")
.with_unit(Unit::new("my_unit"))
.with_description("my_description")
.init();

let attribute = vec![KeyValue::new("key1", "value1")];
counter1.add(10, &attribute);
counter2.add(5, &attribute);
Copy link
Contributor

Choose a reason for hiding this comment

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

We can just loop over test.meter1 and test.meter2 here?

opentelemetry-sdk/src/metrics/mod.rs Show resolved Hide resolved
@cijothomas cijothomas merged commit a80dedf into open-telemetry:main Mar 12, 2024
17 checks passed
sreeo pushed a commit to sreeo/opentelemetry-rust that referenced this pull request Mar 12, 2024
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.

3 participants