feat: add mechanism to load internal module files to patch #52

fabiogomessilva · 2018-06-16T09:51:20Z

Sometimes, there is a need to patch internal module files that are not exported by default. This PR aims to add a mechanism to make it possible to load internal files from a module and make it available to a plugin.

kjin · 2018-06-18T17:19:04Z

packages/opencensus-core/src/trace/instrumentation/base-plugin.ts

-    this.setPluginContext(moduleExports, tracer, version);
+  applyPatch(
+      moduleExports: any, tracer: modelTypes.Tracer, version: string,
+      basedir?: string): any {


basedir is optional here but is passed to functions that require it to have a value later. I would suggest making it required here as well.

It will not be used by all implementations, just the ones that need to patch internal files that are not exported. The parameter as optional does not affect current implementations.

TS optionality doesn't refer to whether it is required to be used, but whether it is required to be given a value. In the latter case it is not optional because some plugins just won't work if you don't pass it.

I understand that you'd have to update the plugins to add a basedir argument -- if you agree that it should be required then it should be done in this PR.

Method loadPlugins from PluginLoader is always passing this parameter value.

result = plugin.applyPatch(exports, this.tracer, version, basedir);

The implementations that will require this parameter will have it available.

I think you might be confounding optional assignment vs. optional usage:

optional assignment

// it is OK for us not to pass a value for `basedir`. plugin.applyPatch(exports, this.tracer, version);

optional usage

class MyPlugin extends BasePlugin { applyPatch(exports, tracer, version, basedir) { // it is OK for us not to use `basedir`. } }

The syntax basedir? suggests that basedir's assignment is optional. But as we have both said, the real intent that we want to convey is that its usage is optional. (There is no syntax for this; the only way to show this intent is to document it.)

kjin · 2018-06-18T17:23:01Z

packages/opencensus-core/src/trace/instrumentation/types.ts

+/**
+ * Maps a name (key) representing a internal file module and its exports
+ */
+export type ModuleExports = {


The type name here should be more descriptive. Something like ModuleExportsMapping or something like that. But I wonder if you really need this type to be defined explicitly, because it's so broad. (Why not just define it locally in base-plugin.ts.)

Agree. Done !

kjin · 2018-06-18T17:25:44Z

packages/opencensus-core/src/trace/instrumentation/base-plugin.ts

+    if (this.internalFileList) {
+      Object.keys(this.internalFileList).map(versionRange => {
+        if (semver.satisfies(this.version, versionRange)) {
+          result = this.loadInternalModuleFiles(


Should you warn if multiple versions match? It sounds like users would be depending on implementation-private behavior.

If multiple versions match, the last one will prevail.

Can you add a logger warning? It should be brought to the user's attention because it is mostly a bug for someone to user overlapping version ranges.

kjin · 2018-06-18T17:25:57Z

packages/opencensus-core/src/trace/instrumentation/base-plugin.ts

+    let result: types.ModuleExports = null;
+    this.logger.debug('loadInternalFiles %o', this.internalFileList);
+    if (this.internalFileList) {
+      Object.keys(this.internalFileList).map(versionRange => {


Prefer forEach over map if you are not using the return value.

kjin · 2018-06-18T17:26:44Z

packages/opencensus-core/src/trace/instrumentation/base-plugin.ts

+  /** a logger */
+  protected logger: Logger;
+  /** list of internal files that need patch and are not exported by default */
+  protected internalFileList: types.PluginInternalFiles;


This is readonly, right?

kjin · 2018-06-18T17:27:51Z

packages/opencensus-core/src/trace/instrumentation/base-plugin.ts

+      basedir: string): types.ModuleExports {
+    const extraModules: types.ModuleExports = {};
+    if (extraModulesList) {
+      Object.keys(extraModulesList).map(modulename => {


Use a camelcase variable name (modulename).

kjin · 2018-06-18T17:30:45Z

packages/opencensus-core/src/trace/instrumentation/base-plugin.ts

+        try {
+          this.logger.debug('loading File %s', extraModulesList[modulename]);
+          extraModules[modulename] =
+              require(path.join(basedir, extraModulesList[modulename]));


I am not certain that this will work in all cases. What does basedir usually look like -- is it absolute or relative?

The basedir parameter is passed to the callback of the hook function from lib "require-in-the-middle", it is an absolute path.

kjin · 2018-06-18T17:32:12Z

packages/opencensus-core/src/trace/instrumentation/base-plugin.ts

    this.moduleExports = moduleExports;
    this.tracer = tracer;
    this.version = version;
+    this.basedir = basedir;
+    this.logger = tracer.logger || logger.logger();


Why not just tracer.logger?

kjin · 2018-06-18T17:34:27Z

...test/instrumentation/node_modules/@opencensus/instrumentation-simple-module/simple-module.ts

@@ -20,18 +20,36 @@ import * as shimmer from 'shimmer';


 export class SimpleModulePlugin extends classes.BasePlugin {
+  protected internalFileList: types.PluginInternalFiles = {


This module doesn't seem very simple anymore. Can you have a separate instrumentation fixture for something that requires an internal module to be patched.

A new one was created.

kjin · 2018-06-18T17:35:36Z

packages/opencensus-nodejs/test/test-plugin-loader.ts

+        const extraFile = 'src/extra-module';
+        const extraModule = require(path.join(indexPath, extraFile));
+        assert.strictEqual(extraModule.name(), 'patched-' + extraModuleName);
+        assert.strictEqual(extraModule.value(), 121);


extraModule is already not user-visible. I would rather see a test based around a module that uses the module exports of an internal file.

I think if the internal file export is accessible, whats the point to load it this way?

It does not simulate a real user-visible behavior. Based on this test alone, it is not clear that if the internal module's exports are patched, it will be visible to the user through behavior changes in simple-module.

Test improved.

kjin · 2018-06-18T17:37:03Z

Overall, this is much better than before. I think it's good to not expose details about module internals to users.

kjin

I think this will be good to land after these comments. I still think moduleExports does not belong as a field on Plugin... let's address that in a separate PR.

kjin · 2018-06-19T01:01:01Z

packages/opencensus-core/src/trace/instrumentation/base-plugin.ts

+   * applyPatch() and applyUnpatch()
+   *
+   * GoF Template Method Pattern - this is the variant part of the pattern
+   * Each plugin should implement his own version


I wasn't aware that this pattern had a specific name, TIL. However, I don't think this necessary belongs in docs, it may be confusing to users. Not everyone has read GoF.

I would just mention that this method will be called when applyPluginPatch is called.

Design Patterns are meant to be a kind of universal language, I think it is not confusing at all, it adds more information and makes things clear. I think our audience is pretty much aware of them.

kjin · 2018-06-19T01:02:42Z

packages/opencensus-core/src/trace/instrumentation/base-plugin.ts

+    if (this.internalFileList) {
+      Object.keys(this.internalFileList).map(versionRange => {
+        if (semver.satisfies(this.version, versionRange)) {
+          result = this.loadInternalModuleFiles(


Can you add a logger warning? It should be brought to the user's attention because it is mostly a bug for someone to user overlapping version ranges.

kjin · 2018-06-19T01:03:25Z

packages/opencensus-core/src/trace/instrumentation/types.ts

   */
-  // tslint:disable:no-any
-  applyPatch(
+  applyPluginPatch(


Suggestion: enable and disable to distinguish this from applyPatch and applyUnpatch.

For the public interface [enable/disable]PluginPatch ? or the protected methods [enable/disable]Patch ?

kjin

Re: naming, I meant just call the functions enable and disable.

kjin · 2018-06-19T02:11:40Z

packages/opencensus-core/src/trace/instrumentation/base-plugin.ts

    if (this.internalFileList) {
      this.logger.debug('loadInternalFiles %o', this.internalFileList);
      Object.keys(this.internalFileList).forEach(versionRange => {
        if (semver.satisfies(this.version, versionRange)) {
+          if (result) {


Not to be pedantic, but this can yield strange results if result just happens to be a falsey value (0 or ''). For correctness' sake I would just make overlap a counter (or otherwise just print the warning right in this function).

Refactory done.

kjin · 2018-06-19T02:16:47Z

packages/opencensus-core/src/trace/instrumentation/base-plugin.ts

+   * applyPatch() and applyUnpatch()
+   *
+   * GoF Template Method Pattern - this is the variant part of the pattern
+   * Each plugin should implement his own version.


I would not consider this universal language. I did not know what this was referring to and had to look it up. It might be true that I am not the common case, but it is better not to assume something like this about the audience. After all, it is possible to understand the concept without knowing its name.

Feel free to document the design pattern at the top of the file, but I do not think it belongs here.

Another note, we should not use gender pronouns in code, especially when referring to impersonal objects. Please use its.

... document the design pattern at the top of the file ...

At the top, it loses context. I have improved the message.

…plate Method

kjin

LGTM. I will file a separate issue about the concern related to loading multiple modules with different versions.

…strumentation#52) * feat: add mechanism to load internal module files to patch * refactor: change Plugin Interface and BasePlugin to implement GoF Template Method * refactor: change http plugin to new interface * refactor: change https plugin to new interface * refactor: change http2 plugin to new interface * refactor: change mongodb plugin to new interface * refactor: improve load internal file test case * refactor: rename methods of Plugin interface

googlebot added the cla: yes label Jun 16, 2018

fabiogomessilva requested a review from kjin June 16, 2018 09:52

fabiogomessilva force-pushed the feat_extend_plugin_setup_v2 branch 7 times, most recently from fc173ef to f81e135 Compare June 16, 2018 15:24

This was referenced Jun 16, 2018

feat: add instrumentation-grpc package to opencensus-node #34

Merged

feat: add mechanism to load internal module files to patch #51

Closed

fabiogomessilva force-pushed the feat_extend_plugin_setup_v2 branch 3 times, most recently from 756415f to 9c3ceed Compare June 18, 2018 10:57

kjin reviewed Jun 18, 2018

View reviewed changes

fabiogomessilva force-pushed the feat_extend_plugin_setup_v2 branch from 2c06ae1 to 7d20f2c Compare June 19, 2018 01:10

kjin reviewed Jun 19, 2018

View reviewed changes

fabiogomessilva force-pushed the feat_extend_plugin_setup_v2 branch from 48e95b5 to c082458 Compare June 19, 2018 11:27

silva-fabio added 8 commits June 19, 2018 09:00

feat: add mechanism to load internal module files to patch

4079ef9

refactor: change Plugin Interface and BasePlugin to implement GoF Tem…

abf63a9

…plate Method

refactor: change http plugin to new interface

2d62646

refactor: change https plugin to new interface

5c37fe1

refactor: change http2 plugin to new interface

cf199c6

refactor: change mongodb plugin to new interface

cfe5c30

refactor: improve load internal file test case

e51ac20

refactor: rename methods of Plugin interface

5d442ac

fabiogomessilva force-pushed the feat_extend_plugin_setup_v2 branch from c082458 to 5d442ac Compare June 19, 2018 12:00

kjin approved these changes Jun 19, 2018

View reviewed changes

kjin merged commit 3a613a7 into census-instrumentation:master Jun 19, 2018

		@@ -20,18 +20,36 @@ import * as shimmer from 'shimmer';


		export class SimpleModulePlugin extends classes.BasePlugin {
		protected internalFileList: types.PluginInternalFiles = {

feat: add mechanism to load internal module files to patch #52

feat: add mechanism to load internal module files to patch #52

Conversation

fabiogomessilva commented Jun 16, 2018

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

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

Choose a reason for hiding this comment

kjin commented Jun 18, 2018

kjin 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

kjin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kjin Jun 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kjin left a comment

Choose a reason for hiding this comment

kjin Jun 19, 2018 •

edited

Loading