chore: Add metric integration tests #2424
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2424 +/- ##
=====================================
Coverage 79.3% 79.3%
=====================================
Files 122 122
Lines 21569 21565 -4
=====================================
Hits 17114 17114
+ Misses 4455 4451 -4 ☔ View full report in Codecov by Sentry. |
| meter_provider | ||
| } | ||
|
|
||
| pub async fn metrics() -> Result<(), Box<dyn Error + Send + Sync + 'static>> { |
There was a problem hiding this comment.
The actual meters we're pushing through for the test
| @@ -0,0 +1,133 @@ | |||
| { | |||
There was a problem hiding this comment.
same as metrics.json but with minor changes to validate that our comparison is working.
| @@ -6,114 +6,128 @@ | |||
| { | |||
There was a problem hiding this comment.
This file was already here, but was seemingly unused, so i've repurposed it to match the style of the other tests
|
@cijothomas i've fleshed out the basic pattern to add metrics integration coverage here; it still needs to cover the rest of the meters, and I need to deal with the windows build issues. I don't need a proper review yet, but if you have a second to glance over it let me know if anything seems fundamentally different to what you expect. |
| @@ -7,7 +7,7 @@ publish = false | |||
|
|
|||
| [dependencies] | |||
| opentelemetry = { path = "../../../opentelemetry", features = ["metrics", "logs"] } | |||
| opentelemetry_sdk = { path = "../../../opentelemetry-sdk", features = ["rt-tokio", "logs", "testing"] } | |||
| opentelemetry_sdk = { path = "../../../opentelemetry-sdk", features = ["rt-tokio", "logs", "testing", "metrics"] } | |||
There was a problem hiding this comment.
nit: we can remove logs,metrics features as they are anyway by default added.
| let mut result_scope_metrics = &mut result_resource_metrics.scope_metrics; | ||
| let mut expected_scope_metrics = &mut expected_resource_metrics.scope_metrics; | ||
|
|
||
| // Zero out all the timestamps so we can usual the default comparisons |
| let meter_provider = init_metrics(); | ||
|
|
||
| let meter = meter_provider.meter("meter"); | ||
| let counter = meter.u64_counter("counter_u64").build(); |
There was a problem hiding this comment.
please add one observable instrument like observable counter .
| // wait for file to flush to disks | ||
| // ideally we should use volume mount but otel collector file exporter doesn't handle permission too well | ||
| // bind mount mitigate the issue by set up the permission correctly on host system | ||
| tokio::time::sleep(Duration::from_secs(5)).await; |
There was a problem hiding this comment.
while this test is important, we also need to add one where we rely on shutdown to push the metrics, instead of using a ultra-low interval for pushing metrics.
| @@ -54,6 +54,7 @@ impl TestSuite { | |||
| async fn integration_tests() { | |||
| trace_integration_tests().await; | |||
| logs_integration_tests().await; | |||
| metrics_integration_tests().await; | |||
There was a problem hiding this comment.
Suggestion for future follow-up PR: While this works fine for now, using a shared collector setup across signals could improve efficiency and maintainability. Currently, the collector is started and stopped separately for each signal.
There was a problem hiding this comment.
That’s a good point - I’ll change it while I’m here, I don’t think it should be a problem as they are collecting separate signals anyhow.
+1 |
Contributes to #2401
Changes
Please provide a brief description of the changes here.
Merge requirement checklist
CHANGELOG.mdfiles updated for non-trivial, user-facing changes