From ea160d9c5c13fb32b39129397c43913d63c8b29f Mon Sep 17 00:00:00 2001 From: Abinet18 <35442169+Abinet18@users.noreply.github.com> Date: Wed, 5 Jul 2023 01:29:28 -0700 Subject: [PATCH] fix: add secureConnectionStart to https only (#3879) Co-authored-by: t2t2 Co-authored-by: Marc Pichler --- CHANGELOG.md | 1 + .../test/fetch.test.ts | 188 ++++++------- .../test/xhr.test.ts | 260 ++++++------------ .../opentelemetry-sdk-trace-web/src/utils.ts | 7 +- .../test/utils.test.ts | 11 +- 5 files changed, 188 insertions(+), 279 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7de3dc1f1b..42ca36c051 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -42,6 +42,7 @@ For experimental package changes, see the [experimental CHANGELOG](experimental/ ### :bug: (Bug Fix) +* fix(sdk-trace-web): add secureConnectionStart to https only [#3879](https://github.com/open-telemetry/opentelemetry-js/pull/3879) @Abinet18 * fix(http-instrumentation): stop listening to `request`'s `close` event once it has emitted `response` [#3625](https://github.com/open-telemetry/opentelemetry-js/pull/3625) @SimenB * fix(sdk-node): fix initialization in bundled environments by not loading @opentelemetry/exporter-jaeger [#3739](https://github.com/open-telemetry/opentelemetry-js/pull/3739) @pichlermarc diff --git a/experimental/packages/opentelemetry-instrumentation-fetch/test/fetch.test.ts b/experimental/packages/opentelemetry-instrumentation-fetch/test/fetch.test.ts index 318e0f98c3..14dcec9978 100644 --- a/experimental/packages/opentelemetry-instrumentation-fetch/test/fetch.test.ts +++ b/experimental/packages/opentelemetry-instrumentation-fetch/test/fetch.test.ts @@ -139,6 +139,19 @@ function createFakePerformanceObs(url: string) { return FakePerfObs; } +function testForCorrectEvents( + events: tracing.TimedEvent[], + eventNames: string[] +) { + for (let i = 0; i < events.length; i++) { + assert.strictEqual( + events[i].name, + eventNames[i], + `event ${eventNames[i]} is not defined` + ); + } +} + describe('fetch', () => { let contextManager: ZoneContextManager; let lastResponse: any | undefined; @@ -152,6 +165,7 @@ describe('fetch', () => { let fetchInstrumentation: FetchInstrumentation; const url = 'http://localhost:8090/get'; + const secureUrl = 'https://localhost:8090/get'; const badUrl = 'http://foo.bar.com/get'; const clearData = () => { @@ -399,53 +413,17 @@ describe('fetch', () => { it('span should have correct events', () => { const span: tracing.ReadableSpan = exportSpy.args[1][0][0]; const events = span.events; - assert.strictEqual(events.length, 9, 'number of events is wrong'); - - assert.strictEqual( - events[0].name, + assert.strictEqual(events.length, 8, 'number of events is wrong'); + testForCorrectEvents(events, [ PTN.FETCH_START, - `event ${PTN.FETCH_START} is not defined` - ); - assert.strictEqual( - events[1].name, PTN.DOMAIN_LOOKUP_START, - `event ${PTN.DOMAIN_LOOKUP_START} is not defined` - ); - assert.strictEqual( - events[2].name, PTN.DOMAIN_LOOKUP_END, - `event ${PTN.DOMAIN_LOOKUP_END} is not defined` - ); - assert.strictEqual( - events[3].name, PTN.CONNECT_START, - `event ${PTN.CONNECT_START} is not defined` - ); - assert.strictEqual( - events[4].name, - PTN.SECURE_CONNECTION_START, - `event ${PTN.SECURE_CONNECTION_START} is not defined` - ); - assert.strictEqual( - events[5].name, PTN.CONNECT_END, - `event ${PTN.CONNECT_END} is not defined` - ); - assert.strictEqual( - events[6].name, PTN.REQUEST_START, - `event ${PTN.REQUEST_START} is not defined` - ); - assert.strictEqual( - events[7].name, PTN.RESPONSE_START, - `event ${PTN.RESPONSE_START} is not defined` - ); - assert.strictEqual( - events[8].name, PTN.RESPONSE_END, - `event ${PTN.RESPONSE_END} is not defined` - ); + ]); }); it('should create a span for preflight request', () => { @@ -479,53 +457,17 @@ describe('fetch', () => { it('preflight request span should have correct events', () => { const span: tracing.ReadableSpan = exportSpy.args[0][0][0]; const events = span.events; - assert.strictEqual(events.length, 9, 'number of events is wrong'); - - assert.strictEqual( - events[0].name, + assert.strictEqual(events.length, 8, 'number of events is wrong'); + testForCorrectEvents(events, [ PTN.FETCH_START, - `event ${PTN.FETCH_START} is not defined` - ); - assert.strictEqual( - events[1].name, PTN.DOMAIN_LOOKUP_START, - `event ${PTN.DOMAIN_LOOKUP_START} is not defined` - ); - assert.strictEqual( - events[2].name, PTN.DOMAIN_LOOKUP_END, - `event ${PTN.DOMAIN_LOOKUP_END} is not defined` - ); - assert.strictEqual( - events[3].name, PTN.CONNECT_START, - `event ${PTN.CONNECT_START} is not defined` - ); - assert.strictEqual( - events[4].name, - PTN.SECURE_CONNECTION_START, - `event ${PTN.SECURE_CONNECTION_START} is not defined` - ); - assert.strictEqual( - events[5].name, PTN.CONNECT_END, - `event ${PTN.CONNECT_END} is not defined` - ); - assert.strictEqual( - events[6].name, PTN.REQUEST_START, - `event ${PTN.REQUEST_START} is not defined` - ); - assert.strictEqual( - events[7].name, PTN.RESPONSE_START, - `event ${PTN.RESPONSE_START} is not defined` - ); - assert.strictEqual( - events[8].name, PTN.RESPONSE_END, - `event ${PTN.RESPONSE_END} is not defined` - ); + ]); }); it('should set trace headers', () => { @@ -639,6 +581,51 @@ describe('fetch', () => { }); }); + describe('when request is secure and successful', () => { + beforeEach(async () => { + const propagateTraceHeaderCorsUrls = [secureUrl]; + await prepareData(secureUrl, { propagateTraceHeaderCorsUrls }); + }); + + afterEach(() => { + clearData(); + }); + + it('span should have correct events', () => { + const span: tracing.ReadableSpan = exportSpy.args[1][0][0]; + const events = span.events; + assert.strictEqual(events.length, 9, 'number of events is wrong'); + testForCorrectEvents(events, [ + PTN.FETCH_START, + PTN.DOMAIN_LOOKUP_START, + PTN.DOMAIN_LOOKUP_END, + PTN.CONNECT_START, + PTN.SECURE_CONNECTION_START, + PTN.CONNECT_END, + PTN.REQUEST_START, + PTN.RESPONSE_START, + PTN.RESPONSE_END, + ]); + }); + + it('preflight request span should have correct events', () => { + const span: tracing.ReadableSpan = exportSpy.args[0][0][0]; + const events = span.events; + assert.strictEqual(events.length, 9, 'number of events is wrong'); + testForCorrectEvents(events, [ + PTN.FETCH_START, + PTN.DOMAIN_LOOKUP_START, + PTN.DOMAIN_LOOKUP_END, + PTN.CONNECT_START, + PTN.SECURE_CONNECTION_START, + PTN.CONNECT_END, + PTN.REQUEST_START, + PTN.RESPONSE_START, + PTN.RESPONSE_END, + ]); + }); + }); + describe('applyCustomAttributesOnSpan option', () => { const prepare = async ( url: string, @@ -815,13 +802,18 @@ describe('fetch', () => { `Wrong number of spans: ${exportSpy.args.length}` ); - assert.strictEqual(events.length, 9, 'number of events is wrong'); + assert.strictEqual(events.length, 8, 'number of events is wrong'); - assert.strictEqual( - events[6].name, + testForCorrectEvents(events, [ + PTN.FETCH_START, + PTN.DOMAIN_LOOKUP_START, + PTN.DOMAIN_LOOKUP_END, + PTN.CONNECT_START, + PTN.CONNECT_END, PTN.REQUEST_START, - `event ${PTN.REQUEST_START} is not defined` - ); + PTN.RESPONSE_START, + PTN.RESPONSE_END, + ]); }); }); @@ -844,12 +836,17 @@ describe('fetch', () => { `Wrong number of spans: ${exportSpy.args.length}` ); - assert.strictEqual(events.length, 9, 'number of events is wrong'); - assert.strictEqual( - events[6].name, + assert.strictEqual(events.length, 8, 'number of events is wrong'); + testForCorrectEvents(events, [ + PTN.FETCH_START, + PTN.DOMAIN_LOOKUP_START, + PTN.DOMAIN_LOOKUP_END, + PTN.CONNECT_START, + PTN.CONNECT_END, PTN.REQUEST_START, - `event ${PTN.REQUEST_START} is not defined` - ); + PTN.RESPONSE_START, + PTN.RESPONSE_END, + ]); }); it('should have an absolute http.url attribute', () => { @@ -882,12 +879,17 @@ describe('fetch', () => { 2, `Wrong number of spans: ${exportSpy.args.length}` ); - assert.strictEqual(events.length, 9, 'number of events is wrong'); - assert.strictEqual( - events[6].name, + assert.strictEqual(events.length, 8, 'number of events is wrong'); + testForCorrectEvents(events, [ + PTN.FETCH_START, + PTN.DOMAIN_LOOKUP_START, + PTN.DOMAIN_LOOKUP_END, + PTN.CONNECT_START, + PTN.CONNECT_END, PTN.REQUEST_START, - `event ${PTN.REQUEST_START} is not defined` - ); + PTN.RESPONSE_START, + PTN.RESPONSE_END, + ]); }); }); diff --git a/experimental/packages/opentelemetry-instrumentation-xml-http-request/test/xhr.test.ts b/experimental/packages/opentelemetry-instrumentation-xml-http-request/test/xhr.test.ts index b304bb9c03..c3614a42e8 100644 --- a/experimental/packages/opentelemetry-instrumentation-xml-http-request/test/xhr.test.ts +++ b/experimental/packages/opentelemetry-instrumentation-xml-http-request/test/xhr.test.ts @@ -162,6 +162,19 @@ function createFakePerformanceObs(url: string) { return FakePerfObs; } +function testForCorrectEvents( + events: tracing.TimedEvent[], + eventNames: string[] +) { + for (let i = 0; i < events.length; i++) { + assert.strictEqual( + events[i].name, + eventNames[i], + `event ${eventNames[i]} is not defined` + ); + } +} + describe('xhr', () => { const asyncTests = [{ async: true }, { async: false }]; asyncTests.forEach(test => { @@ -200,6 +213,7 @@ describe('xhr', () => { let rootSpan: api.Span; let spyEntries: any; const url = 'http://localhost:8090/xml-http-request.js'; + const secureUrl = 'https://localhost:8090/xml-http-request.js'; let fakeNow = 0; let xmlHttpRequestInstrumentation: XMLHttpRequestInstrumentation; @@ -383,69 +397,20 @@ describe('xhr', () => { it('span should have correct events', () => { const span: tracing.ReadableSpan = exportSpy.args[1][0][0]; const events = span.events; - - assert.strictEqual( - events[0].name, + testForCorrectEvents(events, [ EventNames.METHOD_OPEN, - `event ${EventNames.METHOD_OPEN} is not defined` - ); - assert.strictEqual( - events[1].name, EventNames.METHOD_SEND, - `event ${EventNames.METHOD_SEND} is not defined` - ); - assert.strictEqual( - events[2].name, PTN.FETCH_START, - `event ${PTN.FETCH_START} is not defined` - ); - assert.strictEqual( - events[3].name, PTN.DOMAIN_LOOKUP_START, - `event ${PTN.DOMAIN_LOOKUP_START} is not defined` - ); - assert.strictEqual( - events[4].name, PTN.DOMAIN_LOOKUP_END, - `event ${PTN.DOMAIN_LOOKUP_END} is not defined` - ); - assert.strictEqual( - events[5].name, PTN.CONNECT_START, - `event ${PTN.CONNECT_START} is not defined` - ); - assert.strictEqual( - events[6].name, - PTN.SECURE_CONNECTION_START, - `event ${PTN.SECURE_CONNECTION_START} is not defined` - ); - assert.strictEqual( - events[7].name, PTN.CONNECT_END, - `event ${PTN.CONNECT_END} is not defined` - ); - assert.strictEqual( - events[8].name, PTN.REQUEST_START, - `event ${PTN.REQUEST_START} is not defined` - ); - assert.strictEqual( - events[9].name, PTN.RESPONSE_START, - `event ${PTN.RESPONSE_START} is not defined` - ); - assert.strictEqual( - events[10].name, PTN.RESPONSE_END, - `event ${PTN.RESPONSE_END} is not defined` - ); - assert.strictEqual( - events[11].name, EventNames.EVENT_LOAD, - `event ${EventNames.EVENT_LOAD} is not defined` - ); - - assert.strictEqual(events.length, 12, 'number of events is wrong'); + ]); + assert.strictEqual(events.length, 11, 'number of events is wrong'); }); it('should create a span for preflight request', () => { @@ -479,53 +444,17 @@ describe('xhr', () => { it('preflight request span should have correct events', () => { const span: tracing.ReadableSpan = exportSpy.args[0][0][0]; const events = span.events; - assert.strictEqual(events.length, 9, 'number of events is wrong'); - - assert.strictEqual( - events[0].name, + assert.strictEqual(events.length, 8, 'number of events is wrong'); + testForCorrectEvents(events, [ PTN.FETCH_START, - `event ${PTN.FETCH_START} is not defined` - ); - assert.strictEqual( - events[1].name, PTN.DOMAIN_LOOKUP_START, - `event ${PTN.DOMAIN_LOOKUP_START} is not defined` - ); - assert.strictEqual( - events[2].name, PTN.DOMAIN_LOOKUP_END, - `event ${PTN.DOMAIN_LOOKUP_END} is not defined` - ); - assert.strictEqual( - events[3].name, PTN.CONNECT_START, - `event ${PTN.CONNECT_START} is not defined` - ); - assert.strictEqual( - events[4].name, - PTN.SECURE_CONNECTION_START, - `event ${PTN.SECURE_CONNECTION_START} is not defined` - ); - assert.strictEqual( - events[5].name, PTN.CONNECT_END, - `event ${PTN.CONNECT_END} is not defined` - ); - assert.strictEqual( - events[6].name, PTN.REQUEST_START, - `event ${PTN.REQUEST_START} is not defined` - ); - assert.strictEqual( - events[7].name, PTN.RESPONSE_START, - `event ${PTN.RESPONSE_START} is not defined` - ); - assert.strictEqual( - events[8].name, PTN.RESPONSE_END, - `event ${PTN.RESPONSE_END} is not defined` - ); + ]); }); it('should NOT clear the resources', () => { @@ -534,6 +463,53 @@ describe('xhr', () => { 'resources have been cleared' ); }); + describe('When making https requests', () => { + beforeEach(done => { + clearData(); + // this won't generate a preflight span + const propagateTraceHeaderCorsUrls = [secureUrl]; + prepareData(done, secureUrl, { + propagateTraceHeaderCorsUrls, + }); + }); + + it('span should have correct events', () => { + const span: tracing.ReadableSpan = exportSpy.args[1][0][0]; + const events = span.events; + testForCorrectEvents(events, [ + EventNames.METHOD_OPEN, + EventNames.METHOD_SEND, + PTN.FETCH_START, + PTN.DOMAIN_LOOKUP_START, + PTN.DOMAIN_LOOKUP_END, + PTN.CONNECT_START, + PTN.SECURE_CONNECTION_START, + PTN.CONNECT_END, + PTN.REQUEST_START, + PTN.RESPONSE_START, + PTN.RESPONSE_END, + EventNames.EVENT_LOAD, + ]); + assert.strictEqual(events.length, 12, 'number of events is wrong'); + }); + + it('preflight request span should have correct events', () => { + const span: tracing.ReadableSpan = exportSpy.args[0][0][0]; + const events = span.events; + assert.strictEqual(events.length, 9, 'number of events is wrong'); + testForCorrectEvents(events, [ + PTN.FETCH_START, + PTN.DOMAIN_LOOKUP_START, + PTN.DOMAIN_LOOKUP_END, + PTN.CONNECT_START, + PTN.SECURE_CONNECTION_START, + PTN.CONNECT_END, + PTN.REQUEST_START, + PTN.RESPONSE_START, + PTN.RESPONSE_END, + ]); + }); + }); describe('AND origin match with window.location', () => { beforeEach(done => { @@ -786,11 +762,11 @@ describe('xhr', () => { assert.strictEqual( events.length, - 12, + 11, `number of events is wrong: ${events.length}` ); assert.strictEqual( - events[8].name, + events[7].name, PTN.REQUEST_START, `event ${PTN.REQUEST_START} is not defined` ); @@ -1008,67 +984,20 @@ describe('xhr', () => { const span: tracing.ReadableSpan = exportSpy.args[0][0][0]; const events = span.events; - assert.strictEqual( - events[0].name, + testForCorrectEvents(events, [ EventNames.METHOD_OPEN, - `event ${EventNames.METHOD_OPEN} is not defined` - ); - assert.strictEqual( - events[1].name, EventNames.METHOD_SEND, - `event ${EventNames.METHOD_SEND} is not defined` - ); - assert.strictEqual( - events[2].name, PTN.FETCH_START, - `event ${PTN.FETCH_START} is not defined` - ); - assert.strictEqual( - events[3].name, PTN.DOMAIN_LOOKUP_START, - `event ${PTN.DOMAIN_LOOKUP_START} is not defined` - ); - assert.strictEqual( - events[4].name, PTN.DOMAIN_LOOKUP_END, - `event ${PTN.DOMAIN_LOOKUP_END} is not defined` - ); - assert.strictEqual( - events[5].name, PTN.CONNECT_START, - `event ${PTN.CONNECT_START} is not defined` - ); - assert.strictEqual( - events[6].name, PTN.SECURE_CONNECTION_START, - `event ${PTN.SECURE_CONNECTION_START} is not defined` - ); - assert.strictEqual( - events[7].name, PTN.CONNECT_END, - `event ${PTN.CONNECT_END} is not defined` - ); - assert.strictEqual( - events[8].name, PTN.REQUEST_START, - `event ${PTN.REQUEST_START} is not defined` - ); - assert.strictEqual( - events[9].name, PTN.RESPONSE_START, - `event ${PTN.RESPONSE_START} is not defined` - ); - assert.strictEqual( - events[10].name, PTN.RESPONSE_END, - `event ${PTN.RESPONSE_END} is not defined` - ); - assert.strictEqual( - events[11].name, EventNames.EVENT_ERROR, - `event ${EventNames.EVENT_ERROR} is not defined` - ); - + ]); assert.strictEqual(events.length, 12, 'number of events is wrong'); }); }); @@ -1123,23 +1052,11 @@ describe('xhr', () => { it('span should have correct events', () => { const span: tracing.ReadableSpan = exportSpy.args[0][0][0]; const events = span.events; - - assert.strictEqual( - events[0].name, + testForCorrectEvents(events, [ EventNames.METHOD_OPEN, - `event ${EventNames.METHOD_OPEN} is not defined` - ); - assert.strictEqual( - events[1].name, EventNames.METHOD_SEND, - `event ${EventNames.METHOD_SEND} is not defined` - ); - assert.strictEqual( - events[2].name, EventNames.EVENT_ERROR, - `event ${EventNames.EVENT_ERROR} is not defined` - ); - + ]); assert.strictEqual(events.length, 3, 'number of events is wrong'); }); }); @@ -1201,23 +1118,11 @@ describe('xhr', () => { it('span should have correct events', () => { const span: tracing.ReadableSpan = exportSpy.args[0][0][0]; const events = span.events; - - assert.strictEqual( - events[0].name, + testForCorrectEvents(events, [ EventNames.METHOD_OPEN, - `event ${EventNames.METHOD_OPEN} is not defined` - ); - assert.strictEqual( - events[1].name, EventNames.METHOD_SEND, - `event ${EventNames.METHOD_SEND} is not defined` - ); - assert.strictEqual( - events[2].name, EventNames.EVENT_ABORT, - `event ${EventNames.EVENT_ABORT} is not defined` - ); - + ]); assert.strictEqual(events.length, 3, 'number of events is wrong'); }); }); @@ -1280,22 +1185,11 @@ describe('xhr', () => { const span: tracing.ReadableSpan = exportSpy.args[0][0][0]; const events = span.events; - assert.strictEqual( - events[0].name, + testForCorrectEvents(events, [ EventNames.METHOD_OPEN, - `event ${EventNames.METHOD_OPEN} is not defined` - ); - assert.strictEqual( - events[1].name, EventNames.METHOD_SEND, - `event ${EventNames.METHOD_SEND} is not defined` - ); - assert.strictEqual( - events[2].name, EventNames.EVENT_TIMEOUT, - `event ${EventNames.EVENT_TIMEOUT} is not defined` - ); - + ]); assert.strictEqual(events.length, 3, 'number of events is wrong'); }); }); diff --git a/packages/opentelemetry-sdk-trace-web/src/utils.ts b/packages/opentelemetry-sdk-trace-web/src/utils.ts index 2027abc42f..b3f583676f 100644 --- a/packages/opentelemetry-sdk-trace-web/src/utils.ts +++ b/packages/opentelemetry-sdk-trace-web/src/utils.ts @@ -85,7 +85,12 @@ export function addSpanNetworkEvents( addSpanNetworkEvent(span, PTN.DOMAIN_LOOKUP_START, resource); addSpanNetworkEvent(span, PTN.DOMAIN_LOOKUP_END, resource); addSpanNetworkEvent(span, PTN.CONNECT_START, resource); - addSpanNetworkEvent(span, PTN.SECURE_CONNECTION_START, resource); + if ( + hasKey(resource as PerformanceResourceTiming, 'name') && + (resource as PerformanceResourceTiming)['name'].startsWith('https:') + ) { + addSpanNetworkEvent(span, PTN.SECURE_CONNECTION_START, resource); + } addSpanNetworkEvent(span, PTN.CONNECT_END, resource); addSpanNetworkEvent(span, PTN.REQUEST_START, resource); addSpanNetworkEvent(span, PTN.RESPONSE_START, resource); diff --git a/packages/opentelemetry-sdk-trace-web/test/utils.test.ts b/packages/opentelemetry-sdk-trace-web/test/utils.test.ts index 06e550c1f0..dd00e6b061 100644 --- a/packages/opentelemetry-sdk-trace-web/test/utils.test.ts +++ b/packages/opentelemetry-sdk-trace-web/test/utils.test.ts @@ -116,9 +116,16 @@ describe('utils', () => { assert.strictEqual(addEventSpy.callCount, 0); addSpanNetworkEvents(span, entries); - - assert.strictEqual(addEventSpy.callCount, 9); assert.strictEqual(setAttributeSpy.callCount, 2); + //secure connect start should not be added to non-https resource + assert.strictEqual(addEventSpy.callCount, 8); + //secure connect start should be added to an https resource + addEventSpy.resetHistory(); + addSpanNetworkEvents(span, { + ...entries, + name: 'https://foo', + } as PerformanceResourceTiming); + assert.strictEqual(addEventSpy.callCount, 9); }); it('should only include encoded size when content encoding is being used', () => { const addEventSpy = sinon.spy();