From 0c3a820b1bdb22123c220123e6f3700c44aed487 Mon Sep 17 00:00:00 2001 From: Mark Wolff Date: Thu, 3 Oct 2019 15:02:13 -0700 Subject: [PATCH 1/3] fix(grpc): patch loadPackageDefinition --- .../opentelemetry-plugin-grpc/package.json | 1 + .../opentelemetry-plugin-grpc/src/grpc.ts | 86 ++++++++++++++++--- .../test/grpc.test.ts | 8 +- 3 files changed, 78 insertions(+), 17 deletions(-) diff --git a/packages/opentelemetry-plugin-grpc/package.json b/packages/opentelemetry-plugin-grpc/package.json index 7ea39b1444..a5076ecae3 100644 --- a/packages/opentelemetry-plugin-grpc/package.json +++ b/packages/opentelemetry-plugin-grpc/package.json @@ -38,6 +38,7 @@ "access": "public" }, "devDependencies": { + "@grpc/proto-loader": "^0.4.0", "@types/mocha": "^5.2.7", "@types/node": "^12.6.9", "@types/shimmer": "^1.0.1", diff --git a/packages/opentelemetry-plugin-grpc/src/grpc.ts b/packages/opentelemetry-plugin-grpc/src/grpc.ts index aa93a534c0..31ebcc9da8 100644 --- a/packages/opentelemetry-plugin-grpc/src/grpc.ts +++ b/packages/opentelemetry-plugin-grpc/src/grpc.ts @@ -60,10 +60,12 @@ export class GrpcPlugin extends BasePlugin { this._config = {}; } - protected readonly _internalFilesList: ModuleExportsMapping = { - '0.13 - 1.6': { client: 'src/node/src/client.js' }, - '^1.7': { client: 'src/client.js' }, - }; + // TODO: uncomment once makeClientConstructor patch is uncommented + // protected readonly _internalFilesList: ModuleExportsMapping = { + // '0.13 - 1.6': { client: 'src/node/src/client.js' }, + // '^1.7': { client: 'src/client.js' }, + // }; + protected readonly _basedir = basedir; protected patch(): typeof grpcTypes { @@ -73,6 +75,20 @@ export class GrpcPlugin extends BasePlugin { this.version ); + // TODO: uncomment on completion: https://github.com/open-telemetry/opentelemetry-js/issues/285 + // if (this._internalFilesExports['client']) { + // grpcClientModule = this._internalFilesExports[ + // 'client' + // ] as GrpcInternalClientTypes; + + // // Wrap the internally used client constructor + // shimmer.wrap( + // grpcClientModule, + // 'makeClientConstructor', + // this._patchClient() + // ); + // } + if (this._moduleExports.Server) { shimmer.wrap( this._moduleExports.Server.prototype, @@ -91,16 +107,10 @@ export class GrpcPlugin extends BasePlugin { ); } - if (this._internalFilesExports['client']) { - grpcClientModule = this._internalFilesExports[ - 'client' - ] as GrpcInternalClientTypes; - - // Wrap the internally used client constructor - shimmer.wrap( - grpcClientModule, - 'makeClientConstructor', - this._patchClient() + if (this._moduleExports.loadPackageDefinition) { + shimmer.wrap(this._moduleExports, + 'loadPackageDefinition', + this._patchLoadPackageDefinition() ); } @@ -121,6 +131,12 @@ export class GrpcPlugin extends BasePlugin { shimmer.unwrap(this._moduleExports, 'makeGenericClientConstructor'); } + if (this._moduleExports.loadPackageDefinition) { + shimmer.unwrap(this._moduleExports, + 'loadPackageDefinition' + ); + } + if (grpcClientModule) { shimmer.unwrap(grpcClientModule, 'makeClientConstructor'); } @@ -321,6 +337,48 @@ export class GrpcPlugin extends BasePlugin { return (original as any).call(self, call); } + private _patchLoadPackageDefinition() { + const plugin = this; + return (original: typeof grpcTypes.loadPackageDefinition) => { + plugin._logger.debug('patching loadPackageDefinition'); + return function loadPackageDefinition(this: grpc, packageDef: grpcTypes.PackageDefinition) { + const result = original.apply(this, arguments as never); + // Copied from exports.loadPackageDefintion(...) + for (const serviceFqn in packageDef) { + const nameComponents = serviceFqn.split('.'); + const serviceName = nameComponents[nameComponents.length-1]; + let current = result; + for (const packageName of nameComponents.slice(0, -1)) { + if (!current[packageName]) { + current[packageName] = {}; + } + current = current[packageName] as grpcTypes.GrpcObject; + } + + // if makeClientConstructor was used, patch the client + if (current[serviceName].prototype instanceof plugin._moduleExports.Client) { + const serviceClient = current[serviceName] as grpcTypes.ProtobufMessage; + const methodList: string[] = []; + (Object.values(serviceClient.prototype.$method_names) as string[]).forEach(method => { + const originalName = serviceClient.service[method as string].originalName + + // e.g. push both "Capitalize" and "capitalize" + originalName && methodList.push(originalName); + methodList.push(method); + }); + + shimmer.massWrap( + serviceClient.prototype, + methodList as never[], + plugin._getPatchedClientMethods() as never + ); + } + } + return result; + } + } + } + private _patchClient() { const plugin = this; return (original: typeof grpcTypes.makeGenericClientConstructor): never => { diff --git a/packages/opentelemetry-plugin-grpc/test/grpc.test.ts b/packages/opentelemetry-plugin-grpc/test/grpc.test.ts index 36a763ddd7..8380f6c0bb 100644 --- a/packages/opentelemetry-plugin-grpc/test/grpc.test.ts +++ b/packages/opentelemetry-plugin-grpc/test/grpc.test.ts @@ -32,6 +32,7 @@ import * as grpc from 'grpc'; import * as sinon from 'sinon'; const PROTO_PATH = __dirname + '/fixtures/grpc-test.proto'; +const PROTO_OPTIONS = { keepCae: true, enums: String, defaults: true, oneofs: true }; const memoryExporter = new InMemorySpanExporter(); type GrpcModule = typeof grpc; @@ -299,7 +300,7 @@ describe('GrpcPlugin', () => { it('should patch client constructor makeClientConstructor() and makeGenericClientConstructor()', () => { plugin.enable(grpc, new NoopTracer(), new NoopLogger()); - (plugin['_moduleExports'] as any).makeGenericClientConstructor({}); + (grpc as any).makeGenericClientConstructor({}); assert.strictEqual(clientPatchStub.callCount, 1); }); }); @@ -536,8 +537,9 @@ describe('GrpcPlugin', () => { // TODO: add plugin options here once supported }; plugin.enable(grpc, tracer, logger, config); - - const proto = grpc.load(PROTO_PATH).pkg_test; + const protoLoader = require('@grpc/proto-loader'); + const definition = protoLoader.loadSync(PROTO_PATH, PROTO_OPTIONS); + const proto = grpc.loadPackageDefinition(definition).pkg_test; server = startServer(grpc, proto); client = createClient(grpc, proto); }); From 383351e575d1107c3ff7512b49838a89ca8523cf Mon Sep 17 00:00:00 2001 From: Mark Wolff Date: Thu, 3 Oct 2019 17:12:12 -0700 Subject: [PATCH 2/3] fix: linting --- .../opentelemetry-plugin-grpc/src/grpc.ts | 35 ++++++++++++------- .../test/grpc.test.ts | 7 +++- 2 files changed, 28 insertions(+), 14 deletions(-) diff --git a/packages/opentelemetry-plugin-grpc/src/grpc.ts b/packages/opentelemetry-plugin-grpc/src/grpc.ts index 31ebcc9da8..437e5c4e4e 100644 --- a/packages/opentelemetry-plugin-grpc/src/grpc.ts +++ b/packages/opentelemetry-plugin-grpc/src/grpc.ts @@ -26,7 +26,6 @@ import { import { AttributeNames } from './enums/AttributeNames'; import { grpc, - ModuleExportsMapping, GrpcPluginOptions, ServerCallWithMeta, SendUnaryDataCallback, @@ -108,7 +107,8 @@ export class GrpcPlugin extends BasePlugin { } if (this._moduleExports.loadPackageDefinition) { - shimmer.wrap(this._moduleExports, + shimmer.wrap( + this._moduleExports, 'loadPackageDefinition', this._patchLoadPackageDefinition() ); @@ -132,9 +132,7 @@ export class GrpcPlugin extends BasePlugin { } if (this._moduleExports.loadPackageDefinition) { - shimmer.unwrap(this._moduleExports, - 'loadPackageDefinition' - ); + shimmer.unwrap(this._moduleExports, 'loadPackageDefinition'); } if (grpcClientModule) { @@ -341,12 +339,15 @@ export class GrpcPlugin extends BasePlugin { const plugin = this; return (original: typeof grpcTypes.loadPackageDefinition) => { plugin._logger.debug('patching loadPackageDefinition'); - return function loadPackageDefinition(this: grpc, packageDef: grpcTypes.PackageDefinition) { + return function loadPackageDefinition( + this: grpc, + packageDef: grpcTypes.PackageDefinition + ) { const result = original.apply(this, arguments as never); // Copied from exports.loadPackageDefintion(...) for (const serviceFqn in packageDef) { const nameComponents = serviceFqn.split('.'); - const serviceName = nameComponents[nameComponents.length-1]; + const serviceName = nameComponents[nameComponents.length - 1]; let current = result; for (const packageName of nameComponents.slice(0, -1)) { if (!current[packageName]) { @@ -356,11 +357,19 @@ export class GrpcPlugin extends BasePlugin { } // if makeClientConstructor was used, patch the client - if (current[serviceName].prototype instanceof plugin._moduleExports.Client) { - const serviceClient = current[serviceName] as grpcTypes.ProtobufMessage; + if ( + current[serviceName].prototype instanceof + plugin._moduleExports.Client + ) { + const serviceClient = current[ + serviceName + ] as grpcTypes.ProtobufMessage; const methodList: string[] = []; - (Object.values(serviceClient.prototype.$method_names) as string[]).forEach(method => { - const originalName = serviceClient.service[method as string].originalName + (Object.values( + serviceClient.prototype.$method_names + ) as string[]).forEach(method => { + const originalName = + serviceClient.service[method as string].originalName; // e.g. push both "Capitalize" and "capitalize" originalName && methodList.push(originalName); @@ -375,8 +384,8 @@ export class GrpcPlugin extends BasePlugin { } } return result; - } - } + }; + }; } private _patchClient() { diff --git a/packages/opentelemetry-plugin-grpc/test/grpc.test.ts b/packages/opentelemetry-plugin-grpc/test/grpc.test.ts index 8380f6c0bb..88e7ca0309 100644 --- a/packages/opentelemetry-plugin-grpc/test/grpc.test.ts +++ b/packages/opentelemetry-plugin-grpc/test/grpc.test.ts @@ -32,7 +32,12 @@ import * as grpc from 'grpc'; import * as sinon from 'sinon'; const PROTO_PATH = __dirname + '/fixtures/grpc-test.proto'; -const PROTO_OPTIONS = { keepCae: true, enums: String, defaults: true, oneofs: true }; +const PROTO_OPTIONS = { + keepCae: true, + enums: String, + defaults: true, + oneofs: true, +}; const memoryExporter = new InMemorySpanExporter(); type GrpcModule = typeof grpc; From 5f69165b570214eae108e5cb74ce06e9de03141b Mon Sep 17 00:00:00 2001 From: Mark Wolff Date: Mon, 7 Oct 2019 11:15:22 -0700 Subject: [PATCH 3/3] fix: span trying to be ended twice --- packages/opentelemetry-plugin-grpc/src/grpc.ts | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/packages/opentelemetry-plugin-grpc/src/grpc.ts b/packages/opentelemetry-plugin-grpc/src/grpc.ts index 437e5c4e4e..860a53f39e 100644 --- a/packages/opentelemetry-plugin-grpc/src/grpc.ts +++ b/packages/opentelemetry-plugin-grpc/src/grpc.ts @@ -538,12 +538,15 @@ export class GrpcPlugin extends BasePlugin { ((call as unknown) as events.EventEmitter).on( 'status', (status: Status) => { - span.setStatus({ code: CanonicalCode.OK }); - span.setAttribute( - AttributeNames.GRPC_STATUS_CODE, - status.code.toString() - ); - endSpan(); + // if an error was emitted, the span will be ended there + if (status.code === 0) { + span.setStatus({ code: CanonicalCode.OK }); + span.setAttribute( + AttributeNames.GRPC_STATUS_CODE, + status.code.toString() + ); + endSpan(); + } } ); }