Fix BatchSpanProcessor.Shutdown to wait until all spans are processed

[vmihailenco]

Currently it exits too soon - before drainQueue is finished

[Aneurysm9]

Is there a test case that can be added to demonstrate the faulty behavior and ensure it isn't reintroduced?

[jmacd]

This might never exit, I think.

I believe it's WAI. If you shutdown the processor while there are still spans being generated, some of them will not be recorded. What are the semantics you want, here?

[vmihailenco]

I would like to be able to understand what is "working as intented" here and how it might never exit. That is not first time I hear that from you so I guess I should point out that it does not help in any way except politely says "no" without any explanation (keep guessing).

If you shutdown the processor while there are still spans being generated, some of them will not be recorded

Given that after shutdown spans are just not added to the internal queue (channel) I don't see what to discuss here. You are right - they are discarded.

What are the semantics you want, here?

I am not particularly interested in any behavior here. But the code does not do what the comment says and what common sense suggests. Perhaps it makes sense to update the comment. Closing since it is WAI and I don't use this.

[jmacd]

I could be wrong, but it looked like drainQueue() would continue to process spans as long sufficiently many of them are generated to never fully drain the queue. I think we should expose a Flush() operation to flush the processor -- there is also a spec issue written on this topic -- that would allow a user to flush and then shutdown without losing spans.

[jmacd]

You're correct. It's clear that I'm not providing an effective review here, and I'm sorry. I've explained why I would not use this code (because it does not provide a way to limit encoded batch size), and that I only became involved because flaky tests were interfering with the metrics SDK development.

It worries me that we have disabled the tests for this class and are still making changes. What you point out is true, and we could either remove stopCh and drainQueue or we could make this change. I am concerned that even with this change, the existing test wouldn't pass.

The underlying problem seems to be that there is no "correct" way to shut down a processor when there are concurrent writers. I now believe that adding Flush() is the best path forward. We can write a correctness test for Flush() that ensures all spans are written, then all we need to test is that Shutdown closes channels, doesn't block indefinitely, and doesn't panic.

[vmihailenco]

I see at least 2 reasons why tests were flaky:

• Shutdown exits before drainQueue is finished.
• some tests don't use WithBlocking so spans can be dropped if scheduling is lucky/unlucky

This PR fixes both so hopefully tests should pass reliably again. And it is not like previously tests were working properly - they were passing, but not testing that code works correctly.

The underlying problem seems to be that there is no "correct" way to shut down a processor when there are concurrent writers.

Agreed. Shutdown only guarantees that spans added before the shutdown are processed. It does not help with concurrent writers - you should stop/synchronize them separately.

Tests should work fine because all spans are added before calling Shutdown and now tests use WithBlocking so spans can't be dropped when queue is full.

[jmacd]

The last time I wrangled with this topic, I did add WithBlocking() in the tests, but I wasn't able to make the test pass. We discussed this in the SIG call today, and I think the best we could do in terms of giving the user proper semantics, would be to add a Flush() call (with tests) and then simplify Shutdown.

[linux-foundation-easycla]

• ✅ Joshua MacDonald (00dd83d, 4b3feb6)

• ❌ Vladimir Mihailenco The commit (44cfb06 ,5a80a0b8612a7328026263f04cef2cadf2f4121b ,63d524e17a707d6a4fc8cab2d14e7160d595a333 ,09c277d76d6af09a23fd69b3dd8eccf107d415a3) is not authorized under a signed CLA. Please click here to be authorized. For further assistance with EasyCLA, please submit a support request ticket.

[jmacd]

Thank you @vmihailenco, sorry for the delay, and sorry for several mistakes of mine in reviewing this code.

[jmacd]

@lizthegrey as our compliance officer, would you look at the error above (#766 (comment))? I can't see why the build is green given that message.

[jmacd]

@lizthegrey nevermind. I see you posted on another PR that we have to re-authorize the CLA.
However, this makes me think the test is broken because the build is green. Thoughts?

[jmacd]

This restores tests! Merging.