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

Tutorial section provides wrong suggestions (for Kubernetes cluster) #440

Open
jmarianski-kogifi opened this issue Jun 28, 2023 · 1 comment

Comments

@jmarianski-kogifi
Copy link

jmarianski-kogifi commented Jun 28, 2023

While debugging my application I noticed that requests in kubernetes cluster in my nest.js tend to get terminated before they finish processing (when receiving SIGTERM). So I started digging, if the enableShutdownHooks option is there, of if the resolvers are covered by request shutdown protection, etc. My conclusion on the subject was, that there were no errors on my part.

However, I noticed when I removed the section relating to SIGTERM, it started working correctly, the requests stopped being terminated prematurely.

process.on('SIGTERM', () => {
  otelSDK
    .shutdown()
    .then(
      () => console.log('SDK shut down successfully'),
      err => console.log('Error shutting down SDK', err)
    )
    .finally(() => process.exit(0));
});

Perhaps README for this repo should be changed to denote that this section might cause unnecessary side effects? Or perhaps it should be rewritten to instead implement this node service into the application lifecycle?

The most important part of this code excerpt was the process.exit, as that was killing the application. Perhaps it's not even necessary to be here? Especially considering that SIGTERM will affect nest.js on it's own, so process will exit naturally...? Or is there another reason?


When fiddling with attributes I managed to find the best combination for me. Still don't know if it's the best solution for everyone, as not everyone wants to enable shutdown hooks or something else.

As in tutorial we need to register otelSDK in main.ts

    await app.listen(config.port);
    try {
      if (config.exportTracing) {
        await otelSDK.start();
      }
    } catch (e) {
      logger.error(
        "Otel sdk startup error. Most likely related to webpack hot reloading. It should still work fine.",
      );
      logger.error(e);
    }
    if (module.hot) {
      module.hot.accept();
      module.hot.dispose(async () => {
        await app.close();
      });
    }

And now we need to deregister in AppModule

export class AppModule implements OnApplicationShutdown {
  async onApplicationShutdown(signal?: string) {
    this.logger.log({ message: `Shutting down, signal: ${signal}`, signal });
    if (config.exportTracing) {
      await new Promise<void>((resolve) => {
        otelSDK
          .shutdown()
          .then(() => this.logger.log("Tracing terminated"))
          .catch((e) => this.logger.error(e))
          .finally(() => resolve());
      });
    }
  }
}

And voila! application closes properly. I always had issues with hot reloading so don't know the expected state, however right now for me it works good enough to go back to developing.

PS. Sorry for the initial draft of the introduction there, was a little bit focused on finding a solution that I totally forgot my writing skills.

@pragmaticivan
Copy link
Owner

@jmarianski-kogifi I think the OnApplicationShutdown makes sense but I don't know if it's appropriate to start the SDK after you start to listen for the application because otel is supposed to monkeypatch all libraries before nestjs starts.

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

2 participants