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.

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

Fix Shutdown behavior for batchprocessor #2537

Conversation

tigrannajaryan
Copy link
Member

@tigrannajaryan tigrannajaryan commented Feb 23, 2021

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.

@tigrannajaryan tigrannajaryan requested a review from a team February 23, 2021 20:33
@codecov
Copy link

codecov bot commented Feb 23, 2021

Codecov Report

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

Impacted file tree graph

@@            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.

Copy link
Member

@bogdandrutu bogdandrutu left a comment

Choose a reason for hiding this comment

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

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?

component/componenttest/shutdown_verifier.go Outdated Show resolved Hide resolved
@bogdandrutu
Copy link
Member

@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
Copy link
Member Author

@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
Copy link
Member Author

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.

Copy link
Member

@bogdandrutu bogdandrutu left a comment

Choose a reason for hiding this comment

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

Consider to remove build.log?

@tigrannajaryan
Copy link
Member Author

Consider to remove build.log?

Oops. Removed.

component/componenttest/shutdown_verifier.go Outdated Show resolved Hide resolved
component/componenttest/shutdown_verifier.go Outdated Show resolved Hide resolved
processor/batchprocessor/batch_processor.go Outdated Show resolved Hide resolved
@tigrannajaryan tigrannajaryan force-pushed the feature/tigran/shutdown-test branch 2 times, most recently from c980f54 to 6c488e4 Compare March 1, 2021 14:57
@tigrannajaryan
Copy link
Member Author

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.

component/componenttest/shutdown_verifier.go Outdated Show resolved Hide resolved
if err == configerror.ErrDataTypeIsNotSupported {
return
}
require.NoError(t, err)
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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).

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.
@jpkrohling
Copy link
Member

Tests restarted.

This was referenced Mar 15, 2021
@tigrannajaryan tigrannajaryan deleted the feature/tigran/shutdown-test branch March 3, 2022 16:56
hughesjj pushed a commit to hughesjj/opentelemetry-collector that referenced this pull request Apr 27, 2023
…2537)

Enable fluentforward receiver in the default ECS/EC2 configuration so users with [docker logging driver](https://docs.docker.com/config/containers/logging/fluentd/) can easily forward their logs through the collector
Troels51 pushed a commit to Troels51/opentelemetry-collector that referenced this pull request Jul 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants