Add Plugin loader #126

mayurkale22 · 2019-07-23T22:59:10Z

Closes #114

Add a PluginLoader class providing functionality to hook into module loading and apply plugins to enable tracing for http, grpc, mysql, express module.

Features:

Automated and modular testing of individual library/package plugins
Note that this also makes it easy to test against multiple different versions of any given library
portable plugin instrumentation can be installed without manual source code modification.

~~I will refactor the this PR when #110 is merged.~~

codecov-io · 2019-07-23T23:01:19Z

Codecov Report

Merging #126 into master will decrease coverage by 0.25%.
The diff coverage is 88.65%.

@@            Coverage Diff             @@
##           master     #126      +/-   ##
==========================================
- Coverage   93.84%   93.59%   -0.26%     
==========================================
  Files          41       44       +3     
  Lines        3282     3497     +215     
  Branches      385      413      +28     
==========================================
+ Hits         3080     3273     +193     
- Misses        202      224      +22

Impacted Files	Coverage Δ
...try-node-tracer/test/instrumentation/utils.test.ts	`100% <100%> (ø)`
...metry-node-tracer/src/instrumentation/constants.ts	`100% <100%> (ø)`
...e-tracer/test/instrumentation/PluginLoader.test.ts	`78.57% <78.57%> (ø)`
...ry-node-tracer/src/instrumentation/PluginLoader.ts	`88.19% <88.19%> (ø)`
...telemetry-node-tracer/src/instrumentation/utils.ts	`92.75% <92.75%> (ø)`
...ckages/opentelemetry-node-tracer/src/NodeTracer.ts
.../opentelemetry-node-tracer/test/NodeTracer.test.ts
... and 2 more

packages/opentelemetry-tracer/src/instrumentation/constants.ts

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

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

draffensperger · 2019-07-24T00:04:28Z

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

+  }
+
+  // tslint:disable-next-line:no-any
+  protected abstract applyPatch(): any;


Why does applyPatch need to return anything? Could it just be void?

The use case I can think of is that it allows to override the export if it's a function since the only way to patch a function is to completely replace the export. Then the loader could replace the export by whatever is returned by patch.

draffensperger · 2019-07-24T00:05:45Z

packages/opentelemetry-tracer/package.json

@@ -52,6 +67,8 @@
    "@opentelemetry/core": "^0.0.1",
    "@opentelemetry/context-async-hooks": "^0.0.1",
    "@opentelemetry/scope-base": "^0.0.1",
-    "@opentelemetry/types": "^0.0.1"
+    "@opentelemetry/types": "^0.0.1",
+    "require-in-the-middle": "^4.0.0",


Could you talk a bit more about why these are required? Given that this is a more generic package for the tracer, I'm nervous about adding Node-specific dependencies - or just dependencies in general that will add weight to the JS bundle when used in the browser.

This is required to add hooks (pre and post middleware hooks to plugin methods), which allows us to modify modules on-the-fly as they are being required. I was thinking to have separate flavor of tracer (may be web-tracer) for browser. IIUC only types and core packages are common for both node and browser. WDYT?

OK, would it make sense to call the package node-tracer then to make the distinction clear?

I was hoping that basic-tracer could at least be shared between the two as well.

I believe the plugin API is only needed by the automatic-tracer, so we could maybe move it to there to be compatible with the browser ?

IMO Plugin interface should stay in types and it makes sense to keep PluginLoader in node-tracer package.

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

draffensperger · 2019-07-24T00:48:28Z

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

@@ -98,7 +101,8 @@ export class PluginLoader {

        // Expecting a plugin from module;
        try {
-          const plugin: Plugin = require(moduleName).plugin;
+          // tslint:disable-next-line:no-any
+          const plugin: Plugin<any> = require(moduleName).plugin;


What happens if you give it unknown here rather than any?

Compilation Error: Type 'unknown' is not assignable to type 'T'

.gitignore

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

rochdev · 2019-07-24T18:39:59Z

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

+   * loaded.
+   * @param modulesToPatch A list of plugins.
+   */
+  loadPlugins(modulesToPatch: string[]): PluginLoader {


How can plugins be configured?

Also, how can custom plugins be used?

How can plugins be configured?

I was thinking to use list of strings to configure avaiable plugins. Something like,
const DEFAULT_INSTRUMENTATION_PLUGINS = ['http', 'express', 'mysql', ...] and use this const when calling the PluginLoader inside node-tracer constructor. WDYT?

What I meant is, how can configuration options be passed to individual plugins?

One way to do it would be to accept an object with plugin names as keys as configurations as values. To make this easier to use when configuration is not necessary, a boolean could be accepted as well.

For example:

Enable plugin manually

loadPlugins({ express: true })

Disable plugin manually

loadPlugins({ express: false })

Configure plugin manually (also implicitly enables it)

loadPlugins({ express: { middleware: false // configures Express to not have spans per middleware (example) } })

packages/opentelemetry-tracer/src/instrumentation/constants.ts

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

OlivierAlbertini

I added few comments that we could implement at later stage...

OlivierAlbertini · 2019-07-25T00:42:26Z

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

+   * loaded.
+   * @param modulesToPatch A list of plugins.
+   */
+  load(modulesToPatch: string[]): PluginLoader {


It would be nice to add options to pass for each module to patch e.g
pluginLoader.load(['http'], { http: { httpTimings: true, filterings: ['/healthcheck'] } });

Added in Plugin interface PR: https://github.com/open-telemetry/opentelemetry-js/pull/110/files#diff-0e705bb0dee9becebae81dd8b49650fdR31, ptal. An optional config an object to configure the plugin.

I still think enabling and configuring should be done in a single configuration. This means that instead of accepting an array, the loader should accept an object that for now would accept only booleans but can later be extended with configuration options. It will also make it easier to understand that you are not completely overriding the default by doing this.

SGTM, changed to an object (PluginConfig) instead of an array. Also, added @todo to add support for configuration options in the future. PTAL.

OlivierAlbertini · 2019-07-25T00:50:27Z

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

+export abstract class BasePlugin<T> implements Plugin<T> {
+  protected moduleExports!: T;
+  protected tracer!: Tracer;
+


I would add supportedVersions e.g express ['4.X']. Also supportedVersions would be optional since http doesn't need this. WDYT ?

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

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

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

mayurkale22 · 2019-08-01T17:22:10Z

Looks like I have addressed all the comments. PTAL

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

mayurkale22 · 2019-08-05T15:53:21Z

@rochdev @vmarchaud @OlivierAlbertini @draffensperger Looks like I have addressed all the comments. Please approve, if everything looks good.

vmarchaud · 2019-08-05T15:56:14Z

@mayurkale22 LGTM (note we can't approve directly since we are not reviewer)

draffensperger

LGTM pending final comment about the constants container (I'm open to keeping it but would recommend removing it)

packages/opentelemetry-node-tracer/src/instrumentation/constants.ts

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

Co-authored-by: Valentin Marchaud <contact@vmarchaud.fr>

…ack-merge-5.x

mayurkale22 requested review from danielkhan, iredelmeier, justindsmith and rochdev as code owners July 23, 2019 22:59

mayurkale22 commented Jul 23, 2019

View reviewed changes

packages/opentelemetry-tracer/src/instrumentation/constants.ts Outdated Show resolved Hide resolved

mayurkale22 commented Jul 23, 2019

View reviewed changes

packages/opentelemetry-types/src/trace/instrumentation/Plugin.ts Outdated Show resolved Hide resolved

draffensperger reviewed Jul 24, 2019

View reviewed changes

rochdev reviewed Jul 24, 2019

View reviewed changes

mayurkale22 force-pushed the plugin-loader branch 2 times, most recently from f3169e3 to f69df06 Compare July 24, 2019 21:55

OlivierAlbertini reviewed Jul 25, 2019

View reviewed changes

mayurkale22 mentioned this pull request Jul 25, 2019

PluginLoader: Check if version and supportedVersions are satisfied #132

Closed

mayurkale22 force-pushed the plugin-loader branch 2 times, most recently from 0828472 to d87d936 Compare July 25, 2019 03:16

draffensperger reviewed Jul 25, 2019

View reviewed changes

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

mayurkale22 force-pushed the plugin-loader branch 2 times, most recently from c66be2e to 1486639 Compare July 27, 2019 17:04

hekike reviewed Jul 28, 2019

View reviewed changes

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

mayurkale22 force-pushed the plugin-loader branch 4 times, most recently from 13c7e21 to 37b6575 Compare August 1, 2019 16:58

draffensperger reviewed Aug 1, 2019

View reviewed changes

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

mayurkale22 force-pushed the plugin-loader branch from e08b89e to 4e13f69 Compare August 2, 2019 01:39

draffensperger reviewed Aug 2, 2019

View reviewed changes

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

mayurkale22 force-pushed the plugin-loader branch from 2c0eec3 to 8f212e4 Compare August 2, 2019 17:58

mayurkale22 mentioned this pull request Aug 3, 2019

chore(packages): add http and grpc skeletons #160

Merged

draffensperger approved these changes Aug 5, 2019

View reviewed changes

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

OlivierAlbertini reviewed Aug 5, 2019

View reviewed changes

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

mayurkale22 force-pushed the plugin-loader branch from 8f212e4 to 1d5bd29 Compare August 5, 2019 17:28

draffensperger approved these changes Aug 5, 2019

View reviewed changes

mayurkale22 added 15 commits August 5, 2019 10:58

Add Plugin Loader

4a51978

Load simple-module

1ff4844

gts fix

1d36d89

Use type variable T instead of any

e5447c6

Log instead of throwing and remove redundant Plugins suffix

8e4f867

Add package specific .gitignore

0984925

Add @todo

8240978

Change DEFAULT_PLUGIN_PACKAGE_NAME_PREFIX to plugin

116a3b7

fix: build pipeline

49e5285

fix: remove test functions out of the class

9fea469

fix: use private members in test without exposing it

cbdb5a4

feat: add PluginConfig during load

75a4c4a

fix: make comment more descriptive

0846666

fix: move the any to the Plugin type

cd8a6a3

fix: remove constants container

6005993

mayurkale22 force-pushed the plugin-loader branch from 1d5bd29 to 6005993 Compare August 5, 2019 17:59

rochdev approved these changes Aug 5, 2019

View reviewed changes

mayurkale22 merged commit 20c9a49 into open-telemetry:master Aug 5, 2019

mayurkale22 deleted the plugin-loader branch August 5, 2019 22:28

mayurkale22 mentioned this pull request Aug 30, 2019

Nominating @OlivierAlbertini as JavaScript approver open-telemetry/community#166

Merged

dyladan pushed a commit to dyladan/opentelemetry-js that referenced this pull request Sep 9, 2022

fix: align globalThis fallbacks with otel-core (open-telemetry#126)

dc81c75

Co-authored-by: Valentin Marchaud <contact@vmarchaud.fr>

dyladan pushed a commit to dyladan/opentelemetry-js that referenced this pull request Sep 9, 2022

fix: align globalThis fallbacks with otel-core (open-telemetry#126)

9338171

Co-authored-by: Valentin Marchaud <contact@vmarchaud.fr>

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

Merge pull request open-telemetry#126 from renovate-bot/renovate/webp…

7c04a7a

…ack-merge-5.x

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Plugin loader #126

Add Plugin loader #126

mayurkale22 commented Jul 23, 2019 •

edited

Loading

codecov-io commented Jul 23, 2019 •

edited

Loading

draffensperger Jul 24, 2019

rochdev Jul 29, 2019

draffensperger Jul 24, 2019

mayurkale22 Jul 24, 2019

draffensperger Jul 24, 2019

vmarchaud Jul 24, 2019

mayurkale22 Jul 25, 2019

draffensperger Jul 24, 2019

mayurkale22 Jul 24, 2019

rochdev Jul 24, 2019

rochdev Jul 24, 2019

mayurkale22 Jul 25, 2019

rochdev Jul 29, 2019

OlivierAlbertini left a comment

OlivierAlbertini Jul 25, 2019

mayurkale22 Jul 25, 2019

rochdev Aug 1, 2019

mayurkale22 Aug 1, 2019

OlivierAlbertini Jul 25, 2019 •

edited

Loading

mayurkale22 commented Aug 1, 2019

mayurkale22 commented Aug 5, 2019

vmarchaud commented Aug 5, 2019

draffensperger left a comment

Add Plugin loader #126

Add Plugin loader #126

Conversation

mayurkale22 commented Jul 23, 2019 • edited Loading

codecov-io commented Jul 23, 2019 • edited Loading

Codecov Report

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

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

OlivierAlbertini Jul 25, 2019 • edited Loading

Choose a reason for hiding this comment

mayurkale22 commented Aug 1, 2019

mayurkale22 commented Aug 5, 2019

vmarchaud commented Aug 5, 2019

draffensperger left a comment

Choose a reason for hiding this comment

mayurkale22 commented Jul 23, 2019 •

edited

Loading

codecov-io commented Jul 23, 2019 •

edited

Loading

OlivierAlbertini Jul 25, 2019 •

edited

Loading