pragmaticivan / nestjs-otel

OpenTelemetry (Tracing + Metrics) module for Nest framework (node.js) 🔭
Apache License 2.0
565 stars 48 forks source link

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

Open jmarianski-kogifi opened 1 year ago

jmarianski-kogifi commented 1 year ago

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 commented 1 year ago

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

github-actions[bot] commented 6 days ago

This issue was marked stale. It will be closed in 30 days without additional activity.