[plugins] modules loaded before tracer SDK

[mayurkale22]

IMO initialization of NodeTracer module which provides automated tracing (instrumentation) for Node.js applications has to be the first line in the application code before loading actual modules like http, redis, grpc etc. Loading these modules before our tracer SDK may prevent
us from monkeypatching those modules for automatic tracing. I have verified that w/ current http example.

I am proposing to detect list of modules that were loaded using require() before our tracer SDK and log a warning or error to inform the users.

[dyladan]

Probably only warn on modules that would have been instrumented. for instance if http tracer is disabled, it doesn't matter that it was loaded before us.

[vmarchaud]

Totally agree (#654), it's already implemented in all vendor specific APMs because it's such a pain to debug for end users.

[naseemkullah]

Can you please confirm that http needs to be loaded after tracer initialization, or that simply the http server must be instantiated after (but the http module could be loaded before) ? For express the latter is true and I would presume the same for all libs.

[naseemkullah]

See: https://github.com/naseemkullah/k8s-job-dispatcher/blob/d9edefbdd29764ee414de9aa7333cec7d66b6254/server.js#L1-L2 and https://github.com/naseemkullah/k8s-job-dispatcher/blob/d9edefbdd29764ee414de9aa7333cec7d66b6254/server.js#L7

[vmarchaud]

@naseemkullah Nope, the tracer should always be required before any module. In your example, the patch still works because we patch the prototype of the http server which works for already instanciated server, however sometimes it's not possible.

[naseemkullah]

Thanks @vmarchaud

[OlivierAlbertini]

@naseemkullah Nope, the tracer should always be required before any module.

I'm agree with @vmarchaud. It's a good rule. I also encountered some issues as well during integrations with http if tracer was initialized after...

[mayurkale22]

I think we close this one via #654. @vmarchaud please confirm.

[vmarchaud]

@mayurkale22 indeed its done