From 8e25785e3ad8c7dec5f785339a74a7559326eb8e Mon Sep 17 00:00:00 2001 From: CptSchnitz <12687466+CptSchnitz@users.noreply.github.com> Date: Sat, 24 Jul 2021 19:14:52 +0300 Subject: [PATCH 1/5] fix(hapi-instrumentation): close spans on errors in instrumented functions --- .../src/instrumentation.ts | 45 +++++++++++++------ .../test/hapi-server-ext.test.ts | 38 +++++++++++++++- .../test/hapi.test.ts | 42 ++++++++++++++++- 3 files changed, 110 insertions(+), 15 deletions(-) diff --git a/plugins/node/opentelemetry-instrumentation-hapi/src/instrumentation.ts b/plugins/node/opentelemetry-instrumentation-hapi/src/instrumentation.ts index c9b11755a9..223c503aed 100644 --- a/plugins/node/opentelemetry-instrumentation-hapi/src/instrumentation.ts +++ b/plugins/node/opentelemetry-instrumentation-hapi/src/instrumentation.ts @@ -337,15 +337,25 @@ 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 { + let res; + await api.context.with( + api.trace.setSpan(api.context.active(), span), + async () => { + res = await method(...params); + } + ); + return res; + } 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 +396,19 @@ 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 { + const res = await oldHandler(request, h, err); + return res; + } 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 index f941789a30..9c01c8652c 100644 --- 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 index 23f21e8f34..7508c2138f 100644 --- 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', () => { From 72dde067574d4f9f9d0f854f41478148b72f0808 Mon Sep 17 00:00:00 2001 From: CptSchnitz <12687466+CptSchnitz@users.noreply.github.com> Date: Wed, 28 Jul 2021 19:07:19 +0300 Subject: [PATCH 2/5] refactor(hapi-instrumentation): removed uneeded assignment --- .../src/instrumentation.ts | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/plugins/node/opentelemetry-instrumentation-hapi/src/instrumentation.ts b/plugins/node/opentelemetry-instrumentation-hapi/src/instrumentation.ts index 223c503aed..ec18f76fd2 100644 --- a/plugins/node/opentelemetry-instrumentation-hapi/src/instrumentation.ts +++ b/plugins/node/opentelemetry-instrumentation-hapi/src/instrumentation.ts @@ -397,9 +397,8 @@ export class HapiInstrumentation extends InstrumentationBase { attributes: metadata.attributes, }); try { - const res = await oldHandler(request, h, err); - return res; - } catch (err) { + return await oldHandler(request, h, err); + } catch (err) { span.recordException(err); span.setStatus({ code: api.SpanStatusCode.ERROR, From 582748b6b043049cf2543c27a19600ca51a8e39e Mon Sep 17 00:00:00 2001 From: CptSchnitz <12687466+CptSchnitz@users.noreply.github.com> Date: Wed, 28 Jul 2021 19:15:20 +0300 Subject: [PATCH 3/5] refactor(hapi-instrumentation): simplified the use of context.with --- .../src/instrumentation.ts | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/plugins/node/opentelemetry-instrumentation-hapi/src/instrumentation.ts b/plugins/node/opentelemetry-instrumentation-hapi/src/instrumentation.ts index ec18f76fd2..0fdc3b8473 100644 --- a/plugins/node/opentelemetry-instrumentation-hapi/src/instrumentation.ts +++ b/plugins/node/opentelemetry-instrumentation-hapi/src/instrumentation.ts @@ -338,14 +338,10 @@ export class HapiInstrumentation extends InstrumentationBase { attributes: metadata.attributes, }); try { - let res; - await api.context.with( + return await api.context.with( api.trace.setSpan(api.context.active(), span), - async () => { - res = await method(...params); - } + () => method(...params) ); - return res; } catch (err) { span.recordException(err); span.setStatus({ @@ -398,7 +394,7 @@ export class HapiInstrumentation extends InstrumentationBase { }); try { return await oldHandler(request, h, err); - } catch (err) { + } catch (err) { span.recordException(err); span.setStatus({ code: api.SpanStatusCode.ERROR, From 17f7eb27c5f6d0133a728607b788ebed7facdf5f Mon Sep 17 00:00:00 2001 From: CptSchnitz <12687466+CptSchnitz@users.noreply.github.com> Date: Mon, 9 Aug 2021 19:24:44 +0300 Subject: [PATCH 4/5] refactor(hapi-instrumentation): changed context with to avoid closure --- .../src/instrumentation.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/plugins/node/opentelemetry-instrumentation-hapi/src/instrumentation.ts b/plugins/node/opentelemetry-instrumentation-hapi/src/instrumentation.ts index 0fdc3b8473..83d3657d07 100644 --- a/plugins/node/opentelemetry-instrumentation-hapi/src/instrumentation.ts +++ b/plugins/node/opentelemetry-instrumentation-hapi/src/instrumentation.ts @@ -338,9 +338,11 @@ export class HapiInstrumentation extends InstrumentationBase { attributes: metadata.attributes, }); try { - return await api.context.with( + return await api.context.with, Hapi.Lifecycle.Method>( api.trace.setSpan(api.context.active(), span), - () => method(...params) + method, + undefined, + ...params ); } catch (err) { span.recordException(err); From aa104f528d30a8011e783f86b0c254e273765628 Mon Sep 17 00:00:00 2001 From: CptSchnitz <12687466+CptSchnitz@users.noreply.github.com> Date: Mon, 9 Aug 2021 20:32:06 +0300 Subject: [PATCH 5/5] style(hapi-instrumentation): lint --- .../src/instrumentation.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/plugins/node/opentelemetry-instrumentation-hapi/src/instrumentation.ts b/plugins/node/opentelemetry-instrumentation-hapi/src/instrumentation.ts index 83d3657d07..25cfcba838 100644 --- a/plugins/node/opentelemetry-instrumentation-hapi/src/instrumentation.ts +++ b/plugins/node/opentelemetry-instrumentation-hapi/src/instrumentation.ts @@ -338,7 +338,10 @@ export class HapiInstrumentation extends InstrumentationBase { attributes: metadata.attributes, }); try { - return await api.context.with, Hapi.Lifecycle.Method>( + return await api.context.with< + Parameters, + Hapi.Lifecycle.Method + >( api.trace.setSpan(api.context.active(), span), method, undefined,