From c18084fa5299400412ea5b2d12e1a3134169d795 Mon Sep 17 00:00:00 2001 From: Marten Hennoch Date: Tue, 20 Apr 2021 23:30:52 +0300 Subject: [PATCH 1/8] feat(instrumentation-xhr): add applyCustomAttributesOnSpan config option. --- .../src/xhr.ts | 29 ++ .../test/xhr.test.ts | 274 +++++++++++++----- 2 files changed, 230 insertions(+), 73 deletions(-) diff --git a/packages/opentelemetry-instrumentation-xml-http-request/src/xhr.ts b/packages/opentelemetry-instrumentation-xml-http-request/src/xhr.ts index ab9f4f4fa9..db0a55407c 100644 --- a/packages/opentelemetry-instrumentation-xml-http-request/src/xhr.ts +++ b/packages/opentelemetry-instrumentation-xml-http-request/src/xhr.ts @@ -19,6 +19,7 @@ import { isWrapped, InstrumentationBase, InstrumentationConfig, + safeExecuteInTheMiddle, } from '@opentelemetry/instrumentation'; import { hrTime, isUrlIgnored, otperformance } from '@opentelemetry/core'; import { SemanticAttributes } from '@opentelemetry/semantic-conventions'; @@ -45,6 +46,10 @@ import { AttributeNames } from './enums/AttributeNames'; // safe enough const OBSERVER_WAIT_TIME_MS = 300; +export interface XHRCustomAttributeFunction { + (span: api.Span, xhr: XMLHttpRequest): void; +} + /** * XMLHttpRequest config */ @@ -64,6 +69,8 @@ export interface XMLHttpRequestInstrumentationConfig * also not be traced. */ ignoreUrls?: Array; + /** Function for adding custom attributes on the span */ + applyCustomAttributesOnSpan?: XHRCustomAttributeFunction; } /** @@ -171,6 +178,24 @@ export class XMLHttpRequestInstrumentation extends InstrumentationBase applyCustomAttributesOnSpan(span, xhr), + error => { + if (!error) { + return; + } + + api.diag.error('applyCustomAttributesOnSpan', error); + }, + true + ); + } + } + /** * will collect information about all resources created * between "send" and "end" with additional waiting for main resource @@ -398,6 +423,10 @@ export class XMLHttpRequestInstrumentation extends InstrumentationBase { sinon.restore(); }; - prepareData = (done: any, fileUrl: string, config?: any) => { + prepareData = ( + done: any, + fileUrl: string, + config?: XMLHttpRequestInstrumentationConfig + ) => { const fakeXhr = sinon.useFakeXMLHttpRequest(); fakeXhr.onCreate = function (xhr: any) { requests.push(xhr); @@ -698,6 +705,29 @@ describe('xhr', () => { ); }); }); + + describe('and applyCustomAttributesOnSpan hook is configured', () => { + beforeEach(done => { + clearData(); + const propagateTraceHeaderCorsUrls = [url]; + prepareData(done, url, { + propagateTraceHeaderCorsUrls, + applyCustomAttributesOnSpan: function ( + span: api.Span, + xhr: XMLHttpRequest + ) { + const res = JSON.parse(xhr.response); + span.setAttribute('xhr-custom-attribute', res.foo); + }, + }); + }); + + it('span should have custom attribute', () => { + const span: tracing.ReadableSpan = exportSpy.args[1][0][0]; + const attributes = span.attributes; + assert.ok(attributes['xhr-custom-attribute'] === 'bar'); + }); + }); }); describe('when request is NOT successful', () => { @@ -711,7 +741,9 @@ describe('xhr', () => { 'https://raw.githubusercontent.com/open-telemetry/opentelemetry-js/master/package.json'; let fakeNow = 0; - beforeEach(() => { + const prepareData = function ( + config: XMLHttpRequestInstrumentationConfig = {} + ) { const fakeXhr = sinon.useFakeXMLHttpRequest(); fakeXhr.onCreate = function (xhr: any) { requests.push(xhr); @@ -738,7 +770,7 @@ describe('xhr', () => { webTracerWithZoneProvider = new WebTracerProvider(); registerInstrumentations({ - instrumentations: [new XMLHttpRequestInstrumentation()], + instrumentations: [new XMLHttpRequestInstrumentation(config)], tracerProvider: webTracerWithZoneProvider, }); @@ -750,37 +782,89 @@ describe('xhr', () => { webTracerWithZone = webTracerWithZoneProvider.getTracer('xhr-test'); rootSpan = webTracerWithZone.startSpan('root'); + }; + + beforeEach(() => { + prepareData(); }); afterEach(() => { clearData(); }); - describe('when request loads and receives an error code', () => { - beforeEach(done => { - api.context.with( - api.setSpan(api.context.active(), rootSpan), + function timedOutRequest(done: any) { + api.context.with(api.setSpan(api.context.active(), rootSpan), () => { + void getData( + new XMLHttpRequest(), + url, () => { - getData( - new XMLHttpRequest(), - url, - () => { - fakeNow = 100; - }, - testAsync - ).then(() => { - fakeNow = 0; - sinon.clock.tick(1000); - done(); - }); - assert.strictEqual(requests.length, 1, 'request not called'); - requests[0].respond( - 400, - { 'Content-Type': 'text/plain' }, - 'Bad Request' - ); + sinon.clock.tick(XHR_TIMEOUT); + }, + testAsync + ).then(() => { + fakeNow = 0; + sinon.clock.tick(1000); + done(); + }); + }); + } + + function abortedRequest(done: any) { + api.context.with(api.setSpan(api.context.active(), rootSpan), () => { + void getData(new XMLHttpRequest(), url, () => {}, testAsync).then( + () => { + fakeNow = 0; + sinon.clock.tick(1000); + done(); } ); + + assert.strictEqual(requests.length, 1, 'request not called'); + requests[0].abort(); + }); + } + + function erroredRequest(done: any) { + api.context.with(api.setSpan(api.context.active(), rootSpan), () => { + void getData( + new XMLHttpRequest(), + url, + () => { + fakeNow = 100; + }, + testAsync + ).then(() => { + fakeNow = 0; + sinon.clock.tick(1000); + done(); + }); + assert.strictEqual(requests.length, 1, 'request not called'); + requests[0].respond( + 400, + { 'Content-Type': 'text/plain' }, + 'Bad Request' + ); + }); + } + + function networkErrorRequest(done: any) { + api.context.with(api.setSpan(api.context.active(), rootSpan), () => { + void getData(new XMLHttpRequest(), url, () => {}, testAsync).then( + () => { + fakeNow = 0; + sinon.clock.tick(1000); + done(); + } + ); + + assert.strictEqual(requests.length, 1, 'request not called'); + requests[0].error(); + }); + } + + describe('when request loads and receives an error code', () => { + beforeEach(done => { + erroredRequest(done); }); it('span should have correct attributes', () => { const span: tracing.ReadableSpan = exportSpy.args[0][0][0]; @@ -900,21 +984,7 @@ describe('xhr', () => { describe('when request encounters a network error', () => { beforeEach(done => { - api.context.with( - api.setSpan(api.context.active(), rootSpan), - () => { - getData(new XMLHttpRequest(), url, () => {}, testAsync).then( - () => { - fakeNow = 0; - sinon.clock.tick(1000); - done(); - } - ); - - assert.strictEqual(requests.length, 1, 'request not called'); - requests[0].error(); - } - ); + networkErrorRequest(done); }); it('span should have correct attributes', () => { @@ -992,21 +1062,7 @@ describe('xhr', () => { }); beforeEach(done => { - api.context.with( - api.setSpan(api.context.active(), rootSpan), - () => { - getData(new XMLHttpRequest(), url, () => {}, testAsync).then( - () => { - fakeNow = 0; - sinon.clock.tick(1000); - done(); - } - ); - - assert.strictEqual(requests.length, 1, 'request not called'); - requests[0].abort(); - } - ); + abortedRequest(done); }); it('span should have correct attributes', () => { @@ -1084,23 +1140,7 @@ describe('xhr', () => { }); beforeEach(done => { - api.context.with( - api.setSpan(api.context.active(), rootSpan), - () => { - getData( - new XMLHttpRequest(), - url, - () => { - sinon.clock.tick(XHR_TIMEOUT); - }, - testAsync - ).then(() => { - fakeNow = 0; - sinon.clock.tick(1000); - done(); - }); - } - ); + timedOutRequest(done); }); it('span should have correct attributes', () => { @@ -1168,6 +1208,94 @@ describe('xhr', () => { assert.strictEqual(events.length, 3, 'number of events is wrong'); }); }); + + describe('when applyCustomAttributesOnSpan hook is present', () => { + describe('when request loads and receives an error code', () => { + beforeEach(done => { + clearData(); + prepareData({ + applyCustomAttributesOnSpan: function (span, xhr) { + span.setAttribute('xhr-custom-error-code', xhr.status); + }, + }); + erroredRequest(done); + }); + + it('span should have custom attribute', () => { + const span: tracing.ReadableSpan = exportSpy.args[0][0][0]; + const attributes = span.attributes; + assert.ok(attributes['xhr-custom-error-code'] === 400); + }); + }); + + describe('when request encounters a network error', () => { + beforeEach(done => { + clearData(); + prepareData({ + applyCustomAttributesOnSpan: function (span, xhr) { + span.setAttribute('xhr-custom-error-code', xhr.status); + }, + }); + networkErrorRequest(done); + }); + + it('span should have custom attribute', () => { + const span: tracing.ReadableSpan = exportSpy.args[0][0][0]; + const attributes = span.attributes; + assert.ok(attributes['xhr-custom-error-code'] === 0); + }); + }); + + describe('when request is aborted', () => { + before(function () { + // Can only abort Async requests + if (!testAsync) { + this.skip(); + } + }); + + beforeEach(done => { + clearData(); + prepareData({ + applyCustomAttributesOnSpan: function (span, xhr) { + span.setAttribute('xhr-custom-error-code', xhr.status); + }, + }); + abortedRequest(done); + }); + + it('span should have custom attribute', () => { + const span: tracing.ReadableSpan = exportSpy.args[0][0][0]; + const attributes = span.attributes; + assert.ok(attributes['xhr-custom-error-code'] === 0); + }); + }); + + describe('when request times out', () => { + before(function () { + // Can only set timeout for Async requests + if (!testAsync) { + this.skip(); + } + }); + + beforeEach(done => { + clearData(); + prepareData({ + applyCustomAttributesOnSpan: function (span, xhr) { + span.setAttribute('xhr-custom-error-code', xhr.status); + }, + }); + timedOutRequest(done); + }); + + it('span should have custom attribute', () => { + const span: tracing.ReadableSpan = exportSpy.args[0][0][0]; + const attributes = span.attributes; + assert.ok(attributes['xhr-custom-error-code'] === 0); + }); + }); + }); }); }); }); From 5f002b5de9ca9b80882cc5f1dba037fc19fa0e06 Mon Sep 17 00:00:00 2001 From: MartenH <72463136+mhennoch@users.noreply.github.com> Date: Wed, 21 Apr 2021 17:52:42 +0300 Subject: [PATCH 2/8] Use a type not an interface Co-authored-by: Daniel Dyla --- .../opentelemetry-instrumentation-xml-http-request/src/xhr.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/packages/opentelemetry-instrumentation-xml-http-request/src/xhr.ts b/packages/opentelemetry-instrumentation-xml-http-request/src/xhr.ts index db0a55407c..38e805da24 100644 --- a/packages/opentelemetry-instrumentation-xml-http-request/src/xhr.ts +++ b/packages/opentelemetry-instrumentation-xml-http-request/src/xhr.ts @@ -46,9 +46,7 @@ import { AttributeNames } from './enums/AttributeNames'; // safe enough const OBSERVER_WAIT_TIME_MS = 300; -export interface XHRCustomAttributeFunction { - (span: api.Span, xhr: XMLHttpRequest): void; -} +export type XHRCustomAttributeFunction = (span: api.Span, xhr: XMLHttpRequest) => void; /** * XMLHttpRequest config From 9b85f2c85992da02ab130c9afbf6ebe9e2ef1834 Mon Sep 17 00:00:00 2001 From: MartenH <72463136+mhennoch@users.noreply.github.com> Date: Wed, 21 Apr 2021 17:52:56 +0300 Subject: [PATCH 3/8] Check that hook is a function Co-authored-by: Daniel Dyla --- .../opentelemetry-instrumentation-xml-http-request/src/xhr.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/opentelemetry-instrumentation-xml-http-request/src/xhr.ts b/packages/opentelemetry-instrumentation-xml-http-request/src/xhr.ts index 38e805da24..381d0135a0 100644 --- a/packages/opentelemetry-instrumentation-xml-http-request/src/xhr.ts +++ b/packages/opentelemetry-instrumentation-xml-http-request/src/xhr.ts @@ -179,7 +179,7 @@ export class XMLHttpRequestInstrumentation extends InstrumentationBase applyCustomAttributesOnSpan(span, xhr), error => { From b5bbf47fe13c4a9d677ac8f9b73e4620b6c66a0c Mon Sep 17 00:00:00 2001 From: Marten Hennoch Date: Wed, 21 Apr 2021 18:01:41 +0300 Subject: [PATCH 4/8] fix: lint --- .../src/xhr.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/packages/opentelemetry-instrumentation-xml-http-request/src/xhr.ts b/packages/opentelemetry-instrumentation-xml-http-request/src/xhr.ts index 381d0135a0..fb7274cdce 100644 --- a/packages/opentelemetry-instrumentation-xml-http-request/src/xhr.ts +++ b/packages/opentelemetry-instrumentation-xml-http-request/src/xhr.ts @@ -46,7 +46,10 @@ import { AttributeNames } from './enums/AttributeNames'; // safe enough const OBSERVER_WAIT_TIME_MS = 300; -export type XHRCustomAttributeFunction = (span: api.Span, xhr: XMLHttpRequest) => void; +export type XHRCustomAttributeFunction = ( + span: api.Span, + xhr: XMLHttpRequest +) => void; /** * XMLHttpRequest config From cdce70d256f5e23509afac1b227fabb13fd342c3 Mon Sep 17 00:00:00 2001 From: MartenH <72463136+mhennoch@users.noreply.github.com> Date: Thu, 22 Apr 2021 20:30:27 +0300 Subject: [PATCH 5/8] Update packages/opentelemetry-instrumentation-xml-http-request/test/xhr.test.ts Co-authored-by: Bartlomiej Obecny --- .../test/xhr.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/opentelemetry-instrumentation-xml-http-request/test/xhr.test.ts b/packages/opentelemetry-instrumentation-xml-http-request/test/xhr.test.ts index 5df90cee23..8068dddbe1 100644 --- a/packages/opentelemetry-instrumentation-xml-http-request/test/xhr.test.ts +++ b/packages/opentelemetry-instrumentation-xml-http-request/test/xhr.test.ts @@ -1210,7 +1210,7 @@ describe('xhr', () => { }); describe('when applyCustomAttributesOnSpan hook is present', () => { - describe('when request loads and receives an error code', () => { + describe('AND request loads and receives an error code', () => { beforeEach(done => { clearData(); prepareData({ From 38646439915b6e59fb25cab7712d2e1627df5ff3 Mon Sep 17 00:00:00 2001 From: MartenH <72463136+mhennoch@users.noreply.github.com> Date: Thu, 22 Apr 2021 20:30:33 +0300 Subject: [PATCH 6/8] Update packages/opentelemetry-instrumentation-xml-http-request/test/xhr.test.ts Co-authored-by: Bartlomiej Obecny --- .../test/xhr.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/opentelemetry-instrumentation-xml-http-request/test/xhr.test.ts b/packages/opentelemetry-instrumentation-xml-http-request/test/xhr.test.ts index 8068dddbe1..cecdfccad1 100644 --- a/packages/opentelemetry-instrumentation-xml-http-request/test/xhr.test.ts +++ b/packages/opentelemetry-instrumentation-xml-http-request/test/xhr.test.ts @@ -1246,7 +1246,7 @@ describe('xhr', () => { }); }); - describe('when request is aborted', () => { + describe('AND request is aborted', () => { before(function () { // Can only abort Async requests if (!testAsync) { From e68e5bcbe6d241508a835b95e85fe815dd4304bb Mon Sep 17 00:00:00 2001 From: MartenH <72463136+mhennoch@users.noreply.github.com> Date: Thu, 22 Apr 2021 20:30:37 +0300 Subject: [PATCH 7/8] Update packages/opentelemetry-instrumentation-xml-http-request/test/xhr.test.ts Co-authored-by: Bartlomiej Obecny --- .../test/xhr.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/opentelemetry-instrumentation-xml-http-request/test/xhr.test.ts b/packages/opentelemetry-instrumentation-xml-http-request/test/xhr.test.ts index cecdfccad1..cdbafcf5f4 100644 --- a/packages/opentelemetry-instrumentation-xml-http-request/test/xhr.test.ts +++ b/packages/opentelemetry-instrumentation-xml-http-request/test/xhr.test.ts @@ -1228,7 +1228,7 @@ describe('xhr', () => { }); }); - describe('when request encounters a network error', () => { + describe('AND request encounters a network error', () => { beforeEach(done => { clearData(); prepareData({ From abbe3d8e991655b1f463349582dbf27e3f747e06 Mon Sep 17 00:00:00 2001 From: MartenH <72463136+mhennoch@users.noreply.github.com> Date: Thu, 22 Apr 2021 20:30:45 +0300 Subject: [PATCH 8/8] Update packages/opentelemetry-instrumentation-xml-http-request/test/xhr.test.ts Co-authored-by: Bartlomiej Obecny --- .../test/xhr.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/opentelemetry-instrumentation-xml-http-request/test/xhr.test.ts b/packages/opentelemetry-instrumentation-xml-http-request/test/xhr.test.ts index cdbafcf5f4..fa8bb22469 100644 --- a/packages/opentelemetry-instrumentation-xml-http-request/test/xhr.test.ts +++ b/packages/opentelemetry-instrumentation-xml-http-request/test/xhr.test.ts @@ -1271,7 +1271,7 @@ describe('xhr', () => { }); }); - describe('when request times out', () => { + describe('AND request times out', () => { before(function () { // Can only set timeout for Async requests if (!testAsync) {