From b4f04ae01da8925bdd828975f3271174347d7b16 Mon Sep 17 00:00:00 2001 From: Chengzhong Wu Date: Sun, 13 Nov 2022 23:42:26 +0800 Subject: [PATCH] feat(instrumentation-http): monitor error events with events.errorMonitor (#3402) Co-authored-by: Valentin Marchaud --- experimental/CHANGELOG.md | 2 + .../src/http.ts | 5 ++- .../test/functionals/http-enable.test.ts | 45 ++++++++++--------- .../test/functionals/https-enable.test.ts | 45 ++++++++++--------- 4 files changed, 51 insertions(+), 46 deletions(-) diff --git a/experimental/CHANGELOG.md b/experimental/CHANGELOG.md index 6c180c18d5..f0e7bf5ab3 100644 --- a/experimental/CHANGELOG.md +++ b/experimental/CHANGELOG.md @@ -8,6 +8,8 @@ All notable changes to experimental packages in this project will be documented ### :rocket: (Enhancement) +* feat(instrumentation-http): monitor error events with events.errorMonitor [#3402](https://github.com/open-telemetry/opentelemetry-js/pull/3402) @legendecas + ### :bug: (Bug Fix) ### :books: (Refine Doc) diff --git a/experimental/packages/opentelemetry-instrumentation-http/src/http.ts b/experimental/packages/opentelemetry-instrumentation-http/src/http.ts index 4129759589..71f3363f2d 100644 --- a/experimental/packages/opentelemetry-instrumentation-http/src/http.ts +++ b/experimental/packages/opentelemetry-instrumentation-http/src/http.ts @@ -54,6 +54,7 @@ import { safeExecuteInTheMiddle, } from '@opentelemetry/instrumentation'; import { RPCMetadata, RPCType, setRPCMetadata } from '@opentelemetry/core'; +import { errorMonitor } from 'events'; /** * Http instrumentation instrumentation for Opentelemetry @@ -361,7 +362,7 @@ export class HttpInstrumentation extends InstrumentationBase { this._closeHttpSpan(span, SpanKind.CLIENT, startTime, metricAttributes); }); - response.on('error', (error: Err) => { + response.on(errorMonitor, (error: Err) => { this._diag.debug('outgoingRequest on error()', error); utils.setSpanWithError(span, error); const code = utils.parseResponseStatus(SpanKind.CLIENT, response.statusCode); @@ -376,7 +377,7 @@ export class HttpInstrumentation extends InstrumentationBase { this._closeHttpSpan(span, SpanKind.CLIENT, startTime, metricAttributes); } }); - request.on('error', (error: Err) => { + request.on(errorMonitor, (error: Err) => { this._diag.debug('outgoingRequest on request error()', error); utils.setSpanWithError(span, error); this._closeHttpSpan(span, SpanKind.CLIENT, startTime, metricAttributes); diff --git a/experimental/packages/opentelemetry-instrumentation-http/test/functionals/http-enable.test.ts b/experimental/packages/opentelemetry-instrumentation-http/test/functionals/http-enable.test.ts index bee473ed3d..ecf5191427 100644 --- a/experimental/packages/opentelemetry-instrumentation-http/test/functionals/http-enable.test.ts +++ b/experimental/packages/opentelemetry-instrumentation-http/test/functionals/http-enable.test.ts @@ -670,21 +670,21 @@ describe('HttpInstrumentation', () => { ); req.setTimeout(10, () => { req.abort(); - reject('timeout'); + }); + // Instrumentation should not swallow error event. + assert.strictEqual(req.listeners('error').length, 0); + req.on('error', err => { + reject(err); }); return req.end(); }); - try { - await promiseRequest; - assert.fail(); - } catch (error) { - const spans = memoryExporter.getFinishedSpans(); - const [span] = spans; - assert.strictEqual(spans.length, 1); - assert.strictEqual(span.status.code, SpanStatusCode.ERROR); - assert.ok(Object.keys(span.attributes).length >= 6); - } + await assert.rejects(promiseRequest, /Error: socket hang up/); + const spans = memoryExporter.getFinishedSpans(); + const [span] = spans; + assert.strictEqual(spans.length, 1); + assert.strictEqual(span.status.code, SpanStatusCode.ERROR); + assert.ok(Object.keys(span.attributes).length >= 6); }); it('should have 1 ended span when request is aborted after receiving response', async () => { @@ -701,7 +701,7 @@ describe('HttpInstrumentation', () => { (resp: http.IncomingMessage) => { let data = ''; resp.on('data', chunk => { - req.destroy(Error()); + req.destroy(Error('request destroyed')); data += chunk; }); resp.on('end', () => { @@ -709,20 +709,21 @@ describe('HttpInstrumentation', () => { }); } ); + // Instrumentation should not swallow error event. + assert.strictEqual(req.listeners('error').length, 0); + req.on('error', err => { + reject(err); + }); return req.end(); }); - try { - await promiseRequest; - assert.fail(); - } catch (error) { - const spans = memoryExporter.getFinishedSpans(); - const [span] = spans; - assert.strictEqual(spans.length, 1); - assert.strictEqual(span.status.code, SpanStatusCode.ERROR); - assert.ok(Object.keys(span.attributes).length > 7); - } + await assert.rejects(promiseRequest, /Error: request destroyed/); + const spans = memoryExporter.getFinishedSpans(); + const [span] = spans; + assert.strictEqual(spans.length, 1); + assert.strictEqual(span.status.code, SpanStatusCode.ERROR); + assert.ok(Object.keys(span.attributes).length > 7); }); it("should have 1 ended client span when request doesn't listening response", done => { diff --git a/experimental/packages/opentelemetry-instrumentation-http/test/functionals/https-enable.test.ts b/experimental/packages/opentelemetry-instrumentation-http/test/functionals/https-enable.test.ts index 2611642b4c..5cfe4ac953 100644 --- a/experimental/packages/opentelemetry-instrumentation-http/test/functionals/https-enable.test.ts +++ b/experimental/packages/opentelemetry-instrumentation-http/test/functionals/https-enable.test.ts @@ -639,21 +639,21 @@ describe('HttpsInstrumentation', () => { ); req.setTimeout(10, () => { req.abort(); - reject('timeout'); + }); + // Instrumentation should not swallow error event. + assert.strictEqual(req.listeners('error').length, 0); + req.on('error', err => { + reject(err); }); return req.end(); }); - try { - await promiseRequest; - assert.fail(); - } catch (error) { - const spans = memoryExporter.getFinishedSpans(); - const [span] = spans; - assert.strictEqual(spans.length, 1); - assert.strictEqual(span.status.code, SpanStatusCode.ERROR); - assert.ok(Object.keys(span.attributes).length >= 6); - } + await assert.rejects(promiseRequest, /Error: socket hang up/); + const spans = memoryExporter.getFinishedSpans(); + const [span] = spans; + assert.strictEqual(spans.length, 1); + assert.strictEqual(span.status.code, SpanStatusCode.ERROR); + assert.ok(Object.keys(span.attributes).length >= 6); }); it('should have 1 ended span when request is aborted after receiving response', async () => { @@ -670,7 +670,7 @@ describe('HttpsInstrumentation', () => { (resp: http.IncomingMessage) => { let data = ''; resp.on('data', chunk => { - req.destroy(Error()); + req.destroy(Error('request destroyed')); data += chunk; }); resp.on('end', () => { @@ -678,20 +678,21 @@ describe('HttpsInstrumentation', () => { }); } ); + // Instrumentation should not swallow error event. + assert.strictEqual(req.listeners('error').length, 0); + req.on('error', err => { + reject(err); + }); return req.end(); }); - try { - await promiseRequest; - assert.fail(); - } catch (error) { - const spans = memoryExporter.getFinishedSpans(); - const [span] = spans; - assert.strictEqual(spans.length, 1); - assert.strictEqual(span.status.code, SpanStatusCode.ERROR); - assert.ok(Object.keys(span.attributes).length > 7); - } + await assert.rejects(promiseRequest, /Error: request destroyed/); + const spans = memoryExporter.getFinishedSpans(); + const [span] = spans; + assert.strictEqual(spans.length, 1); + assert.strictEqual(span.status.code, SpanStatusCode.ERROR); + assert.ok(Object.keys(span.attributes).length > 7); }); it("should have 1 ended span when response is listened by using req.on('response')", done => {