Oximeter self-stat tests don't need to be based on time

[bnaecker]

• Fixes Test flake: oximeter-collector agent::tests::test_self_stat_collection_count #4657
• Oximeter self stat tests run a few collections, and verify that the relevant counters keep track of those. Previously, that was based on time, advancing time forward until the expected number of collections actually occur. There's nothing really guaranteeing that those collections do occur, since the tasks may not run exactly when we expect. This commit updates the tests so that the producer explicitly updates a shared counter every time they receive a request. The tests then reduce to asserting this counter equals the self-stat counter.

[sunshowers]

Thanks for fixing this!

I'm wondering whether we want to provide the time source as a dependency rather than directly relying on system time. For example, providing a stream of ticks as an argument. Then that could be standard time for production and a custom, manually controlled impl for tests.

[bnaecker]

Thanks for the review @sunshowers!

I'm wondering whether we want to provide the time source as a dependency rather than directly relying on system time. For example, providing a stream of ticks as an argument. Then that could be standard time for production and a custom, manually controlled impl for tests.

I'm not sure I follow this, sorry! This stream-of-ticks is an argument to what, exactly? It seems like there is a test-specific notion of time, since we use tokio::time::{pause,advance} and friends in tests, but that may be different than what you're suggesting.

[sunshowers]

This stream-of-ticks is an argument to what, exactly?

To the service sending oximeter events. I assumed the events were sent on a regular schedule.

The tokio::time stuff is cool but it's a pretty big hammer, I think.

[bnaecker]

To the service sending oximeter events. I assumed the events were sent on a regular schedule.

Ok, I think what you're proposing is that instead of waiting for a timer internally, the collection task would wait on a channel onto which an external entity could send those notifications to collect. Is that accurate?

If so, I think we have something similar already: The tasks wait on a timer expiration, and also a channel on which we can request explicit collections. I could have updated this test to use that directly, I think I just forgot about it until now.

[sunshowers]

Ok, I think what you're proposing is that instead of waiting for a timer internally, the collection task would wait on a channel onto which an external entity could send those notifications to collect. Is that accurate?

Yes.

If so, I think we have something similar already: The tasks wait on a timer expiration, and also a channel on which we can request explicit collections. I could have updated this test to use that directly, I think I just forgot about it until now.

Ahh. Yeah, maybe the test could just disable timer-based collection (if that makes sense) and always rely on explicit collections. Or it could model the timer-based collection as another stream or channel, if you want to include that as part of the test.