Handful of document-load fixes (#192)

* chore: use addSpanNetworkEvents from otel-web

Rather than use a modified copy, adjust event creation code
to use shared routine from otel-web.  This has the pleasant
side effect of also adding http.response_content_length.

* fix: bring resource fetch spans into spec compliance

Names should not be raw URLs, so I arbitrarily chose 'resourceFetch'
to mirror 'documentFetch' - I'm open to any better suggestions.  Because
the resource url is of course very useful I've put it in the 'http.url'
attribute instead.

* chore: lint:fix and use semantic constants

* fix: firefox sometimes has a fetchStart of 0, doesn't emit doc load spans

I noticed that in unit and manual tests Firefox would frequently not
emit doc load events.  Turns out that sometimes the fetchStart was 0
and the logic prevented spans from being emitted.  Simply checking
>= 0 rather than > 0 fixes that.

* chore: use constant for resourceFetch span name

Co-authored-by: Bartlomiej Obecny <[email protected]>

plugins/web/opentelemetry-plugin-document-load/package.json

@@ -74,6 +74,7 @@
   "dependencies": {
     "@opentelemetry/api": "^0.10.1",
     "@opentelemetry/core": "^0.10.1",
+    "@opentelemetry/semantic-conventions": "^0.10.1",
     "@opentelemetry/tracing": "^0.10.1",
     "@opentelemetry/web": "^0.10.1"
   }

plugins/web/opentelemetry-plugin-document-load/src/documentLoad.ts

@@ -28,13 +28,15 @@ import {
 } from '@opentelemetry/core';
 import {
   addSpanNetworkEvent,
+  addSpanNetworkEvents,
   hasKey,
   PerformanceEntries,
   PerformanceLegacy,
   PerformanceTimingNames as PTN,
 } from '@opentelemetry/web';
 import { AttributeNames } from './enums/AttributeNames';
 import { VERSION } from './version';
+import { HttpAttribute } from '@opentelemetry/semantic-conventions';
 
 /**
  * This class represents a document load plugin
@@ -81,21 +83,6 @@ export class DocumentLoad extends BasePlugin<unknown> {
     }
   }
 
-  /**
-   * Adds span network events
-   * @param span
-   * @param entries entries that contains performance information about resource
-   */
-  private _addSpanNetworkEvents(span: Span, entries: PerformanceEntries) {
-    addSpanNetworkEvent(span, PTN.DOMAIN_LOOKUP_START, entries);
-    addSpanNetworkEvent(span, PTN.DOMAIN_LOOKUP_END, entries);
-    addSpanNetworkEvent(span, PTN.CONNECT_START, entries);
-    addSpanNetworkEvent(span, PTN.SECURE_CONNECTION_START, entries);
-    addSpanNetworkEvent(span, PTN.CONNECT_END, entries);
-    addSpanNetworkEvent(span, PTN.REQUEST_START, entries);
-    addSpanNetworkEvent(span, PTN.RESPONSE_START, entries);
-  }
-
   /**
    * Collects information about performance and creates appropriate spans
    */
@@ -123,14 +110,15 @@ export class DocumentLoad extends BasePlugin<unknown> {
         );
         if (fetchSpan) {
           this._tracer.withSpan(fetchSpan, () => {
-            this._addSpanNetworkEvents(fetchSpan, entries);
+            addSpanNetworkEvents(fetchSpan, entries);
             this._endSpan(fetchSpan, PTN.RESPONSE_END, entries);
           });
         }
       });
 
       this._addResourcesSpans(rootSpan);
 
+      addSpanNetworkEvent(rootSpan, PTN.FETCH_START, entries);
       addSpanNetworkEvent(rootSpan, PTN.UNLOAD_EVENT_START, entries);
       addSpanNetworkEvent(rootSpan, PTN.UNLOAD_EVENT_END, entries);
       addSpanNetworkEvent(rootSpan, PTN.DOM_INTERACTIVE, entries);
@@ -142,6 +130,7 @@ export class DocumentLoad extends BasePlugin<unknown> {
       addSpanNetworkEvent(rootSpan, PTN.DOM_CONTENT_LOADED_EVENT_END, entries);
       addSpanNetworkEvent(rootSpan, PTN.DOM_COMPLETE, entries);
       addSpanNetworkEvent(rootSpan, PTN.LOAD_EVENT_START, entries);
+      addSpanNetworkEvent(rootSpan, PTN.LOAD_EVENT_END, entries);
 
       this._endSpan(rootSpan, PTN.LOAD_EVENT_END, entries);
     });
@@ -161,7 +150,6 @@ export class DocumentLoad extends BasePlugin<unknown> {
     // span can be undefined when entries are missing the certain performance - the span will not be created
     if (span) {
       if (hasKey(entries, performanceName)) {
-        addSpanNetworkEvent(span, performanceName, entries);
         span.end(entries[performanceName]);
       } else {
         // just end span
@@ -184,7 +172,7 @@ export class DocumentLoad extends BasePlugin<unknown> {
       keys.forEach((key: string) => {
         if (hasKey(performanceNavigationTiming, key)) {
           const value = performanceNavigationTiming[key];
-          if (typeof value === 'number' && value > 0) {
+          if (typeof value === 'number' && value >= 0) {
             entries[key] = value;
           }
         }
@@ -198,7 +186,7 @@ export class DocumentLoad extends BasePlugin<unknown> {
         keys.forEach((key: string) => {
           if (hasKey(performanceTiming, key)) {
             const value = performanceTiming[key];
-            if (typeof value === 'number' && value > 0) {
+            if (typeof value === 'number' && value >= 0) {
               entries[key] = value;
             }
           }
@@ -218,13 +206,14 @@ export class DocumentLoad extends BasePlugin<unknown> {
     spanOptions: SpanOptions = {}
   ) {
     const span = this._startSpan(
-      resource.name,
+      AttributeNames.RESOURCE_FETCH,
       PTN.FETCH_START,
       resource,
       spanOptions
     );
     if (span) {
-      this._addSpanNetworkEvents(span, resource);
+      span.setAttribute(HttpAttribute.HTTP_URL, resource.name);
+      addSpanNetworkEvents(span, resource);
       this._endSpan(span, PTN.RESPONSE_END, resource);
     }
   }
@@ -257,7 +246,6 @@ export class DocumentLoad extends BasePlugin<unknown> {
         )
       );
       span.setAttribute(AttributeNames.COMPONENT, this.component);
-      addSpanNetworkEvent(span, performanceName, entries);
       return span;
     }
     return undefined;

plugins/web/opentelemetry-plugin-document-load/src/enums/AttributeNames.ts

@@ -18,4 +18,5 @@ export enum AttributeNames {
   COMPONENT = 'component',
   DOCUMENT_LOAD = 'documentLoad',
   DOCUMENT_FETCH = 'documentFetch',
+  RESOURCE_FETCH = 'resourceFetch',
 }

plugins/web/opentelemetry-plugin-document-load/test/documentLoad.test.ts

@@ -40,6 +40,7 @@ import * as assert from 'assert';
 import * as sinon from 'sinon';
 import { ExportResult } from '@opentelemetry/core';
 import { DocumentLoad } from '../src';
+import { HttpAttribute } from '@opentelemetry/semantic-conventions';
 
 export class DummyExporter implements SpanExporter {
   export(
@@ -325,6 +326,11 @@ describe('DocumentLoad Plugin', () => {
         const fsEvents = fetchSpan.events;
 
         assert.strictEqual(rootSpan.name, 'documentFetch');
+        assert.ok(
+          (rootSpan.attributes[
+            HttpAttribute.HTTP_RESPONSE_CONTENT_LENGTH
+          ] as number) > 0
+        );
         assert.strictEqual(fetchSpan.name, 'documentLoad');
         ensureNetworkEventsExists(rsEvents);
 
@@ -418,11 +424,11 @@ describe('DocumentLoad Plugin', () => {
         const srEvents2 = spanResource2.events;
 
         assert.strictEqual(
-          spanResource1.name,
+          spanResource1.attributes[HttpAttribute.HTTP_URL],
           'http://localhost:8090/bundle.js'
         );
         assert.strictEqual(
-          spanResource2.name,
+          spanResource2.attributes[HttpAttribute.HTTP_URL],
           'http://localhost:8090/sockjs-node/info?t=1572620894466'
         );
 
@@ -454,7 +460,7 @@ describe('DocumentLoad Plugin', () => {
         const srEvents1 = spanResource1.events;
 
         assert.strictEqual(
-          spanResource1.name,
+          spanResource1.attributes[HttpAttribute.HTTP_URL],
           'http://localhost:8090/bundle.js'
         );