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

SDK shutdown handling #127

Open
trentm opened this issue Apr 9, 2024 · 2 comments
Open

SDK shutdown handling #127

trentm opened this issue Apr 9, 2024 · 2 comments
Assignees

Comments

@trentm
Copy link
Member

trentm commented Apr 9, 2024

Shutdown TODO notes extracted from "start.js":

// TODO sdk shutdown: also SIGINT?
// TODO sdk shutdown handlers: better log on err
// TODO sdk shutdown: make these handlers configurable?
//  - If so, could move these into sdk.js and it could use its logger for logging an error.
//  - Note, only want luggite.warn over console.warn if errSerializer gets
//    all attributes (console.warn shows more data for ECONNREFUSED)
// TODO sdk shutdown beforeExit: skip this for Lambda (also Azure Fns?)
// TODO sdh shutdown: call process.exit?
// TODO: Whether we have a signal handler for sdk.shutdown() is debatable. It
// definitely *can* change program behaviour. Let's reconsider.
// Note: See https://github.com/open-telemetry/opentelemetry-js/issues/1521
// for some thoughts on automatic handling to shutdown the SDK.

These should all be worked through.

My current thoughts are:

  • We should have shutdown handling by default for our basic/default use case.
  • We should provide a configuration option to turn off the signal handler(s) and mention this in troubleshooting and/or in the advanced/details section of the user guide. This is because users can have legitimate other uses for SIGTERM, etc. and ours will conflict. The docs for this option should show how a user using the golden path or a custom "instrumentation.js" can setup shutdown signal handlers.
  • We should add an equivalent handler for SIGINT and/or document why not.
  • We should document limitations on Windows and have separate later work to support controlled shutdown on Windows. Note (https://nodejs.org/api/process.html#signal-events) that SIGTERM and SIGINT are not supported on Windows. There is SIGBREAK, but I'm not sure of the default handler behaviour there.
  • Multiple shutdowns: When a MeterProvider is involved (and possibly in other cases), shutdown adds async tasks, so we get beforeExit again later. That results in calling sdk.shutdown() twice. Currently that isn't problematic (there may be a diag warn about this?), but we should be able to do better with a module-level global to only attempt to shutdown once.
  • The logging-related issues for shutdown handling and perhaps the Windows-related issues should be moved to a separate issue for later work.
@trentm
Copy link
Member Author

trentm commented Apr 9, 2024

Q: do we get 'beforeExit' before termination due to a signal? I'm guessing not. If we did then we could just have 'beforeExit'

@trentm
Copy link
Member Author

trentm commented Dec 5, 2024

Separate thing:

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

No branches or pull requests

1 participant