chore: more advanced plugin configuration #628

dyladan · 2019-12-16T19:53:44Z

Which problem is this PR solving?

Merge user provided plugin configs with default supported configs #616

Short description of the changes

NodeTracer configs can now be configured in the following ways:

Default configuration and plugins

const tracer = new NodeTracer()

Configure a plugin with options

In this scenario, the http plugin will receive the httpPluginOption configuration.

new NodeTracer({
  plugins: {
    http: {
      enabled: true,
      path: "",
      options: {
        httpPluginOption: "value",
      }
    }
  }
});

Configure shared plugin options

In this scenario, the http plugin will receive the httpPluginOption configuration, the https plugin will receive the httpsPluginOption configuration, and both will receive the sharedPluginOption configuration.

new NodeTracer({
  plugins: {
    http: {
      enabled: true,
      path: "",
      options: {
        httpPluginOption: "value",
      }
    },
    https: {
      enabled: true,
      path: "",
      options: {
        httpsPluginOption: "value",
      }
    }
  },
  sharedPluginOptions: {
    sharedPluginOption: "value",
  }
});

Disable a plugin and leave all others enabled

new NodeTracer({
  http: { enabled: false }
});

You no longer need to explicitly give the whole configuration object

new NodeTracer({
  http: { 
    // no need for `enabled` or `path` if the defaults are used
    options: { configOption: 1 }
  },
})

codecov-io · 2019-12-16T20:19:28Z

Codecov Report

Merging #628 into master will increase coverage by 1.74%.
The diff coverage is 87.43%.

@@            Coverage Diff             @@
##           master     #628      +/-   ##
==========================================
+ Coverage    89.6%   91.35%   +1.74%     
==========================================
  Files         191      215      +24     
  Lines       10034     9932     -102     
  Branches      925      913      -12     
==========================================
+ Hits         8991     9073      +82     
+ Misses       1043      859     -184

Impacted Files	Coverage Δ
...telemetry-plugin-document-load/src/documentLoad.ts	`97.91% <ø> (-0.03%)`	⬇️
packages/opentelemetry-plugin-grpc/src/grpc.ts	`96.23% <ø> (-0.03%)`	⬇️
...-plugin-postgres/opentelemetry-plugin-pg/src/pg.ts	`91.22% <ø> (-0.16%)`	⬇️
packages/opentelemetry-base/src/version.ts	`0% <ø> (ø)`
...ges/opentelemetry-scope-async-hooks/src/version.ts	`0% <0%> (ø)`
packages/opentelemetry-plugin-https/src/version.ts	`0% <0%> (ø)`
packages/opentelemetry-plugin-dns/src/version.ts	`0% <0%> (ø)`
...kages/opentelemetry-exporter-jaeger/src/version.ts	`0% <0%> (ø)`
...s/opentelemetry-plugin-mongodb-core/src/version.ts	`0% <0%> (ø)`
packages/opentelemetry-plugin-http/src/version.ts	`0% <0%> (ø)`
... and 93 more

mayurkale22 · 2019-12-17T05:45:09Z

This is a great start. What are the valid use-cases of sharedPluginOptions? Currrently, looks like enhancedDatabaseReporting (database specific) and ignoreUrls (http/https specific) options are part of sharedPluginOptions.

dyladan · 2019-12-17T14:11:53Z

This is a great start. What are the valid use-cases of sharedPluginOptions? Currrently, looks like enhancedDatabaseReporting (database specific) and ignoreUrls (http/https specific) options are part of sharedPluginOptions.

Right now the plugin options are the same regardless of if they're shared or if they're set on a single plugin, and only a couple exist. I was going to go through all plugins today and look at which configurations they use and create a set of options interfaces in types that generalizes them like DatabasePluginOptions which would have things like enhanced db reporting and RPCPluginOptions which would have things like ignore urls. This prevents the types interfaces from relying on plugins and instead plugins would just use the configuration interfaces exported from types.

dyladan · 2019-12-17T23:23:25Z

@mayurkale22 one use case of shared configurations might be to set enhanced db reporting true for all db modules (mysql, pg, pg-pool, redis).

new NodeTracer({
  sharedPluginOptions: {
    enhancedDatabaseReporting: true,
  }
});

You may also want to set it true in general, but then disable for a single module which is also possible.

new NodeTracer({
  plugins: {
    redis: {
      options: {
        enhancedDatabaseReporting: false,
      },
    },
  },
  sharedPluginOptions: {
    enhancedDatabaseReporting: true,
  },
});

mayurkale22 · 2019-12-18T00:28:17Z

I see, let's discuss this in tomorrow's SIG meeting.

examples/dns/setup.js

obecny · 2019-12-18T19:14:23Z

should we have some rules about naming params to avoid some problems with namespacing later? For example
enhancedDatabaseReporting maybe it should be db.enhancedReporting etc.
'ignoreUrls->http.ignoreUrls`

or maybe if param can be shared among all plugins it should start always with some prefix for example sharedEnhancedDatabaseReporting or shared.EnhancedDatabaseReporting.
Basically we will have more and more plugins and in theory it may happen that the same param can means different thing for different plugins. To avoid such situation I would strongly encourage to add something to the name so we can safely indicate that param can be shared.

Another thing is that the shared params should be immutable so the plugins won't be able to overwrite it (Object.freeze)

dyladan · 2019-12-18T19:21:35Z

@obecny good point. maybe we're overcomplicating it. instead maybe we should just have a single configuration object shared to all plugins?

new NodeTracer({
  options: {
    http: {
      ignoreUrls: ["localhost"],
    },
    db: {
      enhancedDatabaseReporting: true,
    }
  },
});

Then the whole options object is frozen with Object.freeze and sent to all plugins. So the dns plugin can look into the http options and see which urls are ignored. All db related plugins like mysql and pg can look into the db options object and so on.

obecny · 2019-12-18T19:29:33Z

@obecny good point. maybe we're overcomplicating it. instead maybe we should just have a single configuration object shared to all plugins?
new NodeTracer({
  options: {
    http: {
      ignoreUrls: ["localhost"],
    },
    db: {
      enhancedDatabaseReporting: true,
    }
  },
});
Then the whole options object is frozen with Object.freeze and sent to all plugins. So the dns plugin can look into the http options and see which urls are ignored. All db related plugins like mysql and pg can look into the db options object and so on.

This looks cleaner for me and I think you won't be able to pollute the the config with param that can break some other plugins.

OlivierAlbertini · 2019-12-19T16:25:51Z

So the dns plugin can look into the http options and see which urls are ignored. All db related plugins like mysql and pg can look into the db options object and so on.

This looks cleaner for me and I think you won't be able to pollute the the config with param that can break some other plugins.

Unfortunately, ignoring some urls doesn't mean you want to ignore dns lookup as well...
Besides, dns would require to access to the exporter config not http plugin config. As discussed in the SIG, developer will make manual configuration.

This looks cleaner for me too and passing the whole config can have some benefits (http/https/http2).

dyladan · 2019-12-19T16:36:00Z

This looks cleaner for me too and passing the whole config can have some benefits (http/https/http2).

Fully agree. It simplifies the implementation too. I'll update the PR today.

dyladan · 2019-12-30T22:13:33Z

@obecny @OlivierAlbertini take a look. It is drastically simpler now. The shared options object and the plugin-specific options are the same "shape" now. They have a few "sections" like dns and http which can be used by whatever plugins deem appropriate.

new NodeTracer({
  options: {
    http: {
      ignoreIncomingPaths: ["/healthcheck"],
    },
    database: {
      enhancedDatabaseReporting: true,
    },
    dns: {
      ignoreHostnames: ["localhost"]
    }
  },
});

obecny

I have left few comments, my biggest concern is mixing the configs between web and node which I think is bad idea and we should have clear separation for that, we can discuss this on sig meeting

packages/opentelemetry-core/src/utils/url.ts

obecny · 2019-12-31T00:04:45Z

packages/opentelemetry-plugin-xml-http-request/src/xhr.ts


 // how long to wait for observer to collect information about resources
 // this is needed as event "load" is called before observer
 // hard to say how long it should really wait, seems like 300ms is
 // safe enough
 const OBSERVER_WAIT_TIME_MS = 300;

-/**


why did you remove that ?

All plugin configurations are being centralized. That is the point of this PR

obecny · 2019-12-31T00:12:05Z

packages/opentelemetry-types/src/trace/instrumentation/Plugin.ts


+/**


I think mixing plugin options between web and node is very bad idea. Things like database, dns etc will never be shared on web, and xhr will never be shared on node.
I think that such changes w discuss could discuss on sig meeting and how we want to handle that. I would rather have web and node separation like this is done for example in core/platform.

We can talk about it in SIG but I thought this was already pretty much agreed. We've had a couple discussions about it already.

packages/opentelemetry-core/test/utils/url.test.ts

obecny · 2019-12-31T00:44:08Z

packages/opentelemetry-node/src/instrumentation/utils.ts

+ *
+ * @param options config objects to merge
+ */
+export function mergeOptions(


Missing unit tests.
This function doesn't guarantee the "frozen state" when option is array or object.

I'll take another pass at this

packages/opentelemetry-core/src/utils/url.ts

packages/opentelemetry-core/src/trace/instrumentation/BasePlugin.ts

OlivierAlbertini

Perhaps instead of checking each time if this._options.http exists (and for other plugin)

this._options.http &&

We could ensure that it is not null/undefined when we pass the options during the call at the enable method ? WDYT ?

OlivierAlbertini · 2020-01-03T19:45:48Z

packages/opentelemetry-plugin-http/README.md

@@ -31,7 +31,10 @@ const tracer = new NodeTracer({
 // You may use a package name or absolute path to the file.
 path: '@opentelemetry/plugin-http',
 // http plugin options
+ options: {}


if I understand correctly your comment #628 (comment),

This change should be updated, right ?

No, they just match now and are merged together. You can still configure one plugin to have different ignoreUrls for example than the rest of the plugins.

OlivierAlbertini · 2020-01-03T19:47:13Z

packages/opentelemetry-plugin-http/src/http.ts

- if (this._config.applyCustomAttributesOnSpan) {
+ if (
+ this._options.http &&
+ this._options.http.applyCustomAttributesOnSpan


this._options.http?.applyCustomAttributesOnSpan

was having issues getting the compile to work with the new ts features for some reason. didn't feel like spending a lot of time to figure out what was wrong.

packages/opentelemetry-plugin-http/src/http.ts

OlivierAlbertini · 2020-01-03T20:12:06Z

packages/opentelemetry-types/src/trace/instrumentation/Plugin.ts

+ dns?: DNSPluginOptions;
+ http?: HttpPluginOptions;
+ xhr?: XMLHttpRequestPluginOptions;
+ custom?: CustomPluginOptions;


what about:
[custom: string]: any;
People would be able to add their own plugin like this:

const options: PluginOptions = { http: { serverName: 'hello', }, amqp: { something: 'else' } }

I don't have a strong opinion on this, I'm just curious.

There is a whole section named custom which allows you to put custom config options there.

dyladan · 2020-01-03T20:24:33Z

We could ensure that it is not null/undefined when we pass the options during the call at the enable method ? WDYT ?

good idea

…ry#628)

chore: more advanced plugin configuration

67bc7b7

dyladan requested review from bg451, markwolff, mayurkale22, obecny, OlivierAlbertini, rochdev and vmarchaud as code owners December 16, 2019 19:53

chore: revert required config option

d14a6b9

fix: lint

2c05f06

dyladan added Awaiting reviewer feedback document Documentation-related labels Dec 16, 2019

OlivierAlbertini reviewed Dec 18, 2019

View reviewed changes

examples/dns/setup.js Show resolved Hide resolved

chore: add typing for dns options

0014f5b

fix: lint

1a39ec5

dyladan added the do-not-merge/hold label Dec 18, 2019

chore: refactor config to share a single object shape

9cc8d84

dyladan removed the do-not-merge/hold label Dec 30, 2019

Merge remote-tracking branch 'upstream/master' into config

a8a06da

chore: move plugin options to shared interface

7f2f03a

obecny suggested changes Dec 31, 2019

View reviewed changes

dyladan added 2 commits December 31, 2019 12:21

chore: review comments

ceb6b66

fix: lint

cb0b812

OlivierAlbertini reviewed Jan 2, 2020

View reviewed changes

packages/opentelemetry-core/src/trace/instrumentation/BasePlugin.ts Outdated Show resolved Hide resolved

packages/opentelemetry-core/src/trace/instrumentation/BasePlugin.ts Outdated Show resolved Hide resolved

OlivierAlbertini added the needs-rebase label Jan 2, 2020

dyladan added 2 commits January 2, 2020 14:51

Merge remote-tracking branch 'upstream/master' into config

25fd299

fix: lint

9da7a62

dyladan removed the needs-rebase label Jan 2, 2020

dyladan added 5 commits January 2, 2020 14:59

chore: rename _config to _options

401ceb2

chore: fix typo

69cc7e5

chore: fix lint

e35064a

chore: remove old import

34dc175

chore: fix compilation

e5c6798

OlivierAlbertini reviewed Jan 3, 2020

View reviewed changes

mayurkale22 added the size/XL label Jan 6, 2020

OlivierAlbertini mentioned this pull request Jan 30, 2020

chore: 0.4.0 release proposal #751

Merged

OlivierAlbertini added the breaking label Jan 30, 2020

dyladan closed this Feb 5, 2020

Flarna deleted the config branch August 12, 2020 23:10

pichlermarc pushed a commit to dynatrace-oss-contrib/opentelemetry-js that referenced this pull request Dec 15, 2023

Build: Add test-all-versions scripts to redis & ioredis (open-telemet…

b213984

…ry#628)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: more advanced plugin configuration #628

chore: more advanced plugin configuration #628

dyladan commented Dec 16, 2019

codecov-io commented Dec 16, 2019 •

edited

Loading

mayurkale22 commented Dec 17, 2019

dyladan commented Dec 17, 2019

dyladan commented Dec 17, 2019

mayurkale22 commented Dec 18, 2019

obecny commented Dec 18, 2019

dyladan commented Dec 18, 2019

obecny commented Dec 18, 2019 •

edited

Loading

OlivierAlbertini commented Dec 19, 2019 •

edited

Loading

dyladan commented Dec 19, 2019

dyladan commented Dec 30, 2019

obecny left a comment

obecny Dec 31, 2019

dyladan Dec 31, 2019

obecny Dec 31, 2019

dyladan Dec 31, 2019

obecny Dec 31, 2019

dyladan Dec 31, 2019

OlivierAlbertini left a comment

OlivierAlbertini Jan 3, 2020

dyladan Jan 3, 2020

OlivierAlbertini Jan 3, 2020

dyladan Jan 3, 2020

OlivierAlbertini Jan 3, 2020

dyladan Jan 3, 2020

dyladan commented Jan 3, 2020


		/**

chore: more advanced plugin configuration #628

chore: more advanced plugin configuration #628

Conversation

dyladan commented Dec 16, 2019

Which problem is this PR solving?

Short description of the changes

Default configuration and plugins

Configure a plugin with options

Configure shared plugin options

Disable a plugin and leave all others enabled

You no longer need to explicitly give the whole configuration object

codecov-io commented Dec 16, 2019 • edited Loading

Codecov Report

mayurkale22 commented Dec 17, 2019

dyladan commented Dec 17, 2019

dyladan commented Dec 17, 2019

mayurkale22 commented Dec 18, 2019

obecny commented Dec 18, 2019

dyladan commented Dec 18, 2019

obecny commented Dec 18, 2019 • edited Loading

OlivierAlbertini commented Dec 19, 2019 • edited Loading

dyladan commented Dec 19, 2019

dyladan commented Dec 30, 2019

obecny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OlivierAlbertini left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dyladan commented Jan 3, 2020

codecov-io commented Dec 16, 2019 •

edited

Loading

obecny commented Dec 18, 2019 •

edited

Loading

OlivierAlbertini commented Dec 19, 2019 •

edited

Loading