fix(hapi-instrumentation): close spans on errors in instrumented func…

…tions (#589) * fix(hapi-instrumentation): close spans on errors in instrumented functions * refactor(hapi-instrumentation): removed uneeded assignment * refactor(hapi-instrumentation): simplified the use of context.with * refactor(hapi-instrumentation): changed context with to avoid closure * style(hapi-instrumentation): lint Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com>
open-telemetry · Aug 9, 2021 · e95221b · e95221b
1 parent e8c207b
commit e95221b
Show file tree

Hide file tree

Showing 3 changed files with 110 additions and 15 deletions.
diff --git a/plugins/node/opentelemetry-instrumentation-hapi/src/instrumentation.ts b/plugins/node/opentelemetry-instrumentation-hapi/src/instrumentation.ts
@@ -337,15 +337,26 @@ export class HapiInstrumentation extends InstrumentationBase {
         const span = instrumentation.tracer.startSpan(metadata.name, {
           attributes: metadata.attributes,
         });
-        let res;
-        await api.context.with(
-          api.trace.setSpan(api.context.active(), span),
-          async () => {
-            res = await method(...params);
-          }
-        );
-        span.end();
-        return res;
+        try {
+          return await api.context.with<
+            Parameters<Hapi.Lifecycle.Method>,
+            Hapi.Lifecycle.Method
+          >(
+            api.trace.setSpan(api.context.active(), span),
+            method,
+            undefined,
+            ...params
+          );
+        } catch (err) {
+          span.recordException(err);
+          span.setStatus({
+            code: api.SpanStatusCode.ERROR,
+            message: err.message,
+          });
+          throw err;
+        } finally {
+          span.end();
+        }
       };
       return newHandler as T;
     }
@@ -386,10 +397,18 @@ export class HapiInstrumentation extends InstrumentationBase {
         const span = instrumentation.tracer.startSpan(metadata.name, {
           attributes: metadata.attributes,
         });
-        const res = await oldHandler(request, h, err);
-        span.end();
-
-        return res;
+        try {
+          return await oldHandler(request, h, err);
+        } catch (err) {
+          span.recordException(err);
+          span.setStatus({
+            code: api.SpanStatusCode.ERROR,
+            message: err.message,
+          });
+          throw err;
+        } finally {
+          span.end();
+        }
       };
       if (route.options?.handler) {
         route.options.handler = newHandler;

diff --git a/plugins/node/opentelemetry-instrumentation-hapi/test/hapi-server-ext.test.ts b/plugins/node/opentelemetry-instrumentation-hapi/test/hapi-server-ext.test.ts
@@ -14,7 +14,7 @@
  * limitations under the License.
  */
 
-import { context, trace } from '@opentelemetry/api';
+import { context, SpanStatusCode, trace } from '@opentelemetry/api';
 import { NodeTracerProvider } from '@opentelemetry/node';
 import { AsyncHooksContextManager } from '@opentelemetry/context-async-hooks';
 import {
@@ -371,5 +371,41 @@ describe('Hapi Instrumentation - Server.Ext Tests', () => {
       assert.strictEqual(res.statusCode, 200);
       assert.deepStrictEqual(memoryExporter.getFinishedSpans().length, 0);
     });
+
+    it('should end span and record the error if an error is thrown in ext', async () => {
+      const errorMessage = 'error';
+      const rootSpan = tracer.startSpan('rootSpan');
+      const extension: hapi.ServerExtEventsRequestObject = {
+        type: 'onRequest',
+        method: async (request, h, err) => {
+          throw new Error(errorMessage);
+        },
+      };
+      server.ext(extension);
+      await server.start();
+      assert.strictEqual(memoryExporter.getFinishedSpans().length, 0);
+
+      await context.with(
+        trace.setSpan(context.active(), rootSpan),
+        async () => {
+          const res = await server.inject({
+            method: 'GET',
+            url: '/test',
+          });
+          assert.strictEqual(res.statusCode, 500);
+
+          rootSpan.end();
+          assert.deepStrictEqual(memoryExporter.getFinishedSpans().length, 2);
+          const extHandlerSpan = memoryExporter
+            .getFinishedSpans()
+            .find(span => span.name === 'ext - onRequest');
+
+          assert.notStrictEqual(extHandlerSpan, undefined);
+          assert.strictEqual(extHandlerSpan?.events[0].name, 'exception');
+          assert.strictEqual(extHandlerSpan.status.code, SpanStatusCode.ERROR);
+          assert.strictEqual(extHandlerSpan.status.message, errorMessage);
+        }
+      );
+    });
   });
 });
diff --git a/plugins/node/opentelemetry-instrumentation-hapi/test/hapi.test.ts b/plugins/node/opentelemetry-instrumentation-hapi/test/hapi.test.ts
@@ -14,7 +14,7 @@
  * limitations under the License.
  */
 
-import { context, trace } from '@opentelemetry/api';
+import { context, trace, SpanStatusCode } from '@opentelemetry/api';
 import { RPCType, setRPCMetadata } from '@opentelemetry/core';
 import { SemanticAttributes } from '@opentelemetry/semantic-conventions';
 import { NodeTracerProvider } from '@opentelemetry/node';
@@ -364,6 +364,46 @@ describe('Hapi Instrumentation - Core Tests', () => {
         }
       );
     });
+
+    it('should end span and record the error if an error is thrown in route handler', async () => {
+      const errorMessage = 'error';
+      const rootSpan = tracer.startSpan('rootSpan', {});
+      server.route({
+        method: 'GET',
+        path: '/users/{userId}',
+        handler: (request, h) => {
+          throw new Error(errorMessage);
+        },
+      });
+
+      await server.start();
+      assert.strictEqual(memoryExporter.getFinishedSpans().length, 0);
+      const rpcMetadata = { type: RPCType.HTTP, span: rootSpan };
+      await context.with(
+        setRPCMetadata(trace.setSpan(context.active(), rootSpan), rpcMetadata),
+        async () => {
+          const res = await server.inject({
+            method: 'GET',
+            url: '/users/1',
+          });
+          assert.strictEqual(res.statusCode, 500);
+
+          rootSpan.end();
+          assert.deepStrictEqual(memoryExporter.getFinishedSpans().length, 2);
+
+          const requestHandlerSpan = memoryExporter
+            .getFinishedSpans()
+            .find(span => span.name === 'route - /users/{userId}');
+          assert.notStrictEqual(requestHandlerSpan, undefined);
+          assert.strictEqual(requestHandlerSpan?.events[0].name, 'exception');
+          assert.strictEqual(
+            requestHandlerSpan.status.code,
+            SpanStatusCode.ERROR
+          );
+          assert.strictEqual(requestHandlerSpan.status.message, errorMessage);
+        }
+      );
+    });
   });
 
   describe('Disabling Hapi instrumentation', () => {