fix: rewrite plugin loader #686

kjin · 2018-03-08T22:21:59Z

This change re-writes the Plugin Loader entirely.

The existing Plugin Loader code exists as a set of module-scope functions and variables. While there aren’t any major problems here, it’s worth re-writing it for the following reasons:

To make it more idiomatic to TypeScript, especially now that it has type annotations
To make the code more readable
To expose a more testable interface (and improve unit tests)
To depend on a third party module (require-in-the-middle) for monkeypatching
To make future behavioral changes more easy to implement:
- Support for multiple plugins for the same module
- Support for plugins that don’t monkeypatch at all

User-facing changes:

Intercept<T> is no longer a type on the public interface of the Trace Agent; its definition was merged with Patch<T>. Arguably, they are all "patches". Users will still be warned if they provide both patch and intercept. (Edit: This has been reverted.)
We no longer warn if neither patch or intercept exist on a Patch. This is to prevent needless warnings when a module such as knex needs patches for some versions but not others.
All log messages from the plugin loader have been revised to be more regex-searchable.
We accept an undocumented '[core]' plugin key. A plugin loaded under this key would behave as if the key were any other core module, such as 'http'. The plan is that at some point, we would use the '[core]' key to patch core modules rather than 'http', because it's somewhat misleading that 'http' also patches 'https'.

(All of these are up for discussion.)

Notes for Reviewers

This change used to also introduce multiple plugins for a single module. I decided against that since it's (1) too big of a change and (2) unclear whether this makes sense for an end-user. Just something to keep in mind if you see something weird like storing plugins as an array or two layers of version-checks (you should make a note of it if it doesn't make sense).
The computed PR diff for trace-plugin-loader.ts is completely useless. I recommend looking at the commits separately -- the first commit is just the removal of the old file.

codecov · 2018-03-15T20:00:51Z

Codecov Report

Merging #686 into master will increase coverage by 1.09%.
The diff coverage is 92.66%.

@@            Coverage Diff             @@
##           master     #686      +/-   ##
==========================================
+ Coverage   89.45%   90.54%   +1.09%     
==========================================
  Files          29       29              
  Lines        1422     1439      +17     
  Branches      287      285       -2     
==========================================
+ Hits         1272     1303      +31     
+ Misses         66       57       -9     
+ Partials       84       79       -5

Impacted Files	Coverage Δ
src/plugins/plugin-knex.ts	`92.85% <ø> (ø)`	⬆️
src/index.ts	`90.14% <100%> (+0.14%)`	⬆️
src/util.ts	`95.45% <100%> (+2.59%)`	⬆️
src/trace-plugin-loader.ts	`92.56% <92.41%> (+6.6%)`	⬆️
src/plugins/plugin-mysql2.ts	`72.97% <0%> (-10.82%)`	⬇️
src/trace-writer.ts	`88.52% <0%> (+0.81%)`	⬆️
src/plugins/plugin-http.ts	`90.24% <0%> (+10.97%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dca5cc0...eb986a3. Read the comment docs.

jinwoo

Still reviewing. Sending some comments first.

src/index.ts

-    pluginLoader.deactivate();
+    try {
+      const loader = pluginLoader.get();
+      loader.deactivate();


src/index.ts

+    try {
+      const loader = pluginLoader.get();
+      loader.deactivate();
+    } catch (e) {


src/plugin-types.ts

-export type Instrumentation<T> = Patch<T>|Intercept<T>;
-
-export type Plugin = Array<Instrumentation<any>>;
+export type Plugin = Array<Partial<Patch<any>>>;


src/trace-plugin-loader.ts

+export class ModulePluginWrapper implements PluginWrapper {
+  // Sentinel value to indicate that a plugin has not been loaded into memory
+  // yet.
+  private static readonly NOT_LOADED: Plugin = [];


src/trace-plugin-loader.ts

+    // ranges
+    let numFiles = 0;
+    plugin.forEach(instrumentation => {
+      const postLoadVersions = instrumentation.versions;


src/trace-plugin-loader.ts

+  }
+
+  unapplyAll() {
+    this.unpatchFns.reverse().forEach(fn => fn());


src/trace-plugin-loader.ts

+          this.unpatchFns.push(() => {
+            this.logger.info(
+                `PluginWrapper#unapplyAll: [${logString}] Unpatching file.`);
+            instrumentation.unpatch!(exportedValue);


jinwoo

Sorry I didn't look at test code very closely.

src/trace-plugin-loader.ts

-    logger.error('Plugins activated more than once.');
-    return;
+/**
+ * A class that represents wrapper logic on top of plugins that patch core


src/trace-plugin-loader.ts

+   */
+  applyPlugin<T>(moduleExports: T, file: string, version: string): T {
+    return this.children.reduce((exportedValue, child) => {
+      return child.applyPlugin(exportedValue, file, version);


src/trace-plugin-loader.ts

+ */
+export class PluginLoader {
+  // Key on which core modules are stored.
+  static readonly CORE_MODULE = '[core]';


src/trace-plugin-loader.ts

+   * @param config The configuration for this instance.
+   */
+  constructor(logger: Logger, config: PluginLoaderConfig) {
+    this.logger = logger;


src/trace-plugin-loader.ts

+      const value = config.plugins[key];
+      // Core module plugins share a common key.
+      const coreModule = builtinModules.indexOf(key) !== -1 ||
+          key === PluginLoader.CORE_MODULE;


src/trace-plugin-loader.ts

+   * loaded or applied, as well as unpatching any modules for which plugins
+   * specified an unpatching method.
+   */
+  deactivate(): PluginLoader {


src/trace-plugin-loader.ts

+   * Adds a search path for plugin modules. Intended for testing purposes only.
+   * @param searchPath The path to add.
+   */
+  static setPluginSearchPath(searchPath: string) {


src/trace-plugin-loader.ts

+    const parts = moduleStr.replace(/\\/g, '/').split('/');
+    let indexOfFile = 1;
+    if (parts[0].startsWith('@')) {  // for @org/package
+      indexOfFile = 2;


test/test-plugin-loader.ts

@@ -1,5 +1,5 @@
 /**
- * Copyright 2017 Google Inc. All Rights Reserved.
+ * Copyright 2018 Google Inc. All Rights Reserved.


test/test-plugin-loader.ts

-        'module-h': 'module-h-plugin'
-      }
+
+    it('doesn\'t unpatch twice', () => {


.gitignore

@@ -1,4 +1,5 @@
 node_modules/
+!test/fixtures/loader/node_modules/


src/plugin-types.ts

-export type Instrumentation<T> = Patch<T>|Intercept<T>;
-
-export type Plugin = Array<Instrumentation<any>>;
+export type Plugin = Array<Partial<Patch<any>>>;


src/plugins/plugin-knex.ts

@@ -18,7 +18,6 @@
 var shimmer = require('shimmer');
 var util = require('util');

-// knex 0.10.x and 0.11.x do not need patching


foo

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Mar 8, 2018

kjin force-pushed the plugin-loader-rewrite branch 2 times, most recently from b210598 to 64b0bf2 Compare March 15, 2018 20:00

kjin force-pushed the plugin-loader-rewrite branch 6 times, most recently from 57ae381 to e3f0558 Compare March 19, 2018 19:59

kjin mentioned this pull request Mar 19, 2018

test: don't use exec to test preloaded modules #696

Merged

kjin force-pushed the plugin-loader-rewrite branch from e3f0558 to 5cc7dad Compare March 19, 2018 20:47

kjin changed the title ~~feat: allow multiple plugins for a single module~~ refactor: rewrite plugin loader Mar 19, 2018

kjin force-pushed the plugin-loader-rewrite branch 6 times, most recently from a64a8e2 to f330840 Compare March 20, 2018 00:24

kjin changed the title ~~refactor: rewrite plugin loader~~ fix: rewrite plugin loader Mar 20, 2018

kjin force-pushed the plugin-loader-rewrite branch 2 times, most recently from cc98657 to 8fa83cf Compare March 20, 2018 00:50

kjin requested review from ofrobots, jinwoo and DominicKramer and removed request for ofrobots, jinwoo and DominicKramer March 20, 2018 00:55

kjin force-pushed the plugin-loader-rewrite branch 2 times, most recently from 5badda4 to 019db1f Compare March 20, 2018 17:57

kjin force-pushed the plugin-loader-rewrite branch from 019db1f to 6f73473 Compare March 20, 2018 20:23

kjin requested review from ofrobots, jinwoo and DominicKramer March 20, 2018 20:29

jinwoo reviewed Mar 20, 2018

View reviewed changes

kjin force-pushed the plugin-loader-rewrite branch from 065bc5b to d7d46a0 Compare March 21, 2018 00:14

jinwoo approved these changes Mar 22, 2018

View reviewed changes

kjin force-pushed the plugin-loader-rewrite branch from d7d46a0 to 11a0fc9 Compare March 22, 2018 18:37

DominicKramer reviewed Mar 22, 2018

View reviewed changes

kjin force-pushed the plugin-loader-rewrite branch from 11a0fc9 to 59644b1 Compare March 22, 2018 21:10

DominicKramer approved these changes Mar 22, 2018

View reviewed changes

kjin added 6 commits March 23, 2018 10:28

refactor: remove trace-plugin-loader.ts

d7f1c12

refactor: re-implement plugin loader

9ba58b5

test: rewrite plugin loader tests

e0e6599

test: update other tests

c40a9fd

foo

fix: drop warning message for non-patched modules

bd9160e

refactor: address comments

eb986a3

kjin force-pushed the plugin-loader-rewrite branch from 59644b1 to eb986a3 Compare March 23, 2018 17:28

kjin merged commit 1604c48 into googleapis:master Mar 23, 2018

This was referenced Mar 23, 2018

fix: rewrite all log messages #700

Merged

Plugin loader doesn't support circular requires #618

Closed

kjin mentioned this pull request Apr 4, 2018

fix: fix https tracing breakage in node <9 and rewrite http tests #717

Merged

release-please bot mentioned this pull request Jun 30, 2022

chore(main): release 7.0.0 #1452

Merged

		@@ -1,4 +1,5 @@
		node_modules/
		!test/fixtures/loader/node_modules/

fix: rewrite plugin loader #686

fix: rewrite plugin loader #686

Conversation

kjin commented Mar 8, 2018 • edited Loading

Notes for Reviewers

codecov bot commented Mar 15, 2018 • edited Loading

Codecov Report

jinwoo left a comment

Choose a reason for hiding this comment

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

jinwoo left a comment

Choose a reason for hiding this comment

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

kjin commented Mar 8, 2018 •

edited

Loading

codecov bot commented Mar 15, 2018 •

edited

Loading