From d6f669aa36caec3d339a3b06a91449f8401a2037 Mon Sep 17 00:00:00 2001 From: Mark Wolff Date: Fri, 20 Sep 2019 15:07:22 -0700 Subject: [PATCH] feat(baseplugin): add internalfilesloader (#240) * feat(baseplugin): add internalfilesloader * refactor: remove new ctor * refactor: baseplugin internal file loader * test: don't browser test internal files loading * chore: add todo issue * refactor: cast modules as unknown isntead of any * chore: fix linting --- .../src/trace/instrumentation/BasePlugin.ts | 100 +++++++++++++++++- .../test/trace/BasePlugin.test.ts | 79 ++++++++++++++ .../test-package/foo/bar/internal.d.ts | 1 + .../fixtures/test-package/foo/bar/internal.js | 4 + .../test/trace/fixtures/test-package/index.js | 4 + .../trace/fixtures/test-package/package.json | 11 ++ .../opentelemetry-plugin-grpc/package.json | 3 +- .../opentelemetry-plugin-grpc/src/grpc.ts | 83 +-------------- 8 files changed, 203 insertions(+), 82 deletions(-) create mode 100644 packages/opentelemetry-core/test/trace/BasePlugin.test.ts create mode 100644 packages/opentelemetry-core/test/trace/fixtures/test-package/foo/bar/internal.d.ts create mode 100644 packages/opentelemetry-core/test/trace/fixtures/test-package/foo/bar/internal.js create mode 100644 packages/opentelemetry-core/test/trace/fixtures/test-package/index.js create mode 100644 packages/opentelemetry-core/test/trace/fixtures/test-package/package.json diff --git a/packages/opentelemetry-core/src/trace/instrumentation/BasePlugin.ts b/packages/opentelemetry-core/src/trace/instrumentation/BasePlugin.ts index f13fb016ad..5a58aa6962 100644 --- a/packages/opentelemetry-core/src/trace/instrumentation/BasePlugin.ts +++ b/packages/opentelemetry-core/src/trace/instrumentation/BasePlugin.ts @@ -14,15 +14,30 @@ * limitations under the License. */ -import { Tracer, Plugin, Logger, PluginConfig } from '@opentelemetry/types'; +import { + Tracer, + Plugin, + 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 implements Plugin { + supportedVersions?: string[]; + readonly moduleName?: string; // required for internalFilesExports + readonly version?: string; // required for internalFilesExports + protected readonly _basedir?: string; // 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; - supportedVersions?: string[]; enable( moduleExports: T, @@ -33,6 +48,7 @@ export abstract class BasePlugin implements Plugin { this._moduleExports = moduleExports; this._tracer = tracer; this._logger = logger; + this._internalFilesExports = this._loadInternalFilesExports(); if (config) this._config = config; return this.patch(); } @@ -41,6 +57,86 @@ export abstract class BasePlugin implements Plugin { 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; } diff --git a/packages/opentelemetry-core/test/trace/BasePlugin.test.ts b/packages/opentelemetry-core/test/trace/BasePlugin.test.ts new file mode 100644 index 0000000000..c32778e3d0 --- /dev/null +++ b/packages/opentelemetry-core/test/trace/BasePlugin.test.ts @@ -0,0 +1,79 @@ +/** + * Copyright 2019, OpenTelemetry Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * 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.0.1'; + readonly _basedir = basedir; + + protected readonly _internalFilesList = { + '0.0.1': { + 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')); diff --git a/packages/opentelemetry-core/test/trace/fixtures/test-package/foo/bar/internal.d.ts b/packages/opentelemetry-core/test/trace/fixtures/test-package/foo/bar/internal.d.ts new file mode 100644 index 0000000000..2a1f45f2a7 --- /dev/null +++ b/packages/opentelemetry-core/test/trace/fixtures/test-package/foo/bar/internal.d.ts @@ -0,0 +1 @@ +export declare function internallyExportedFunction(): boolean; diff --git a/packages/opentelemetry-core/test/trace/fixtures/test-package/foo/bar/internal.js b/packages/opentelemetry-core/test/trace/fixtures/test-package/foo/bar/internal.js new file mode 100644 index 0000000000..0f0e71ee74 --- /dev/null +++ b/packages/opentelemetry-core/test/trace/fixtures/test-package/foo/bar/internal.js @@ -0,0 +1,4 @@ +Object.defineProperty(exports, "__esModule", { value: true }); +exports.internallyExportedFunction = function internallyExportedFunction() { + return true; +} diff --git a/packages/opentelemetry-core/test/trace/fixtures/test-package/index.js b/packages/opentelemetry-core/test/trace/fixtures/test-package/index.js new file mode 100644 index 0000000000..bddb4ea002 --- /dev/null +++ b/packages/opentelemetry-core/test/trace/fixtures/test-package/index.js @@ -0,0 +1,4 @@ +Object.defineProperty(exports, "__esModule", { value: true }); +exports.externallyExportedFunction = function externallyExportedFunction() { + return true; +} diff --git a/packages/opentelemetry-core/test/trace/fixtures/test-package/package.json b/packages/opentelemetry-core/test/trace/fixtures/test-package/package.json new file mode 100644 index 0000000000..9bb9c8f66c --- /dev/null +++ b/packages/opentelemetry-core/test/trace/fixtures/test-package/package.json @@ -0,0 +1,11 @@ +{ + "name": "test-package", + "version": "0.0.1", + "description": "", + "main": "index.js", + "scripts": { + "test": "echo \"Error: no test specified\" && exit 1" + }, + "author": "", + "license": "ISC" +} diff --git a/packages/opentelemetry-plugin-grpc/package.json b/packages/opentelemetry-plugin-grpc/package.json index e1494dd4a2..8624feb5c5 100644 --- a/packages/opentelemetry-plugin-grpc/package.json +++ b/packages/opentelemetry-plugin-grpc/package.json @@ -59,7 +59,6 @@ "@opentelemetry/node-sdk": "^0.0.1", "@opentelemetry/scope-async-hooks": "^0.0.1", "@opentelemetry/types": "^0.0.1", - "shimmer": "^1.2.1", - "semver": "^6.3.0" + "shimmer": "^1.2.1" } } diff --git a/packages/opentelemetry-plugin-grpc/src/grpc.ts b/packages/opentelemetry-plugin-grpc/src/grpc.ts index 266be7b67d..eda6b30d6e 100644 --- a/packages/opentelemetry-plugin-grpc/src/grpc.ts +++ b/packages/opentelemetry-plugin-grpc/src/grpc.ts @@ -27,7 +27,6 @@ import { AttributeNames } from './enums/AttributeNames'; import { grpc, ModuleExportsMapping, - ModuleNameToFilePath, GrpcPluginOptions, ServerCallWithMeta, SendUnaryDataCallback, @@ -42,7 +41,6 @@ import { import * as events from 'events'; import * as grpcModule from 'grpc'; import * as shimmer from 'shimmer'; -import * as semver from 'semver'; import * as path from 'path'; /** The metadata key under which span context is stored as a binary value. */ @@ -55,91 +53,20 @@ export class GrpcPlugin extends BasePlugin { options!: GrpcPluginOptions; - constructor(public moduleName: string, public version: string) { + constructor(readonly moduleName: string, readonly version: string) { super(); // TODO: remove this once options will be passed // see https://github.com/open-telemetry/opentelemetry-js/issues/210 this.options = {}; } - // TODO: Delete if moving internal file loaders to BasePlugin - // --- Note: Incorrectly ordered: Begin internal file loader --- // - // tslint:disable-next-line:no-any - protected _internalFilesExports: { [key: string]: any } | undefined; - protected readonly _internalFileList: ModuleExportsMapping = { + protected readonly _internalFilesList: ModuleExportsMapping = { '0.13 - 1.6': { client: 'src/node/src/client.js' }, '^1.7': { client: 'src/client.js' }, }; - - /** - * Load internal files according to version range - */ - private _loadInternalFiles(): ModuleExportsMapping { - let result: ModuleExportsMapping = {}; - if (this._internalFileList) { - this._logger.debug('loadInternalFiles %o', this._internalFileList); - Object.keys(this._internalFileList).forEach(versionRange => { - if (semver.satisfies(this.version, versionRange)) { - if (result) { - this._logger.warn( - 'Plugin for %s@%s, has overlap version range (%s) for internal files: %o', - this.moduleName, - this.version, - versionRange, - this._internalFileList - ); - } - result = this._loadInternalModuleFiles( - this._internalFileList[versionRange], - basedir - ); - } - }); - if (Object.keys(result).length === 0) { - this._logger.debug( - 'No internal file could be loaded for %s@%s', - this.moduleName, - this.version - ); - } - } - return result; - } - - /** - * Load internal files from a module and set internalFilesExports - */ - private _loadInternalModuleFiles( - extraModulesList: ModuleNameToFilePath, - basedir: string - ): ModuleExportsMapping { - const extraModules: ModuleExportsMapping = {}; - if (extraModulesList) { - Object.keys(extraModulesList).forEach(moduleName => { - try { - this._logger.debug('loading File %s', extraModulesList[moduleName]); - extraModules[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 - ); - } - }); - } - return extraModules; - } - // --- End of internal file loader stuff --- // + protected readonly _basedir = basedir; protected patch(): typeof grpcModule { - if (!this._internalFilesExports) { - this._internalFilesExports = this._loadInternalFiles(); - } this._logger.debug( 'applying patch to %s@%s', this.moduleName, @@ -155,8 +82,8 @@ export class GrpcPlugin extends BasePlugin { ); } - if (this._internalFilesExports && this._internalFilesExports['client']) { - grpcClientModule = this._internalFilesExports['client']; + if (this._internalFilesExports['client']) { + grpcClientModule = this._internalFilesExports['client'] as object; shimmer.wrap( grpcClientModule,