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

Make graceful shutdown optional #3577

Merged
merged 2 commits into from
Jul 13, 2021
Merged

Make graceful shutdown optional #3577

merged 2 commits into from
Jul 13, 2021

Conversation

rakyll
Copy link
Contributor

@rakyll rakyll commented Jul 8, 2021

service.Collector is handling the signal management and implementing a graceful shutdown. This is a problem for collectors that want to wire their own signal handling in cases they want to add custom logic and/or have resources other than the service.Collector to shutdown. Moving the graceful shutdown out of the service package to let the main programs do the wiring themselves.

Fixes #3490

@rakyll rakyll requested review from a team and bogdandrutu July 8, 2021 05:43
@rakyll
Copy link
Contributor Author

rakyll commented Jul 8, 2021

One cool fact about this PR is that it's partly written by GitHub Copilot.

Copy link
Member

@tigrannajaryan tigrannajaryan left a comment

Choose a reason for hiding this comment

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

This removes a functionality that was previously available to other distros. Other distros that needs this will have to reimplement it.

Instead of removing it can we make it optional or decouple it in some other way from service.Collector so that distros that don't need it are not forced to use it and distros that do need it can still continue using it?

@rakyll
Copy link
Contributor Author

rakyll commented Jul 8, 2021

Can we file some issues on the distros and make sure they are switching to the new model?

@tigrannajaryan
Copy link
Member

Can we file some issues on the distros and make sure they are switching to the new model?

We can, but what's the new model? Do we suggest that they copy runInteractive, runService and shutdownGracefully functions? It would be nicer if we offered the functionality in some more easily usable way to other distros (and to our own contrib, which is another distro in this context :-) ).

@rakyll
Copy link
Contributor Author

rakyll commented Jul 9, 2021

@tigrannajaryan A distro already needs to write the main program from scratch including the windows bits. Nonetheless, I'll introduce an option to the collector settings, so this is not a breaking change.

service.Collector is handling the signal management and implementing
a graceful shutdown. This is a problem for collectors that want to
wire their own signal handling in cases they want to add custom logic
and/or have resources other than the service.Collector to shutdown.
Making the automatic shutdown an optional behavior for users who
may need to disable it.

Fixes #3490
@rakyll rakyll changed the title Handle graceful shutdown in main Make graceful shutdown optional Jul 9, 2021
@rakyll
Copy link
Contributor Author

rakyll commented Jul 9, 2021

PTAL

@tigrannajaryan tigrannajaryan merged commit 56c7382 into open-telemetry:main Jul 13, 2021
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.

Allow service.Collector users to implement custom graceful shutdown
3 participants