feat: add OTEL_NO_PATCH_MODULES #1153

markwolff · 2020-06-09T15:15:59Z

Which problem is this PR solving?

Closes Allow to disable plugin via environment variable #655

Short description of the changes

Reads new environment variable OPENTELEMETRY_NO_PATCH_MODULES in PluginLoader to selectively disable plugins for certain modules
Environment variable is parsed as a comma separated list of library names, e.g https,mongodb,@grpc/grpc-js will disable plugins (if installed) for these 3 modules

codecov · 2020-06-09T15:19:08Z

Codecov Report

Merging #1153 into master will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1153      +/-   ##
==========================================
+ Coverage   92.33%   92.37%   +0.03%     
==========================================
  Files         119      119              
  Lines        3433     3447      +14     
  Branches      696      700       +4     
==========================================
+ Hits         3170     3184      +14     
  Misses        263      263

Impacted Files	Coverage Δ
...telemetry-node/src/instrumentation/PluginLoader.ts	`93.82% <100.00%> (+1.28%)`	⬆️

obecny · 2020-06-09T15:23:02Z

@open-telemetry/javascript-maintainers & @open-telemetry/javascript-approvers I think we will soon have more and more new env variables we already have 3 PR opened all of them implementing more less the same functionality. I think we should unify all the environment properties to keep them in one place and have some environment class that will be created using the platform approach to either read values from process.env, global, window etc. and then to have enum for all env properties. We should unify this the sooner the better.
the mentioned PR:
#1069
#974

dyladan · 2020-06-09T15:23:55Z

Can we add a special case of some kind which disables all? Possibly * or something

edit: * might not be great because shell expansion may interfere

markwolff · 2020-06-09T16:51:01Z

I think we will soon have more and more new env variables we already have 3 PR opened all of them implementing more less the same functionality. I think we should unify all the environment properties to keep them in one place and have some environment class that will be created using the platform approach to either read values from process.env, global, window etc. and then to have enum for all env properties. We should unify this the sooner the better.

@obecny I agree. Something like getPlatformVariable(name: PlatformVariableEnum) seems good to me.

Can we add a special case of some kind which disables all? Possibly * or something

@dyladan do you know where shell expansion would be an issue? I tried on a few different environments and didn't run into any issues.

dyladan · 2020-06-09T17:21:58Z

Can we add a special case of some kind which disables all? Possibly * or something

@dyladan do you know where shell expansion would be an issue? I tried on a few different environments and didn't run into any issues.

I was just envisioning a scenario where someone does like VAR=* npm start and somehow their shell expands the * to be all the files in the directory. Maybe this isn't a concern, I'm just not a shell expansion guru.

dyladan · 2020-06-09T21:36:12Z

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

@@ -46,6 +52,14 @@ function filterPlugins(plugins: Plugins): Plugins {
  }, {});
 }

+function getIgnoreList(): string[] | '*' {
+  const envIgnoreList: string = process.env[envPluginDisabledList] || '';
+  if (envIgnoreList === '*') {


Can we pull this out to a constant?

mayurkale22 · 2020-06-10T17:16:21Z

packages/opentelemetry-node/README.md

+`OPENTELEMETRY_NO_PATCH_MODULES` accepts a
+comma separated list of module names to disabled specific plugins.
+The names should match what you use to `require` the module into your application.
+For example, `OPENTELEMETRY_PATCH_MODULES=pg,https` will disable the postgres plugin and the https plugin. To disable **all** plugins, set the environment variable to `*`.


Suggested change

For example, `OPENTELEMETRY_PATCH_MODULES=pg,https` will disable the postgres plugin and the https plugin. To disable **all** plugins, set the environment variable to `*`.

For example, `OPENTELEMETRY_NO_PATCH_MODULES=pg,https` will disable the postgres plugin and the https plugin. To disable **all** plugins, set the environment variable to `*`.

updated. thanks!

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

mayurkale22

Added a minor comment, otherwise LGTM

mwear

LGTM. Just one comment.

We currently have an OTEL_RESOURCE_LABELS environment variable here. It appears from open-telemetry/opentelemetry-specification#572 that most languages are using OTEL_ as a prefix. Should we consider calling this OTEL_NO_PATCH_MODULES?

markwolff · 2020-06-11T22:39:15Z

LGTM. Just one comment.

We currently have an OTEL_RESOURCE_LABELS environment variable here. It appears from open-telemetry/opentelemetry-specification#572 that most languages are using OTEL_ as a prefix. Should we consider calling this OTEL_NO_PATCH_MODULES?

Yes this is a good point, naming alignment would be great. I think shorter is better. @open-telemetry/javascript-approvers

vmarchaud

lgtm

markwolff added 3 commits June 9, 2020 00:30

feat(pluginloader): add module ignore list env var

d2cd19a

fix: linting

8f64731

fix: add csv test

cdfce49

markwolff requested review from bg451, dyladan, mayurkale22, mwear, naseemkullah, obecny, OlivierAlbertini, rochdev and vmarchaud as code owners June 9, 2020 15:15

markwolff mentioned this pull request Jun 9, 2020

Add platform agnostic way to read environment variables #1157

Closed

markwolff added 2 commits June 9, 2020 15:26

feat(plugins): add ignore *, add readme docs

79cd378

fix: linting

9baf67c

dyladan reviewed Jun 9, 2020

View reviewed changes

refactor: use wildcard constant

82921f5

dyladan approved these changes Jun 10, 2020

View reviewed changes

mayurkale22 reviewed Jun 10, 2020

View reviewed changes

markwolff added 2 commits June 10, 2020 14:40

fix: address review comments

ab5ff49

Merge branch 'master' into add-plugin-ignore-env-var

0402a68

mayurkale22 reviewed Jun 10, 2020

View reviewed changes

packages/opentelemetry-node/src/instrumentation/PluginLoader.ts Outdated Show resolved Hide resolved

mayurkale22 approved these changes Jun 10, 2020

View reviewed changes

markwolff added 2 commits June 10, 2020 15:18

style: make const all caps

7c39c60

style: add jsdoc comment

0a1db44

Merge branch 'master' into add-plugin-ignore-env-var

aa50385

mwear approved these changes Jun 11, 2020

View reviewed changes

refactor: rename to OTEL_NO_PATCH_MODULES

d12e1ee

vmarchaud approved these changes Jun 12, 2020

View reviewed changes

Merge branch 'master' into add-plugin-ignore-env-var

1285782

dyladan merged commit 8c56442 into open-telemetry:master Jun 12, 2020

dyladan added the enhancement New feature or request label Jun 12, 2020

markwolff changed the title ~~feat: add OPENTELEMETRY_NO_PATCH_MODULES~~ feat: add OTEL_NO_PATCH_MODULES Jun 17, 2020

markwolff deleted the add-plugin-ignore-env-var branch June 17, 2020 19:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add OTEL_NO_PATCH_MODULES #1153

feat: add OTEL_NO_PATCH_MODULES #1153

markwolff commented Jun 9, 2020

codecov bot commented Jun 9, 2020 •

edited

Loading

obecny commented Jun 9, 2020

dyladan commented Jun 9, 2020 •

edited

Loading

markwolff commented Jun 9, 2020

dyladan commented Jun 9, 2020

dyladan Jun 9, 2020

markwolff Jun 9, 2020

mayurkale22 Jun 10, 2020

markwolff Jun 10, 2020

mayurkale22 left a comment

mwear left a comment •

edited by markwolff

Loading

markwolff commented Jun 11, 2020

vmarchaud left a comment

	For example, `OPENTELEMETRY_PATCH_MODULES=pg,https` will disable the postgres plugin and the https plugin. To disable all plugins, set the environment variable to `*`.
	For example, `OPENTELEMETRY_NO_PATCH_MODULES=pg,https` will disable the postgres plugin and the https plugin. To disable all plugins, set the environment variable to `*`.

feat: add OTEL_NO_PATCH_MODULES #1153

feat: add OTEL_NO_PATCH_MODULES #1153

Conversation

markwolff commented Jun 9, 2020

Which problem is this PR solving?

Short description of the changes

codecov bot commented Jun 9, 2020 • edited Loading

Codecov Report

obecny commented Jun 9, 2020

dyladan commented Jun 9, 2020 • edited Loading

markwolff commented Jun 9, 2020

dyladan commented Jun 9, 2020

dyladan Jun 9, 2020

Choose a reason for hiding this comment

markwolff Jun 9, 2020

Choose a reason for hiding this comment

mayurkale22 Jun 10, 2020

Choose a reason for hiding this comment

markwolff Jun 10, 2020

Choose a reason for hiding this comment

mayurkale22 left a comment

Choose a reason for hiding this comment

mwear left a comment • edited by markwolff Loading

Choose a reason for hiding this comment

markwolff commented Jun 11, 2020

vmarchaud left a comment

Choose a reason for hiding this comment

codecov bot commented Jun 9, 2020 •

edited

Loading

dyladan commented Jun 9, 2020 •

edited

Loading

mwear left a comment •

edited by markwolff

Loading