From 860134aaabf0f71cbf894fda40807d8c8ed6d597 Mon Sep 17 00:00:00 2001 From: Godfrey Chan Date: Thu, 30 Jan 2025 14:46:42 -0800 Subject: [PATCH] refactor(instrumentation-grpc): fix eslint warnings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ``` /home/runner/work/opentelemetry-js/opentelemetry-js/experimental/packages/opentelemetry-instrumentation-grpc/src/clientUtils.ts 105:42 warning Unexpected any. Specify a different type @typescript-eslint/no-explicit-any /home/runner/work/opentelemetry-js/opentelemetry-js/experimental/packages/opentelemetry-instrumentation-grpc/src/instrumentation.ts 129:69 warning Unexpected any. Specify a different type @typescript-eslint/no-explicit-any 134:69 warning Unexpected any. Specify a different type @typescript-eslint/no-explicit-any 139:68 warning Unexpected any. Specify a different type @typescript-eslint/no-explicit-any 144:68 warning Unexpected any. Specify a different type @typescript-eslint/no-explicit-any /home/runner/work/opentelemetry-js/opentelemetry-js/experimental/packages/opentelemetry-instrumentation-grpc/src/serverUtils.ts 112:23 warning Don't use `Function` as a type @typescript-eslint/ban-types 152:23 warning Don't use `Function` as a type @typescript-eslint/ban-types 207:31 warning Don't use `Function` as a type @typescript-eslint/ban-types 211:31 warning Don't use `Function` as a type @typescript-eslint/ban-types /home/runner/work/opentelemetry-js/opentelemetry-js/experimental/packages/opentelemetry-instrumentation-grpc/test/helper.ts 83:11 warning Don't use `Function` as a type @typescript-eslint/ban-types ``` This commit does not involve any changes to runtime code and can be safely merged without any concerns about changing behavior. The first can be replaced with the more precise type that we are expecting. The second just wansn't needed anymore. The third is highly unusual. After spending significant amount of time reading and stepping through the code – I am quite confident that the `.call({}, ...)` (and `.apply({}, ...)` elsewhere in the tests) are a mistake that we inherited from very old code. However I don't feel confident enough that I can *explain* what is going on here to make a change to the runtime code, so in the meantime, I think the best course of action is to leave it as-is, document the issue and have someone with more expertise untangle this down the road. The last one was a change in test code to use a more precise call signature. Ref #5365 --- .../src/clientUtils.ts | 2 +- .../src/instrumentation.ts | 8 ++++---- .../src/serverUtils.ts | 18 +++++++++++++++++- .../test/helper.ts | 2 +- 4 files changed, 23 insertions(+), 7 deletions(-) diff --git a/experimental/packages/opentelemetry-instrumentation-grpc/src/clientUtils.ts b/experimental/packages/opentelemetry-instrumentation-grpc/src/clientUtils.ts index 0974b239482..91aec18fddb 100644 --- a/experimental/packages/opentelemetry-instrumentation-grpc/src/clientUtils.ts +++ b/experimental/packages/opentelemetry-instrumentation-grpc/src/clientUtils.ts @@ -102,7 +102,7 @@ export function patchResponseMetadataEvent( call: EventEmitter, metadataCapture: metadataCaptureType ) { - call.on('metadata', (responseMetadata: any) => { + call.on('metadata', (responseMetadata: Metadata) => { metadataCapture.client.captureResponseMetadata(span, responseMetadata); }); } diff --git a/experimental/packages/opentelemetry-instrumentation-grpc/src/instrumentation.ts b/experimental/packages/opentelemetry-instrumentation-grpc/src/instrumentation.ts index 3daec0b63c5..cf2f9dd2a03 100644 --- a/experimental/packages/opentelemetry-instrumentation-grpc/src/instrumentation.ts +++ b/experimental/packages/opentelemetry-instrumentation-grpc/src/instrumentation.ts @@ -126,22 +126,22 @@ export class GrpcInstrumentation extends InstrumentationBase( endSpan(); }); - // Types of parameters 'call' and 'call' are incompatible. + // TODO: Investigate this call/signature – it was inherited from very old + // code and the `this: {}` is highly suspicious, and likely isn't doing + // anything useful. There is probably a more precise cast we can do here. + // eslint-disable-next-line @typescript-eslint/ban return (original as Function).call({}, call); } @@ -149,6 +152,11 @@ function clientStreamAndUnaryHandler( }; context.bind(context.active(), call); + + // TODO: Investigate this call/signature – it was inherited from very old + // code and the `this: {}` is highly suspicious, and likely isn't doing + // anything useful. There is probably a more precise cast we can do here. + // eslint-disable-next-line @typescript-eslint/ban return (original as Function).call({}, call, patchedCallback); } @@ -204,10 +212,18 @@ export function handleUntracedServerFunction( case 'unary': case 'clientStream': case 'client_stream': + // TODO: Investigate this call/signature – it was inherited from very old + // code and the `this: {}` is highly suspicious, and likely isn't doing + // anything useful. There is probably a more precise cast we can do here. + // eslint-disable-next-line @typescript-eslint/ban return (originalFunc as Function).call({}, call, callback); case 'serverStream': case 'server_stream': case 'bidi': + // TODO: Investigate this call/signature – it was inherited from very old + // code and the `this: {}` is highly suspicious, and likely isn't doing + // anything useful. There is probably a more precise cast we can do here. + // eslint-disable-next-line @typescript-eslint/ban return (originalFunc as Function).call({}, call); default: break; diff --git a/experimental/packages/opentelemetry-instrumentation-grpc/test/helper.ts b/experimental/packages/opentelemetry-instrumentation-grpc/test/helper.ts index aa6cc0b8d69..28cfd349327 100644 --- a/experimental/packages/opentelemetry-instrumentation-grpc/test/helper.ts +++ b/experimental/packages/opentelemetry-instrumentation-grpc/test/helper.ts @@ -80,7 +80,7 @@ type TestGrpcClient = Client & { interface TestGrpcCall { description: string; methodName: string; - method: Function; + method: (...args: any[]) => unknown; request: TestRequestResponse | TestRequestResponse[]; result: TestRequestResponse | TestRequestResponse[]; metadata?: Metadata;