Merge user provided plugin configs with default supported configs

[mayurkale22]

Currently, we allow users to config the plugins as per their requirement (which is great), but newly provided config replaces the existing default config instead of merging with it.

For example: If i decided to disable certain plugin or change its config, I have to update that plugin and add all other plugins to activate them.

const tracer = new NodeTracer({
  plugins: { 
    dns: { enabled: false },
    http: { enabled: true, path: ... },
    mysql: { enabled: true, path: ... },
    ...
   }
});

Expected behavior: Update only specific plugin and others should work seamlessly.

const tracer = new NodeTracer({
  plugins: { 
    dns: { path: '<new path>' },
   }
});

Code reference at:

opentelemetry-js/packages/opentelemetry-node/src/NodeTracer.ts

Line 39 in d32565a

this._pluginLoader.load(config.plugins || DEFAULT_INSTRUMENTATION_PLUGINS);

[dyladan]

I think at the same time we should define a configuration interface for configuring behavior of plugins.

Right now this is our plugin configuration interface

export interface PluginConfig {
    /**
     * Whether to enable the plugin.
     * @default true
     */
    enabled?: boolean;
    /**
     * Path of the trace plugin to load.
     * @default '@opentelemetry/plugin-http' in case of http.
     */
    path?: string;
    /**
     * Request methods that match any string in ignoreMethods will not be traced.
     */
    ignoreMethods?: string[];
    /**
     * URLs that partially match any regex in ignoreUrls will not be traced.
     * In addition, URLs that are _exact matches_ of strings in ignoreUrls will
     * also not be traced.
     */
    ignoreUrls?: Array<string | RegExp>;
    /**
     * List of internal files that need patch and are not exported by
     * default.
     */
    internalFilesExports?: PluginInternalFiles;
    /**
     * If true, additional information about query parameters and
     * results will be attached (as `attributes`) to spans representing
     * database operations.
     */
    enhancedDatabaseReporting?: boolean;
}

• ignoreMethods
  • unused
  • definition of "method" is not clear
• ignoreUrls
  • used by xhr
  • other plugins that could benefit from this but don't
• internalFilesExports
  • used by grpc, but not configured by user. this set by the plugin loader
  • should not be a user-visible configuration
• enhancedDatabaseReporting
  • unused
  • not well defined behavior

I am proposing to separate user-configurable options and internal configuration similar to the following:

const tracer = new NodeTracer({
  plugins: { 
    dns: { options: { someconfig: true } },
   },
   sharedPluginOptions: {
      someotherconfig: true,
   }
});

Additionally, I think we should better define and document the configuration options we have that could be useful for other plugins.

[OlivierAlbertini]

I like the idea of

sharedPluginOptions: {
      someotherconfig: true,
   }

[dyladan]

We should look at how other sigs are configuring plugins. Ruby has some way to provide a hash of options directly in the gemfile (similar to package.json), and I know some js packages allow configuration inside the package.json. Maybe this is a good way for us to go.

[naseemkullah]

For now can we dial back the scope and just merge user defined config with the default config? as far as having default values for enabled and path remain untouched so one could simply:

  plugins: {
    http: {
      ignoreIncomingPaths: [/\/healthz/],
    },
  },

rather than:

  plugins: {
    http: {
      enabled: true,
      ignoreIncomingPaths: [/\/healthz/],
      path: '@opentelemetry/plugin-http',
    },
    mysql: { enabled: true, path: '@opentelemetry/plugin-mysql' },
    redis: { enabled: true, path: '@opentelemetry/plugin-redis' },
  },

[dyladan]

Yes I think this is a good idea. If you want to make a PR that would be great, or else I can do it when I have time.

[naseemkullah]

Yes I think this is a good idea. If you want to make a PR that would be great, or else I can do it when I have time.

Sure thing! I'll submit a PR.