Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(pluginloader): move internalFilesList #401

Closed
wants to merge 1 commit into from
Closed

refactor(pluginloader): move internalFilesList #401

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
87 changes: 1 addition & 86 deletions packages/opentelemetry-core/src/trace/instrumentation/BasePlugin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,23 +20,19 @@ import {
Logger,
PluginConfig,
PluginInternalFiles,
PluginInternalFilesVersion,
} from '@opentelemetry/types';
import * as semver from 'semver';
import * as path from 'path';

/** This class represent the base to patch plugin. */
export abstract class BasePlugin<T> implements Plugin<T> {
supportedVersions?: string[];
readonly moduleName?: string; // required for internalFilesExports
readonly version?: string; // required for internalFilesExports
protected readonly _basedir?: string; // required for internalFilesExports
readonly internalFilesList?: PluginInternalFiles; // required for internalFilesExports

protected _moduleExports!: T;
protected _tracer!: Tracer;
protected _logger!: Logger;
protected _internalFilesExports!: { [module: string]: unknown }; // output for internalFilesExports
protected readonly _internalFilesList?: PluginInternalFiles; // required for internalFilesExports
protected _config!: PluginConfig;

enable(
Expand All @@ -48,7 +44,6 @@ export abstract class BasePlugin<T> implements Plugin<T> {
this._moduleExports = moduleExports;
this._tracer = tracer;
this._logger = logger;
this._internalFilesExports = this._loadInternalFilesExports();
if (config) this._config = config;
return this.patch();
}
Expand All @@ -57,86 +52,6 @@ export abstract class BasePlugin<T> implements Plugin<T> {
this.unpatch();
}

/**
* @TODO: To avoid circular dependencies, internal file loading functionality currently
* lives in BasePlugin. It is not meant to work in the browser and so this logic
* should eventually be moved somewhere else where it makes more sense.
* https://github.com/open-telemetry/opentelemetry-js/issues/285
*/
private _loadInternalFilesExports(): PluginInternalFiles {
if (!this._internalFilesList) return {};
if (!this.version || !this.moduleName || !this._basedir) {
// log here because internalFilesList was provided, so internal file loading
// was expected to be working
this._logger.debug(
'loadInternalFiles failed because one of the required fields was missing: moduleName=%s, version=%s, basedir=%s',
this.moduleName,
this.version,
this._basedir
);
return {};
}
let extraModules: PluginInternalFiles = {};
this._logger.debug('loadInternalFiles %o', this._internalFilesList);
Object.keys(this._internalFilesList).forEach(versionRange => {
this._loadInternalModule(versionRange, extraModules);
});
if (Object.keys(extraModules).length === 0) {
this._logger.debug(
'No internal files could be loaded for %s@%s',
this.moduleName,
this.version
);
}
return extraModules;
}

private _loadInternalModule(
versionRange: string,
outExtraModules: PluginInternalFiles
): void {
if (semver.satisfies(this.version!, versionRange)) {
if (Object.keys(outExtraModules).length > 0) {
this._logger.warn(
'Plugin for %s@%s, has overlap version range (%s) for internal files: %o',
this.moduleName,
this.version,
versionRange,
this._internalFilesList
);
}
this._requireInternalFiles(
this._internalFilesList![versionRange],
this._basedir!,
outExtraModules
);
}
}

private _requireInternalFiles(
extraModulesList: PluginInternalFilesVersion,
basedir: string,
outExtraModules: PluginInternalFiles
): void {
if (!extraModulesList) return;
Object.keys(extraModulesList).forEach(moduleName => {
try {
this._logger.debug('loading File %s', extraModulesList[moduleName]);
outExtraModules[moduleName] = require(path.join(
basedir,
extraModulesList[moduleName]
));
} catch (e) {
this._logger.error(
'Could not load internal file %s of module %s. Error: %s',
path.join(basedir, extraModulesList[moduleName]),
this.moduleName,
e.message
);
}
});
}

protected abstract patch(): T;
protected abstract unpatch(): void;
}
64 changes: 0 additions & 64 deletions packages/opentelemetry-core/test/trace/BasePlugin.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,67 +13,3 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/

import { BasePlugin, NoopTracer, NoopLogger } from '../../src';
import * as assert from 'assert';
import * as path from 'path';
import * as types from './fixtures/test-package/foo/bar/internal';

const tracer = new NoopTracer();
const logger = new NoopLogger();

describe('BasePlugin', () => {
describe('internalFilesLoader', () => {
it('should load internally exported files', () => {
const testPackage = require('./fixtures/test-package');
const plugin = new TestPlugin();
assert.doesNotThrow(() => {
plugin.enable(testPackage, tracer, logger);
});

// @TODO: https://github.com/open-telemetry/opentelemetry-js/issues/285
if (typeof process !== 'undefined' && process.release.name === 'node') {
assert.ok(plugin['_internalFilesExports']);
assert.strictEqual(
(plugin['_internalFilesExports']
.internal as typeof types).internallyExportedFunction(),
true
);
assert.strictEqual(
plugin['_internalFilesExports'].expectUndefined,
undefined
);
assert.strictEqual(
(plugin['_moduleExports']![
'externallyExportedFunction'
] as Function)(),
true
);
} else {
assert.ok(true, 'Internal file loading is not tested in the browser');
}
});
});
});

class TestPlugin extends BasePlugin<{ [key: string]: Function }> {
readonly moduleName = 'test-package';
readonly version = '0.1.0';
readonly _basedir = basedir;

protected readonly _internalFilesList = {
'0.1.0': {
internal: 'foo/bar/internal.js',
},
'^1.0.0': {
expectUndefined: 'foo/bar/internal.js',
},
};

protected patch(): { [key: string]: Function } {
return this._moduleExports;
}
protected unpatch(): void {}
}

const basedir = path.dirname(require.resolve('./fixtures/test-package'));
76 changes: 72 additions & 4 deletions packages/opentelemetry-node-sdk/src/instrumentation/PluginLoader.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,13 @@
* limitations under the License.
*/

import { Logger, Plugin, Tracer, PluginConfig } from '@opentelemetry/types';
import {
Logger,
Plugin,
Tracer,
PluginConfig,
PluginInternalPatch,
} from '@opentelemetry/types';
import * as hook from 'require-in-the-middle';
import * as utils from './utils';

Expand All @@ -29,6 +35,10 @@ export interface Plugins {
[pluginName: string]: PluginConfig;
}

type PluginInternalModuleInfo = {
[key: string]: { patch: PluginInternalPatch; plugin: Plugin };
};

/**
* Returns the Plugins object that meet the below conditions.
* Valid criteria: 1. It should be enabled. 2. Should have non-empty path.
Expand Down Expand Up @@ -69,6 +79,27 @@ export class PluginLoader {
if (this._hookState === HookState.UNINITIALIZED) {
const pluginsToLoad = filterPlugins(plugins);
const modulesToHook = Object.keys(pluginsToLoad);
const pluginModules: { [key: string]: Plugin } = {};
const internalPatches: PluginInternalModuleInfo = {};

for (const pluginName of modulesToHook) {
try {
const modulePath = pluginsToLoad[pluginName].path!;
const plugin: Plugin = require(modulePath).plugin;
pluginModules[modulePath] = plugin;

// Group together things needed for internal module patching
Object.assign(
internalPatches,
searchForValidInternalModules(plugin, pluginName)
);
} catch (e) {
this.logger.error(
`PluginLoader#load: could not load plugin ${pluginName}. Error: ${e.message}`
);
}
}

// Do not hook require when no module is provided. In this case it is
// not necessary. With skipping this step we lower our footprint in
// customer applications and require-in-the-middle won't show up in CPU
Expand All @@ -79,10 +110,27 @@ export class PluginLoader {
}

// Enable the require hook.
hook(modulesToHook, (exports, name, baseDir) => {
hook(modulesToHook, { internals: true }, (exports, name, baseDir) => {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the new entry point into the internal stuff. It causes this to function to trigger for every internal file loaded in the module. This is probably not good for perf even though the module is immediately returned as is when there is no patching to be done.

if (this._hookState !== HookState.ENABLED) return exports;

if (internalPatches[name]) {
// Patch the internal module and return
this.logger.debug(
`PluginLoader#load: trying to patch internal module ${name}`
);
const internalPatch = internalPatches[name];
return internalPatch.patch.call(
internalPatch.plugin,
exports as never
);
} else if (!pluginsToLoad[name]) {
// Return the internal module as is
return exports;
}

const config = pluginsToLoad[name];
const modulePath = config.path!;
const plugin: Plugin = pluginModules[modulePath];
let version = null;

if (!baseDir) {
Expand All @@ -107,8 +155,6 @@ export class PluginLoader {

// Expecting a plugin from module;
try {
const plugin: Plugin = require(modulePath).plugin;

if (!utils.isSupportedVersion(version, plugin.supportedVersions)) {
return exports;
}
Expand Down Expand Up @@ -154,3 +200,25 @@ export class PluginLoader {
export function searchPathForTest(searchPath: string) {
module.paths.push(searchPath);
}

/**
* Group together fields needed to perform internal module patching
* @param plugin plugin which implements getInternalPatch
* @param pluginName name of the plugin, e.g. grpc, http
*/
function searchForValidInternalModules(
plugin: Plugin,
pluginName: string
): PluginInternalModuleInfo {
const internalPatches: PluginInternalModuleInfo = {};
if (plugin.internalFilesList && plugin.getInternalPatch) {
Object.keys(plugin.internalFilesList).forEach(fname => {
const ritmName = `${pluginName}/${fname}`;
internalPatches[ritmName] = {
patch: plugin.getInternalPatch!(fname) as never,
plugin: plugin,
};
});
}
return internalPatches;
}
2 changes: 0 additions & 2 deletions packages/opentelemetry-plugin-grpc/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,15 +41,13 @@
"@types/mocha": "^5.2.7",
"@types/node": "^12.6.9",
"@types/shimmer": "^1.0.1",
"@types/sinon": "^7.0.13",
"codecov": "^3.5.0",
"grpc": "~1.23.3",
"gts": "^1.1.0",
"mocha": "^6.2.0",
"nyc": "^14.1.1",
"node-pre-gyp": "^0.12.0",
"rimraf": "^3.0.0",
"sinon": "^7.5.0",
"tslint-microsoft-contrib": "^6.2.0",
"tslint-consistent-codestyle": "^1.15.1",
"ts-mocha": "^6.0.0",
Expand Down
Loading