fix: add secureConnectionStart to https only (#3879)

Co-authored-by: t2t2 <taavot@gmail.com> Co-authored-by: Marc Pichler <marc.pichler@dynatrace.com>
open-telemetry · Jul 5, 2023 · ea160d9 · ea160d9
1 parent dccd906
commit ea160d9
Show file tree

Hide file tree

Showing 5 changed files with 188 additions and 279 deletions.
diff --git 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
@@ -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,
+      ]);
     });
   });