Fix Shutdown behavior for batchprocessor

[tigrannajaryan]

I added a Shutdown() test that does basic verification of the behavior of the
Shutdown() function. More verifications can be added later.

The test revealed a bug in batchprocessor Shutdown() function which would
not wait until all pending data was drained.

[codecov]

Codecov Report

Merging #2537 (f123dd2) into main (0ed7a4c) will decrease coverage by 0.04%.
The diff coverage is 78.94%.

@@            Coverage Diff             @@
##             main    #2537      +/-   ##
==========================================
- Coverage   91.76%   91.72%   -0.05%     
==========================================
  Files         265      266       +1     
  Lines       15082    15101      +19     
==========================================
+ Hits        13840    13851      +11     
- Misses        861      867       +6     
- Partials      381      383       +2     

Impacted Files	Coverage Δ
processor/batchprocessor/batch_processor.go	90.08% <ø> (ø)
component/componenttest/shutdown_verifier.go	78.94% <78.94%> (ø)
exporter/exporterhelper/metricshelper.go	94.59% <0.00%> (-5.41%)	⬇️
exporter/otlpexporter/otlp.go	71.79% <0.00%> (-2.57%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0ed7a4c...f123dd2. Read the comment docs.

[bogdandrutu]

I am not sure what is the right thing to do here. Do we want every component to protect the ConsumeSignal, or we want to guarantee from the framework that no calls to ConsumeSignal come after Shutdown?

[bogdandrutu]

@tigrannajaryan also if this is something we want to do, to reject incoming requests if component is shutting down, we should probably do this in the processorhelper :)

[tigrannajaryan]

@tigrannajaryan also if this is something we want to do, to reject incoming requests if component is shutting down, we should probably do this in the processorhelper :)

Yes, that's coming in a future PR.

[tigrannajaryan]

I am not sure what is the right thing to do here. Do we want every component to protect the ConsumeSignal, or we want to guarantee from the framework that no calls to ConsumeSignal come after Shutdown?

I am going to add protection in helpers so it will be in one place. Components that don't use helper will have to do it themselves.

[bogdandrutu]

Consider to remove build.log?

[tigrannajaryan]

Consider to remove build.log?

Oops. Removed.

[tigrannajaryan]

I removed most of the original code which was attempting to protect the processor from Consume* calls happening after Shutdown() is called. According to Component contract this should just not happen, so we don't need to protect from it, but instead make sure all components follow the contract (never call Consume* of the next component after Shutdown() returns).

This PR now merely ensure batchprocessor follows the contract. The test verifies it.

[bogdandrutu]

I know that it sounds strange to have unittest for testing helpers but for this logic is probably good :) Maybe in a separate PR?

[tigrannajaryan]

There is a TODO to handle other data type later in the file. We can address as part of that TODO (this code may change as a result of that, so I don't want to spend time on it now).

[jpkrohling]

Tests restarted.