Feat/plugin injection

[richardcornelissen]

Which problem is this PR solving?

Load resolved plugins instead of dynamically importing them.
We're using webpack to pack microservices, which does not include plugins if they're dynamically imported.

Short description of the changes

• Add optional plugin property to PluginConfig in @opentelemetry/api
• Let PluginLoader load the resolved plugin instead of importing it in @opentelemetry/node
• Users can now perform the following:

import {plugin as httpsPlugin} from '@opentelemetry/plugin-https';

const provider = new NodeTracerProvider({
  plugins: {
    https: {
      plugin: httpsPlugin,
    },
  },
});

[linux-foundation-easycla]

The committers are authorized under a signed CLA.

• ✅ Richard (37cdebd, e11e906)

[codecov]

Codecov Report

Merging #1704 (394e8e3) into master (471306f) will decrease coverage by 0.10%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1704      +/-   ##
==========================================
- Coverage   92.36%   92.26%   -0.11%     
==========================================
  Files         157      159       +2     
  Lines        5108     5195      +87     
  Branches     1091     1110      +19     
==========================================
+ Hits         4718     4793      +75     
- Misses        390      402      +12     

Impacted Files	Coverage Δ
...telemetry-node/src/instrumentation/PluginLoader.ts	93.50% <100.00%> (+0.17%)	⬆️
...ry-context-zone-peer-dep/src/ZoneContextManager.ts	85.18% <0.00%> (ø)
...es/opentelemetry-context-zone-peer-dep/src/util.ts	100.00% <0.00%> (ø)

[dyladan]

I like this idea, but this should also be fixed when all plugins are moved to the new instrumentation class

[obecny]

It looks fine, although I'm wondering if we really want this now, it all depends how urgent is this ?. We are going to implement soon something like this #1672 and then the PluginLoader is going to be deprecated then.
And if I understand correctly then your case should also be covered by this ?
I'm fine with having this solution for the time being, just want to point this out to avoid confusion later that this is going to be deprecated and will require a bit of refactoring from the user anyway.

[vmarchaud]

lgtm, i agree with others that this is only temporary

[obecny]

Just realised one thing the change is in PluginConfig in API not sdk, after GA it will stay there quite long. Let's discuss it on tomorrow JS SIG

[dyladan]

I think we should consider moving all plugin loading and config out of the API if possible. AFAIK it's just the interface so it shouldn't be a problem.

[richardcornelissen]

Hi all, thanks for your comments and looking into this PR.

After some more progress, I would like to add the following points that might help in your discussion:

• In our use case (webpack bundling of an AWS Lambda function), this change only works for built-in modules (i.e. http and https) due to the way PluginLoader uses require-in-the-middle to hook module requires. For example, mysql is embedded in the bundle, meaning it's not present in node_modules.
• There is a workaround for require-in-the-middle with Webpack described here. However, due to the PluginLoader requiring a package.json and a package version (see utils.ts#getPackageVersion), which aren't present in a bundle.
• We've tried to use opentelemetry-plugin-aws-sdk to instrument aws-sdk (which is already present on lambda runtime), but requiring /var/runtime/node_modules/aws-sdk/package.json fails (Error: Cannot find module '/var/runtime/node_modules/aws-sdk/package.json').

Hope this also helps with any further discussion on improving the instrumentation.

[Flarna]

The sequence of when and how a plugin is loaded/started will not solve the webpack related problems.

The version for modules to instrument is needed as the API and as a result the instrumentation points may change heavily between versions.
I think a better way forward would be to enhance webpack to be more friendly to APM tooling which relies on require-in-the-middle and similar solutions.

[dyladan]

The sequence of when and how a plugin is loaded/started will not solve the webpack related problems.

This isn't about changing the sequence. It means the app developer can include the plugins they need as import statements, so they are ensured to be included in the webpack bundle. In order for them to work, you still need to declare the dependencies to be patched as externals. There are popular modules to do this automatically such as https://www.npmjs.com/package/webpack-node-externals

[dyladan]

We talked about this in the SIG meeting and we would like to move all plugin related interfaces out of the API module and into the core module which is where the BasePlugin class resides. That way, it is not covered by the RC LTS guarantees and can be deprecated when the Instrumentation migration is complete without violating the API LTS agreement.

[obecny]

We talked about this in the SIG meeting and we would like to move all plugin related interfaces out of the API module and into the core module which is where the BasePlugin class resides. That way, it is not covered by the RC LTS guarantees and can be deprecated when the Instrumentation migration is complete without violating the API LTS agreement.

#1715

[obecny]

@richardcornelissen pls fix the conflicts

[dyladan]

@richardcornelissen please fix conflicts or we cannot merge

[richardcornelissen]

@dyladan @obecny Sorry for the wait, conflicts have been resolved

[dyladan]

@richardcornelissen please update the branch or allow writes from maintainers

[dyladan]

@richardcornelissen this PR cannot be merged unless it is up to date with the master branch.

[vmarchaud]

Plugins have been removed in favor of instrumantion so i'll close this