Add Plugin interface and BasePlugin class

[mayurkale22]

Idea: The automatic-tracer would use this interface to automatically patch list of configured/provided modules i.e. Every instrumentation plugin would implement this interface.

Something like this.

// @example
export class RedisPlugin extends Plugin {
    constructor() {}
    enable<T>(moduleExports: T, tracer: Tracer): T {}
    disable(): void {}
}

Let me know if this makes sense or suggest an alternative.

[codecov-io]

Codecov Report

Merging #110 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #110   +/-   ##
=======================================
  Coverage   98.74%   98.74%           
=======================================
  Files          29       29           
  Lines        1915     1915           
  Branches      221      221           
=======================================
  Hits         1891     1891           
  Misses         24       24

[draffensperger]

Could you give an example of this?

[mayurkale22]

High level stuff, something like this:

const DEFAULT_INSTRUMENTATION_MODULES = ['http', 'https', ...];
export class AutomaticTracer implements types.Tracer {
  constructor(){
    this.loadPlugins(DEFAULT_INSTRUMENTATION_MODULES);
  }

  loadPlugins(modulesToPatch: string[]) {
    // load plugin using its instrumentation plugin package name ie. opentelemetry-instrumentation-http.
    // Each plugin module should implement the Plugin interface and export an instance named as "plugin".
    const plugin: Plugin = require(moduleName as string).plugin;

    // Hook into the modules
    Hook(modulesToPatch, function (exports, name, basedir) {
      return plugin.enable(exports, tracer);
    }
  }
}

* Name can be either AutomaticTracer or just Tracer or something else.

[draffensperger]

Thanks - I actually just meant, what would be an example value for "moduleExports"? And would it make sense to add that to the JSDoc comment for the param?

[vmarchaud]

I don't think it can be typed since it's what the patched module export as object

[draffensperger]

Yes, makes sense on not giving it a strict type - I mostly just wanted to understand how the parameter is used and the examples clarify that!

[rochdev]

Since all plugins will be called in the same way by some sort of instrumenter, should there be a method/parameter to configure the plugin? Having that in the constructor would mean we can't validate it from the interface.

[mayurkale22]

Sure. Can we add another optional param (config)? What are the possible attributes for config? ex: version, default attributes like these?

[rochdev]

I think config is basically just an object and the possible properties depend on each plugin.

[rochdev]

One other thing that seems to be missing too is a way to define the supported versions/modules on every plugin in a standard way.

[mayurkale22]

enable(moduleExports: T, tracer: Tracer, 
       version: string, config?: {[key: unknown]: unknown}): T;

WDYT?

[rochdev]

config?: {[key: unknown]: unknown}

I think it's safe to assume keys will be strings, especially if it's an object since other types are not possible.

version: string

I think maybe the module name and version filters should be protected properties on the base tracer that the implementations can set. I don't think these should be set by the caller.

[mayurkale22]

Added the config object. ptal

[rochdev]

Small nit, but otherwise LGTM. I also thought a bit more about the config and maybe it could be passed to the constructor instead, but that can be reevaluated later.

[draffensperger]

I asked this question on anther PR, but asking it here again for completeness: does it make sense for BasePlugin to be part of opentelemetry-core given that it's a Node platform specific concept?

[mayurkale22]

I am just afraid about circular dependency if we move BasePlugin in Node-Tracer package because any instrumented plugin (ex: package/plugin-http) needs to extends BasePlugin from Node-Tracer and add same package in Node-Tracer for enabling the patching.

[draffensperger]

Ah, makes sense! Yeah, I think it's fine then.

[rochdev]

If it's Node specific, then IMO it definitely shouldn't be in core. Maybe we need a new package to store this? Otherwise, any way we can make sure that the base class is not Node specific?

[draffensperger]

Well, the whole concept of a Plugin as a runtime thing is somewhat Node specific. A couple options:

• Have a package like node-core that contains Node-specific shared functionality. There could also be a web-core package that has web specific utilities in the future.
• Just make Plugin an interface and move it to the types package - we lose the little bit of reusable code that it provides, but this isn't hard to duplicate for each plugin.

[rochdev]

My only worry with a compile-time approach is that there are a lot of different bundlers out there, so we can't assume everyone uses Webpack. There are also a lot of apps and libraries out there that do not require compilation at all.

[draffensperger]

Yes, I agree. The challenge with the runtime approach for patching something like a framework is that the build process will have renamed properties in the framework and/or inlined functions (or even whole classes!). So you can't really know how to monkey-patch a framework at runtime.

Our current approach in OpenCensus Web is to only patch the core web APIs, which always have defined names at runtime, and as a plus side will work no matter how you compile your JS and no matter what web framework you use :)

[draffensperger]

But to come back to why I wonder if the Plugin class as defined may fit well for the web, those core browser APIs don't have any notion of require or modules, they are just globals. (There is some module work in Chrome and new browsers for some new things, but most are still globals)

We could still have Plugin represent the patch/unpatch methods but they would be patching globals not a module object.

[rochdev]

the build process will have renamed properties in the framework

True, but many frameworks expose some sort of global object that is not renamed (otherwise it couldn't be interacted with). I think maybe a good compromise would be having compile-time requirements that will allow the plugins to work at runtime. Then it would be possible to handle the requirements automatically with a Webpack loader, but if the user is not using Webpack they can still do it manually. So basically at compile time, anything needed by the plugin would simply get exposed, and then available at runtime for the plugin to do its magic. That approach would make it a lot easier for users to do the compile-time step manually if needed.

We could still have Plugin represent the patch/unpatch methods but they would be patching globals not a module object.

I think that's a fair assumption.

[draffensperger]

Those are cool ideas! Definitely worth exploring more once we get deeper into the browser side of this.