fix: connect's error handling middleware not called properly (opentel… (

#1076)

Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com>
Co-authored-by: Gerhard Stöbich <deb2001-github@yahoo.de>
Co-authored-by: Valentin Marchaud <contact@vmarchaud.fr>

plugins/node/opentelemetry-instrumentation-connect/src/instrumentation.ts

@@ -78,7 +78,7 @@ export class ConnectInstrumentation extends InstrumentationBase<Server> {
 
   public _patchNext(next: NextFunction, finishSpan: () => void): NextFunction {
     return function nextFunction(this: NextFunction, err?: any): void {
-      const result = next.apply(this, err);
+      const result = next.apply(this, [err]);
       finishSpan();
       return result;
     };
@@ -114,13 +114,18 @@ export class ConnectInstrumentation extends InstrumentationBase<Server> {
     middleWare: HandleFunction
   ): HandleFunction {
     const instrumentation = this;
-    return function (this: Use): void {
+    const isErrorMiddleware = middleWare.length === 4;
+
+    function patchedMiddleware(this: Use): void {
       if (!instrumentation.isEnabled()) {
         return (middleWare as any).apply(this, arguments);
       }
-      const req = arguments[0] as IncomingMessage;
-      const res = arguments[1] as ServerResponse;
-      const next = arguments[2] as NextFunction;
+      const [reqArgIdx, resArgIdx, nextArgIdx] = isErrorMiddleware
+        ? [1, 2, 3]
+        : [0, 1, 2];
+      const req = arguments[reqArgIdx] as IncomingMessage;
+      const res = arguments[resArgIdx] as ServerResponse;
+      const next = arguments[nextArgIdx] as NextFunction;
 
       const rpcMetadata = getRPCMetadata(context.active());
       if (routeName && rpcMetadata?.type === RPCType.HTTP) {
@@ -150,10 +155,18 @@ export class ConnectInstrumentation extends InstrumentationBase<Server> {
       }
 
       res.addListener('close', finishSpan);
-      arguments[2] = instrumentation._patchNext(next, finishSpan);
+      arguments[nextArgIdx] = instrumentation._patchNext(next, finishSpan);
 
       return (middleWare as any).apply(this, arguments);
-    };
+    }
+
+    Object.defineProperty(patchedMiddleware, 'length', {
+      value: middleWare.length,
+      writable: false,
+      configurable: true,
+    });
+
+    return patchedMiddleware;
   }
 
   public _patchUse(original: Server['use']): Use {

plugins/node/opentelemetry-instrumentation-connect/test/instrumentation.test.ts

@@ -101,6 +101,36 @@ describe('connect', () => {
     });
   });
   describe('when connect is enabled', () => {
+    it('should invoke error middleware properly', async () => {
+      const errorHandler = (
+        err: any,
+        req: connect.IncomingMessage,
+        res: http.ServerResponse,
+        next: connect.NextFunction
+      ) => {
+        res.end('recovered');
+      };
+
+      const middlewareGeneratingError = (
+        req: connect.IncomingMessage,
+        res: http.ServerResponse,
+        next: connect.NextFunction
+      ) => {
+        next(new Error('boom!'));
+      };
+
+      app.use(middlewareGeneratingError);
+      app.use(errorHandler);
+
+      const response = await httpRequest.get(`http://localhost:${PORT}/`);
+
+      const errorExpression = new RegExp('.*error.*', 'ig');
+      assert(!errorExpression.test(response as string));
+
+      const recoverExpression = new RegExp('.*recovered.*', 'ig');
+      assert(recoverExpression.test(response as string));
+    });
+
     it('should generate span for anonymous middleware', async () => {
       app.use((req, res, next) => {
         next();