ignoreMethods vs ignoreIncomingPaths and ignoreUrls vs ignoreOutgoingUrls

[mayurkale22]

This is related to #584

The PluginConfig interface has ignoreMethods and ignoreUrls attributes to ignore the traces on provided methods and urls but we are not respecting these values (especially in HTTP plugin) instead HTTP plugin has its own options (https://github.com/open-telemetry/opentelemetry-js/tree/master/packages/opentelemetry-plugin-http#http-plugin-options) - ignoreIncomingPaths and ignoreOutgoingUrls.

I think we should consolidate them in one place and update the HTTP example to show the proper usage.

/cc @OlivierAlbertini

[dyladan]

I think we should allow plugins to have their own config options.

const tracer = new NodeTracer({
  plugins: {
    http: {
      enabled: true,
      path: '@opentelemetry/plugin-http',
      config: {
        ignoreIncomingPaths: [/\/healthz/],
      },
    },
  },
});

Otherwise we will have no way to allow unofficial plugins to have configs and still be typescript friendly

edit: and we will be constantly populating the PluginConfig type with options that most plugins just ignore.

[dyladan]

As another option we could allow fully configured plugins to be passed in:

import {HttpPlugin} from '@opentelemetry/plugin-http';

const tracer = new NodeTracer({
  plugins: {
    http: {
      enabled: true,
      module: new HttpPlugin({ /* config here */ }),
  }
});

[OlivierAlbertini]

In Opencensus (and in OpenTelemetry)

• ignoreIncomingPaths is relative [//healthz/] and ["/healthz"] would work
• ignoreOutgoingUrls is the full url so ["/healthz"] won't work.

It's more convenient to have path for ignoreIncomingPaths

I believe "ignoreMethods and ignoreUrls" come from a discussion where we wanted to have one config for all plugins and we never cleaned up after the SIG discussion

[mayurkale22]

Maybe something like this?

export interface PluginConfig {
  enabled?: boolean;
  path?: string;
  config?: HTTPConfig | gRPCConfig | DbConfig
}

export interface HTTPConfig {
  ignoreIncomingPaths?: IgnoreMatcher[];
  ignoreOutgoingUrls?: IgnoreMatcher[];
  applyCustomAttributesOnSpan?: HttpCustomAttributeFunction;
  ...
}

export interface gRPCConfig {
  internalFilesExports?: PluginInternalFiles;
  ...
}

export interface DbConfig {
  enhancedDatabaseReporting?: boolean;
  ...
}

[OlivierAlbertini]

Maybe something like this?

export interface PluginConfig {
  enabled?: boolean;
  path?: string;
  config?: HTTPConfig | gRPCConfig | DbConfig
}

export interface HTTPConfig {
  ignoreIncomingPaths?: IgnoreMatcher[];
  ignoreOutgoingUrls?: IgnoreMatcher[];
  applyCustomAttributesOnSpan?: HttpCustomAttributeFunction;
  ...
}

export interface gRPCConfig {
  internalFilesExports?: PluginInternalFiles;
  ...
}

export interface DbConfig {
  enhancedDatabaseReporting?: boolean;
  ...
}

Looks good to me. Perhaps, we could discuss at the tomorrow SIG ?

[Flarna]

I think we should still allow configuration of plugins not hosted in this repo. By listing the supported configs in PluginConfig developers of some plugin have to request changes in the pubic OTel API or use casts to avoid typescript issues.

[dyladan]

I was thinking something more along the lines of this:

// packages/opentelemetry-types/src/trace/instrumentation/Plugin.ts
export interface PluginConfig<Config = any> {
  enabled?: boolean;
  path?: string;
  config?: Config;
}

// packages/opentelemetry-node/src/instrumentation/PluginLoader.ts
import {HttpPluginConfig} from "@opentelemetry/plugin-http;
import {MySQLPluginConfig} from "@opentelemetry/plugin-mysql;

export interface Plugins {
  http?: PluginConfig<HttpPluginConfig>;
  mysql?: PluginConfig<MySQLPluginConfig>;
  // ... all other default modules here
  [module: string]: PluginConfig | undefined;
}

The compiler seems to like this solution too

Suggesting plugins to use

Suggesting plugin configs

Allowing custom plugins with configs

[OlivierAlbertini]

Ref this issue #214

[dyladan]

Ref this issue #214

I read through the issue. A couple times the idea of plugin-specific config options were brought up but it was never really refuted or encouraged, just dropped. Similarly in the associated PR it was never really discussed.

As was brought up in the SIG, my suggested option above has the potential to require unwanted dependencies in the node and/or web packages which is not desirable, as well as create potential for circular dependencies. Maybe it is possible to move the plugin config interfaces into the types package in order to remove that dependency? This removes any possible issues with circular dependencies anyways since types does not depend on any packages.

@obecny @OlivierAlbertini

[OlivierAlbertini]

Maybe it is possible to move the plugin config interfaces into the types package in order to remove that dependency?

I'm agree.
https://github.com/open-telemetry/opentelemetry-js/blob/master/packages/opentelemetry-plugin-http/src/types.ts#L60
It's important that developers add their own plugins that is not yet in this repo. e.g
As a developer, I created a "plugin-amqp" and I want to use this plugin with the standard sdk.
As a developer, I created another "plugin-http" with more configurations and I want to integrate this plugin with the standard sdk.

[pauldraper]

FYI, http plugin configuration should really just accept a generic filter hook.

Instead of

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

use

{
  incomingFilter: req => !/\/healthz/.test(req.url),
}

Filtering could depend on URL, method. It could use equality, includes, regex, something else.

I agree with "fully configured plugin" approach.

[naseemkullah]

Any update on this?

FYI I tried:

import { HttpPluginConfig } from '@opentelemetry/plugin-http';

const http: HttpPluginConfig = {
  enabled: true,
  path: '@opentelemetry/plugin-http',
  ignoreIncomingPaths: [/\/healthz/],
};

const provider = new NodeTracerProvider({
  logLevel: LogLevel.ERROR,
  plugins: {
    http,
  },
  sampler: new ProbabilitySampler(0.5),
});

but got

node_modules/@opentelemetry/plugin-http/build/src/http.d.ts:44:95 - error TS2304: Cannot find name 'URL'.

44     protected _getPatchOutgoingGetFunction(clientRequest: (options: RequestOptions | string | URL, ...args: HttpRequestArgs) => ClientRequest): (original: Func<ClientRequest>) => Func<ClientRequest>;

[dyladan]

@naseemkullah you need to update your version of @types/node

[naseemkullah]

Does not seem to help, only thing that helps if if i add dom as a value for lib in tsconfig as per DefinitelyTyped/DefinitelyTyped#19799 ... is that the right thing to do?

[dyladan]

I don't think so, because that just means it's attempting to use the global URL which is available in browsers but not node. I'll look into this.

That is a separate issue to this one though.

[Flarna]

Global URL is available in Node but it's missing in type definitions. Reason is that the URL object is slightly different between node and DOM. Therefore adding the node variant at global scope for node breaks all projects which have dom in lib - and this are a lot projects.

see DefinitelyTyped/DefinitelyTyped#25356 and DefinitelyTyped/DefinitelyTyped#25342

[dyladan]

Looks like this is just a typing issue anyways https://github.com/open-telemetry/opentelemetry-js/blob/master/packages/opentelemetry-plugin-http/src/http.ts#L148. The http plugin is a node-only plugin so we should just make the type explicitly use the URL from the url module.

This should be as simple as changing all instances of URL to url.URL.

[Flarna]

Maybe the projects should be configured more strict. e.g. plugin-http is node only therefore lib dom should be not needed/used at all. As there is no lib config in tsconfig.json of the plugin-http nor in tsconfig.base.json typescript includes dom on default.

[dyladan]

enabled is now the only config shared between all instrumentations