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.

Sign up for GitHub

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

Jump to bottom

Add tests for periodic reader from various RT combinations #2147

Merged
merged 2 commits into from
Sep 25, 2024
Merged

Add tests for periodic reader from various RT combinations #2147

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
909852e
Add tests for periodic reader from various RT combinations
cijothomas Sep 25, 2024
c9b4e78
fmt
cijothomas Sep 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 60 additions & 21 deletions opentelemetry-sdk/src/metrics/periodic_reader.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -381,29 +381,41 @@ mod tests {
use std::sync::mpsc;

#[test]
fn collection_triggered_by_interval() {
// Arrange
let interval = std::time::Duration::from_millis(1);
let exporter = InMemoryMetricsExporter::default();
let reader = PeriodicReader::builder(exporter.clone(), runtime::TokioCurrentThread)
.with_interval(interval)
.build();
let (sender, receiver) = mpsc::channel();
fn collection_triggered_by_interval_tokio_current() {
collection_triggered_by_interval_helper(runtime::TokioCurrentThread);
}

// Act
let meter_provider = SdkMeterProvider::builder().with_reader(reader).build();
let meter = meter_provider.meter("test");
let _counter = meter
.u64_observable_counter("testcounter")
.with_callback(move |_| {
sender.send(()).expect("channel should still be open");
})
.init();
#[tokio::test(flavor = "multi_thread", worker_threads = 1)]
async fn collection_triggered_by_interval_from_tokio_multi_one_thread_on_runtime_tokio() {
collection_triggered_by_interval_helper(runtime::Tokio);
}

// Assert
receiver
.recv()
.expect("message should be available in channel, indicating a collection occurred");
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn collection_triggered_by_interval_from_tokio_multi_two_thread_on_runtime_tokio() {
collection_triggered_by_interval_helper(runtime::Tokio);
}

#[tokio::test(flavor = "multi_thread", worker_threads = 1)]
async fn collection_triggered_by_interval_from_tokio_multi_one_thread_on_runtime_tokio_current()
{
collection_triggered_by_interval_helper(runtime::TokioCurrentThread);
}

#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn collection_triggered_by_interval_from_tokio_multi_two_thread_on_runtime_tokio_current()
{
collection_triggered_by_interval_helper(runtime::TokioCurrentThread);
}

#[tokio::test(flavor = "current_thread")]
#[ignore = "See issue https://github.com/open-telemetry/opentelemetry-rust/issues/2056"]
async fn collection_triggered_by_interval_from_tokio_current_on_runtime_tokio() {
collection_triggered_by_interval_helper(runtime::Tokio);
}

#[tokio::test(flavor = "current_thread")]
async fn collection_triggered_by_interval_from_tokio_current_on_runtime_tokio_current() {
collection_triggered_by_interval_helper(runtime::TokioCurrentThread);
}

#[test]
Expand All @@ -424,4 +436,31 @@ mod tests {
matches!(result.unwrap_err(), MetricsError::Other(err) if err == "reader is not registered")
);
}

fn collection_triggered_by_interval_helper<RT>(runtime: RT)
where
RT: crate::runtime::Runtime,
{
let interval = std::time::Duration::from_millis(1);
let exporter = InMemoryMetricsExporter::default();
let reader = PeriodicReader::builder(exporter.clone(), runtime)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we expect to merge #2142 soon? If that's the case, then these tests would have to be removed/skipped. Maybe add these tests post #2142 once we have a mechanism to accept custom runtimes?

Copy link
Member Author

Choose a reason for hiding this comment

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

I am using these tests to primarily validate that when 2142 approach is merged, we'll not have hangs/deadlocks, not matter which async runtime user is on.

(Ofcourse, will modify these tests to match 2142 later, as there will be no need of specifying runtime, and reducing test matrix too)

Copy link
Contributor

Choose a reason for hiding this comment

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

Not as inconvenient but I guess the simpler thing (for review) would be to have the tests added in #2142 itself. Otherwise, we just increase the amount of diff for review.

.with_interval(interval)
.build();
let (sender, receiver) = mpsc::channel();

// Act
let meter_provider = SdkMeterProvider::builder().with_reader(reader).build();
let meter = meter_provider.meter("test");
let _counter = meter
.u64_observable_counter("testcounter")
.with_callback(move |_| {
sender.send(()).expect("channel should still be open");
})
.init();

// Assert
receiver
.recv()
.expect("message should be available in channel, indicating a collection occurred");
}
}
Loading