opentelemetry-js
opentelemetry-js copied to clipboard
ignoreMethods vs ignoreIncomingPaths and ignoreUrls vs ignoreOutgoingUrls
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
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.
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 */ }),
}
});
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
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;
...
}
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 ?
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.
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
Ref this issue https://github.com/open-telemetry/opentelemetry-js/issues/214
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
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.
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.
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>;
@naseemkullah you need to update your version of @types/node
Does not seem to help, only thing that helps if if i add dom
as a value for lib
in tsconfig
as per https://github.com/DefinitelyTyped/DefinitelyTyped/issues/19799 ... is that the right thing to do?
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.
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 https://github.com/DefinitelyTyped/DefinitelyTyped/pull/25356 and https://github.com/DefinitelyTyped/DefinitelyTyped/issues/25342
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
.
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.
enabled
is now the only config shared between all instrumentations