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

[exporter/loadbalancer] close exporters on shutdown #36024

Conversation

VihasMakwana
Copy link
Contributor

@VihasMakwana VihasMakwana commented Oct 28, 2024

Description

I noticed that we don't close the exporters during shutdown. This PR iterates through the exporters and closes them in a graceful manner.

@VihasMakwana VihasMakwana force-pushed the loadbalancer-receiver-shutdown-exporters branch from 8ad359b to 8059a83 Compare October 28, 2024 13:37
@VihasMakwana VihasMakwana marked this pull request as draft October 28, 2024 14:01
@VihasMakwana VihasMakwana force-pushed the loadbalancer-receiver-shutdown-exporters branch from 064a037 to dd8e814 Compare October 28, 2024 20:22
@VihasMakwana VihasMakwana marked this pull request as ready for review October 28, 2024 20:22
@@ -220,6 +220,10 @@ func endpointFound(endpoint string, endpoints []string) bool {
func (lb *loadBalancer) Shutdown(ctx context.Context) error {
err := lb.res.shutdown(ctx)
lb.stopped = true

for _, e := range lb.exporters {
err = errors.Join(err, e.Shutdown(ctx))
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe this can be optimized down the road with a slice pre-allocated for number of exporters, and append errors to it and call errors.Join at the end. However, this is likely to not matter much here as a few exporters would exist here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree.

Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Nov 15, 2024
@VihasMakwana
Copy link
Contributor Author

@jpkrohling @open-telemetry/collector-contrib-approvers can someone take a look?

@github-actions github-actions bot removed the Stale label Nov 16, 2024
@TylerHelmuth TylerHelmuth merged commit 830ffbf into open-telemetry:main Nov 18, 2024
160 checks passed
@github-actions github-actions bot added this to the next release milestone Nov 18, 2024
RutvikS-crest pushed a commit to RutvikS-crest/opentelemetry-collector-contrib that referenced this pull request Dec 9, 2024
…6024)

#### Description

I noticed that we don't close the exporters during shutdown. This PR
iterates through the exporters and closes them in a graceful manner.
sbylica-splunk pushed a commit to sbylica-splunk/opentelemetry-collector-contrib that referenced this pull request Dec 17, 2024
…6024)

#### Description

I noticed that we don't close the exporters during shutdown. This PR
iterates through the exporters and closes them in a graceful manner.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants