Initialisation order with express initialisation

[annettejanewilson]

Hi! It's not clear from the documentation, but from experiment I believe that it's necessary to initialise opentelemetry, or at least to register the express instrumentation, before calling any function in express, is that true? I.e. it's not enough merely to have registered the express instrumentation before calling app.listen() but it has to have happened before any call to app.get(), app.use() or creating an instance of Router. I'm asking because those calls - at least in the code I'm working with - generally happen at require/import time because they are expected to be side-effect free. I think instead this code now needs to be treated as having side-effects and to be exported as an initialisation function to allow it to be explicitly ordered after opentelemetry initialisation.

If I'm right, would you be amenable to a PR to add this information to https://github.com/open-telemetry/opentelemetry-js-contrib/blob/main/plugins/node/opentelemetry-instrumentation-express/README.md ? Or is there a better place to document this requirement? I'll admit I find it hard to know where to look for guidance on using opentelemetry-js compared to, e.g. opentelemetry-python.

[Flarna]

The instrumentation needs to be created/registered before require('express').

[annettejanewilson]

Ah, wow, that's pretty restrictive. Especially given that initialisation is asynchronous, it means pretty substantial (and I would say surprising) constraints on how the application is structured, right? Initialising opentelemetry can't be just another part of application initialisation, it has to go into some bootstrap section that doesn't even require the rest of the application code until after waiting for asynchronous initialisation to complete. That's right, yes? Assuming that we're using opentelemetry.NodeSDK as in the Getting Started guide.

I guess I have two problems here - one is that I think this is surprising and not adequately documented, although I would be interested to help once I better understand it. The other is that while I can certainly make it work, it feels like pretty unnatural contortions of the startup code. I need to provide guidelines to my colleagues for installing this in a great many services and I'm worried that this breaks a lot of assumptions about what a normal orderly startup looks like, and is going to need handling as a special case for each service. (E.g. assumptions like "requires can happen at the root level", "requires don't have side effects", "configuration is passed explicitly")

Now that I look again more closely I'm actually not even clear on how the Getting Started guide works - it appears to use node --require tracing.js ... to initialise tracing before running the application itself, but it does so by invoking sdk.start() which is asynchronous, and there doesn't appear to be any mechanism to wait for that to complete before invoking require('express') or app.listen(...). I feel like I'm missing something about how this is all intended to come together. Any insight would be greatly appreciated.

[vmarchaud]

Ah, wow, that's pretty restrictive. [...]
The other is that while I can certainly make it work, it feels like pretty unnatural contortions of the startup code

Currently its the only reliable way to automatically instrument code, there is effort to try to make it baked into libraries themselves but i don't think we'll avoid this requirement in the near future.

Especially given that initialisation is asynchronous, it means pretty substantial (and I would say surprising) constraints on how the application is structured, right?

SDK initialisation is async because of resource detectors (which can fetch metadata from network like in cloud providers) but you don't need it to be done before starting your applications. You only need to import instrumentations before importing your code, thats why in all examples we use -r tracing.js

I guess I have two problems here - one is that I think this is surprising and not adequately documented

If you have any idea how to make it more clear (and have time to do so) we would gladfully accept any PR that improve it !

[annettejanewilson]

Ahhh! Okay, this wasn't clear either. It's good to know that sdk.start() guarantees to do this part of the initialisation synchronously.

I think the things I would want to know from the documentation are:

• It's necessary to have registered this instrumentation before any code calls require('express')
• By the time sdk.start() returns it is guaranteed that the provided instrumentations have been registered.

I can submit a PR to add the first of those to opentelemetry-instrumentation-express/README.md but I'm not sure where's a good place to record/discover the other. There's rudimentary TypeDoc documentation in the code, is expanding that the way to go? Is that something you would like to see become more thorough?

[vmarchaud]

I can submit a PR to add the first of those to opentelemetry-instrumentation-express/README.md but I'm not sure where's a good place to record/discover the other

I mean both are general behavior for all instrumentations (not only express), i think it may be better to have it explained in a more general place like the getting started guide, inside the main readme or maybe in the sdk readme ?

[KonstantinShemyak]

The limitation has been documented in #3786.