fix auto instrumentation of loaded modules without an entrypoint defined

[BrunoBerisso]

The automatic instrumentation works well but enforces the API consumer to follow a fixed lifecycle on his side. The intention of these changes is to add means to instrument already supported modules manually at any point in time.

I believe this PR would close handle issue #560, at least for my case.

Thanks for maintaining this project.

Note: this is a WIP to validate the approach. If the owners think this make sense I will be happy to push the feature during the review process.

[rochdev]

I'm not sure I understand the difference between this approach and calling tracer.use() later in the lifecycle, which should work but is definitely not recommended.

[BrunoBerisso]

In my case, tracer.use() fail because getModules didn't return any result for Express 4.16.4

[rochdev]

Were you able to find the reason for this? I would rather have this fixed in tracer.use() if possible so that it covers more use cases.

[BrunoBerisso]

Yes. I think the reason is instrumentation doesn't have a file value set and rely on the project.json main field to match the module id.

The final result is that it doesn't find anything... If you think it's better to fix tracer.use() I will be happy to do it.

[rochdev]

Yes. I think the reason is instrumentation doesn't have a file value set and rely on the project.json main field to match the module id.

I assumed this would always be set but I guess it can default to index.js when not present.

The final result is that it doesn't find anything... If you think it's better to fix tracer.use() I will be happy to do it.

I would definitely prefer that to introducing a new method, at least for now. Thanks for looking into this!

[BrunoBerisso]

FYI: I just add index.js to instrumentation object for Express and I don't even need to call use() for the module to be patched. The initialization process gets the modules to patch from require.cache so any supported module already loaded will be processed.

I believe this load process has some important things to address (like checking for versions set on the instrumentation object before patching, etc) and I need this fixed to move forward with my integration.

Can I open another PR to set file: 'index.js' in the instrumentation for Express and then continue working on this one?

[rochdev]

Can I open another PR to set file: 'index.js' in the instrumentation for Express and then continue working on this one?

Sure, please do! I would however suggest adding a default to the instrumenter instead, so that any library without a main field in package.json would default to index.js like Node is doing.

[BrunoBerisso]

I would however suggest adding a default to the instrumenter instead

I did just what you suggested and works as expected on my side, thanks!
This branch is updated with the latest changes, I can't check the conflicts on src/instrumenter.js as it says here. I pull and rebase before pushing so not sure what's wrong...

Let me know if it looks good and if possible an eta to know when this could be available on my side. Thanks

[rochdev]

Could you add a test to avoid any regression in the future?

For the merge conflict, the issue is that we changed the structure of the project, so the file that should be changed is now packages/dd-trace/src/instrumenter.js.

[rochdev]

Let me know if it looks good and if possible an eta to know when this could be available on my side.

We don't have a well defined release cycle, but I plan to do a minor release this week. If that ends up not being possible I'll simply release a patch release instead.

[BrunoBerisso]

Thanks @rochdev Was a pleasure 👍