enhancement(sinks): Adaptive Request Concurrency

[bruceg]

Here is my implementation of the automatic concurrency management, which is in use in all "service2" based sinks (which appears to be everything except prometheus and aws_cloudwatch_logs).

I will continue to work on the tests started in auto_concurrency/service.rs, but otherwise I believe this is feature complete.

Closes #2529

[MOZGIII]

Might be useful to switch to try_lock here and in the rest of the places if possible - to avoid the effects of additional locking affecting test results.

[bruceg]

So, if we use lock, then the stats may not reflect how the sink will operate in practice because there is additional lock contention. If we use try_lock, then the stats may not reflect how the sink actually did operate because it sometimes didn't take the lock and so couldn't report stats, resulting in incomplete data. Either way, stats can't always reflect actual operation, which I agree is unfortunate. Using try_lock also requires additional conditionals when trying to report stats. I think I'll stick with lock unless there is another consideration.

Now, I can reorganize the code a bit to make the time the mutex is held even shorter, which will reduce the effect. Do you have any thoughts on whether it is better to take the mutex once and hold it for a bit longer in adjust_to_back_pressure, or to take it once for each place it is needed and drop it immediately?

[MOZGIII]

Yeah, I see the issue with the try_lock access to stats.

I had some thoughts on using lock-free ring buffers (channels?) to collect the stats for test purposes, and then to aggregate them separately. But it's very early - I'm still reading through the code and wrapping my head around what's going on.

A bit preliminary, but to me, implementing channel based mocks to tightly control the execution in a tight lock-step looks promising here. This would allow us to assert the states in all the right places, and eliminate the need for manual locking and associated race conditions.

The idea is we use a channel to send notifications that we reached a certain state to the test controlling logic along the way, and we then wait on another rx channel to continue. The test logic waits for a particular notification, does the assertion, and sends a message that lets the tested code continue the execution. We might even be able to use the exact values in the tests and make the tests deterministic.

Again, this is very preliminary - I'll have to take a deeper look if this has any actual benefits over what we have already - but I'm very curious to hear what are your thoughts on this.

[bruceg]

I had considered using channels for reporting test metrics, but couldn't see a path that was as straightforward as using locking. It is worth considering, though.

I see there being roughly 3 levels of testing: low level (does it take the steps that the code says it should), behavior (do the steps give it the expected high-level behavior), and performance (does it interoperate optimally with live servers). It sounds like you are describing some fairly low-level tests, covering some of the steps tested in auto_concurrency/service.rs using tokio's timers to manually advance steps. It might be worth covering other paths though.

[MOZGIII]

Yes, you're right, the approach I'm thinking about is just briefly above - and very similar to - tracing the code along it's execution in lock-step, and asserting that exactly what's expected happens.

I'm thinking of tokio timers as just one of the ways to advance.
Consider having state at the test end rather than as part of the struct, and only passing it into the Controller code via a channel. That channel will pass ownership and access to state, and it'll have to naturally be passed between the logical threads to mix the updates and assertions.

[MOZGIII]

I ran tests locally and got some failures.

The command I used was TEST_LOG=trace cargo test --lib -- sinks::util::auto_concurrency --nocapture 2>&1 | tee log.txt.

Output: log.txt

UPD: if I do cargo test --lib without filtering the tests, the outcome is the same. It also reproduces reliably.

I encourage others to run the tests locally to gather more stats (literally).

[bruceg]

Hmm, I can definitely confirm the stats issues you are having, as I am seeing them now too. When I added the tests, though, I ran them many times to ensure the boundaries were reasonable. While it would be good to track down what changed, and to adjust the boundaries to ensure tests now pass, this makes me wonder if these tests are reliable enough to keep around.

[bruceg]

The issue is that commit 8605d64 switched the behavior of specifying a number for in_flight_limit, effectively turning the tests on to a fixed concurrency again instead of the variable concurrency. I discovered this while working on making the variable concurrency opt-in, and I have a fix.

[MOZGIII]

Looks good overall! We might want to revisit tests though.

[lukesteensen]

Looks good! Excited to get this in and collect some real-world feedback.