fix(instrumentation-fetch): perf observer bugs

packages/opentelemetry-instrumentation-fetch/src/fetch.ts

@@ -32,7 +32,7 @@ import { VERSION } from './version';
 // hard to say how long it should really wait, seems like 300ms is
 // safe enough
 const OBSERVER_WAIT_TIME_MS = 300;
-
+const urlNormalizingA = document.createElement('a');
 /**
  * FetchPlugin Config
  */
@@ -252,11 +252,10 @@ export class FetchInstrumentation extends InstrumentationBase<
     response: FetchResponse
   ) {
     const endTime = core.hrTime();
-    spanData.observer?.disconnect();
-
     this._addFinalSpanAttributes(span, response);
 
     setTimeout(() => {
+      spanData.observer?.disconnect();
       this._findResourceAndAddNetworkEvents(span, spanData, endTime);
       this._tasksCount--;
       this._clearResources();
@@ -352,15 +351,18 @@ export class FetchInstrumentation extends InstrumentationBase<
   private _prepareSpanData(spanUrl: string): SpanData {
     const startTime = core.hrTime();
     const entries: PerformanceResourceTiming[] = [];
-
     if (typeof window.PerformanceObserver === 'undefined') {
       return { entries, startTime, spanUrl };
     }
 
     const observer: PerformanceObserver = new PerformanceObserver(list => {
-      const entries = list.getEntries() as PerformanceResourceTiming[];
-      entries.forEach(entry => {
-        if (entry.initiatorType === 'fetch' && entry.name === spanUrl) {
+      const perfObsEntries = list.getEntries() as PerformanceResourceTiming[];
+      urlNormalizingA.href = spanUrl;
+      perfObsEntries.forEach(entry => {
+        if (
+          entry.initiatorType === 'fetch' &&
+          entry.name === urlNormalizingA.href
+        ) {
           entries.push(entry);
         }
       });

packages/opentelemetry-instrumentation-fetch/test/fetch.test.ts

@@ -96,6 +96,36 @@ function createMainResource(resource = {}): PerformanceResourceTiming {
   return mainResource;
 }
 
+function createFakePerformanceObs(url: string) {
+  class FakePerfObs implements PerformanceObserver {
+    constructor(private readonly cb: PerformanceObserverCallback) {}
+    observe() {
+      const absoluteUrl = url.startsWith('http') ? url : location.origin + url;
+      const resources: PerformanceObserverEntryList = {
+        getEntries(): PerformanceEntryList {
+          return [
+            createResource({ name: absoluteUrl }) as any,
+            createMainResource({ name: absoluteUrl }) as any,
+          ];
+        },
+        getEntriesByName(): PerformanceEntryList {
+          return [];
+        },
+        getEntriesByType(): PerformanceEntryList {
+          return [];
+        },
+      };
+      this.cb(resources, this);
+    }
+    disconnect() {}
+    takeRecords(): PerformanceEntryList {
+      return [];
+    }
+  }
+
+  return FakePerfObs;
+}
+
 describe('fetch', () => {
   let sandbox: sinon.SinonSandbox;
   let contextManager: ZoneContextManager;
@@ -169,7 +199,12 @@ describe('fetch', () => {
 
     if (disablePerfObserver) {
       sandbox.stub(window, 'PerformanceObserver').value(undefined);
+    } else {
+      sandbox
+        .stub(window, 'PerformanceObserver')
+        .value(createFakePerformanceObs(fileUrl));
     }
+
     if (disableGetEntries) {
       sandbox.stub(performance, 'getEntriesByType').value(undefined);
     } else {
@@ -597,6 +632,64 @@ describe('fetch', () => {
     });
   });
 
+  describe('when PerformanceObserver is used by default', () => {
+    beforeEach(done => {
+      // All above tests test it already but just in case
+      // lets explicitly turn getEntriesByType off so we can be sure
+      // that the perf entries come from the observer.
+      prepareData(done, url, {}, undefined, false, true);
+    });
+    afterEach(() => {
+      clearData();
+    });
+    it('should create both spans with network events', () => {
+      const span: tracing.ReadableSpan = exportSpy.args[0][0][0];
+      const events = span.events;
+
+      assert.strictEqual(
+        exportSpy.args.length,
+        2,
+        `Wrong number of spans: ${exportSpy.args.length}`
+      );
+
+      assert.strictEqual(events.length, 9, 'number of events is wrong');
+
+      assert.strictEqual(
+        events[6].name,
+        PTN.REQUEST_START,
+        `event ${PTN.REQUEST_START} is not defined`
+      );
+    });
+  });
+
+  describe('when fetching with relative url', () => {
+    beforeEach(done => {
+      prepareData(done, '/get', {}, undefined, false, true);
+    });
+    afterEach(() => {
+      clearData();
+    });
+    it('should create spans with network info', () => {
+      // no prefetch span because mock observer uses location.origin as url when relative
+      // and prefetch span finding compares url origins
+      const span: tracing.ReadableSpan = exportSpy.args[0][0][0];
+      const events = span.events;
+
+      assert.strictEqual(
+        exportSpy.args.length,
+        1,
+        `Wrong number of spans: ${exportSpy.args.length}`
+      );
+
+      assert.strictEqual(events.length, 9, 'number of events is wrong');
+      assert.strictEqual(
+        events[6].name,
+        PTN.REQUEST_START,
+        `event ${PTN.REQUEST_START} is not defined`
+      );
+    });
+  });
+
   describe('when PerformanceObserver is undefined', () => {
     beforeEach(done => {
       prepareData(done, url, {}, undefined, true, false);
@@ -606,12 +699,21 @@ describe('fetch', () => {
       clearData();
     });
 
-    it('should still create spans', () => {
+    it('should fallback to getEntries', () => {
+      const span: tracing.ReadableSpan = exportSpy.args[0][0][0];
+      const events = span.events;
+
       assert.strictEqual(
         exportSpy.args.length,
         2,
         `Wrong number of spans: ${exportSpy.args.length}`
       );
+      assert.strictEqual(events.length, 9, 'number of events is wrong');
+      assert.strictEqual(
+        events[6].name,
+        PTN.REQUEST_START,
+        `event ${PTN.REQUEST_START} is not defined`
+      );
     });
   });
 
@@ -622,7 +724,12 @@ describe('fetch', () => {
     afterEach(() => {
       clearData();
     });
-    it('should capture fetch without preflight', () => {
+    it('should still capture fetch with basic attributes', () => {
+      const span: tracing.ReadableSpan = exportSpy.args[0][0][0];
+      const events = span.events;
+      const attributes = span.attributes;
+      const keys = Object.keys(attributes);
+
       assert.strictEqual(
         exportSpy.args.length,
         1,
@@ -633,6 +740,15 @@ describe('fetch', () => {
         'HTTP GET',
         'wrong span captured'
       );
+
+      assert.strictEqual(events.length, 0, 'Should not have any events');
+
+      // should still have basic attributes
+      assert.strictEqual(
+        attributes[keys[3]],
+        200,
+        `Missing basic attribute ${HttpAttribute.HTTP_STATUS_CODE}`
+      );
     });
   });
 });