fix(plugin-express): fix double span end #908

open-telemetry · Apr 9, 2020 · b8f94e0 · b8f94e0
1 parent 43bb5a4
commit b8f94e0
Show file tree

Hide file tree

Showing 3 changed files with 12 additions and 43 deletions.
diff --git a/packages/opentelemetry-plugin-express/README.md b/packages/opentelemetry-plugin-express/README.md
@@ -45,6 +45,10 @@ const provider = new NodeTracerProvider();
 
 See [examples/express](https://github.com/open-telemetry/opentelemetry-js/tree/master/examples/express) for a short example.
 
+### Caveats
+
+Because of the way express works, it's hard to correctly compute the time took that each span took to complete. For this reason the time you'll see reported for each span will **not** represent the time it really took.
+
 ### Express Plugin Options
 
 Express plugin has few options available to choose from. You can set the following:

diff --git a/packages/opentelemetry-plugin-express/src/express.ts b/packages/opentelemetry-plugin-express/src/express.ts
@@ -30,12 +30,7 @@ import {
   ExpressPluginConfig,
   ExpressLayerType,
 } from './types';
-import {
-  getLayerMetadata,
-  storeLayerPath,
-  patchEnd,
-  isLayerIgnored,
-} from './utils';
+import { getLayerMetadata, storeLayerPath, isLayerIgnored } from './utils';
 import { VERSION } from './version';
 
 /**
@@ -190,27 +185,20 @@ export class ExpressPlugin extends BasePlugin<typeof express> {
         const span = plugin._tracer.startSpan(metadata.name, {
           attributes: Object.assign(attributes, metadata.attributes),
         });
+        span.end();
         // verify we have a callback
-        let callbackIdx = Array.from(arguments).findIndex(
-          arg => typeof arg === 'function'
-        );
-        let callbackHasBeenCalled = false;
+        const args = Array.from(arguments);
+        const callbackIdx = args.findIndex(arg => typeof arg === 'function');
         if (callbackIdx >= 0) {
           arguments[callbackIdx] = function() {
-            callbackHasBeenCalled = true;
             if (!(req.route && arguments[0] instanceof Error)) {
               (req[_LAYERS_STORE_PROPERTY] as string[]).pop();
             }
-            return patchEnd(span, plugin._tracer.bind(next))();
+            const callback = args[callbackIdx] as Function;
+            return plugin._tracer.bind(callback).apply(this, arguments);
           };
         }
-        const result = original.apply(this, arguments);
-        // if the layer return a response, the callback will never
-        // be called, so we need to manually close the span
-        if (callbackHasBeenCalled === false) {
-          span.end();
-        }
-        return result;
+        return original.apply(this, arguments);
       };
     });
   }

diff --git a/packages/opentelemetry-plugin-express/src/utils.ts b/packages/opentelemetry-plugin-express/src/utils.ts
@@ -14,7 +14,7 @@
  * limitations under the License.
  */
 
-import { CanonicalCode, Span, Attributes } from '@opentelemetry/api';
+import { Attributes } from '@opentelemetry/api';
 import {
   ExpressLayer,
   AttributeNames,
@@ -80,29 +80,6 @@ export const getLayerMetadata = (
   }
 };
 
-/**
- * Ends a created span.
- * @param span The created span to end.
- * @param resultHandler A callback function.
- */
-export const patchEnd = (span: Span, resultHandler: Function): Function => {
-  return function patchedEnd(this: {}, ...args: unknown[]) {
-    const error = args[0];
-    if (error instanceof Error) {
-      span.setStatus({
-        code: CanonicalCode.INTERNAL,
-        message: error.message,
-      });
-    } else {
-      span.setStatus({
-        code: CanonicalCode.OK,
-      });
-    }
-    span.end();
-    return resultHandler.apply(this, args);
-  };
-};
-
 /**
  * Check whether the given obj match pattern
  * @param constant e.g URL of request