fix(instrumentation-fetch, instrumentation-xml-http-request) content …

…length attributes now prese (#5230)
open-telemetry · Dec 12, 2024 · e4d9c21 · e4d9c21
1 parent bdee949
commit e4d9c21
Show file tree

Hide file tree

Showing 7 changed files with 96 additions and 27 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -19,6 +19,8 @@ For semantic convention package changes, see the [semconv CHANGELOG](packages/se
 
 * fix(sdk-trace-base): do not load OTEL_ env vars on module load, but when needed [#5224](https://github.com/open-telemetry/opentelemetry-js/pull/5224)
 
+* fix(instrumentation-xhr, instrumentation-fetch): content length attributes no longer get removed with `ignoreNetworkEvents: true` being set [#5229](https://github.com/open-telemetry/opentelemetry-js/issues/5229)
+
 ### :books: (Refine Doc)
 
 ### :house: (Internal)

diff --git a/experimental/packages/opentelemetry-instrumentation-fetch/src/fetch.ts b/experimental/packages/opentelemetry-instrumentation-fetch/src/fetch.ts
@@ -112,9 +112,11 @@ export class FetchInstrumentation extends InstrumentationBase<FetchInstrumentati
       },
       api.trace.setSpan(api.context.active(), span)
     );
-    if (!this.getConfig().ignoreNetworkEvents) {
-      web.addSpanNetworkEvents(childSpan, corsPreFlightRequest);
-    }
+    web.addSpanNetworkEvents(
+      childSpan,
+      corsPreFlightRequest,
+      this.getConfig().ignoreNetworkEvents
+    );
     childSpan.end(
       corsPreFlightRequest[web.PerformanceTimingNames.RESPONSE_END]
     );
@@ -262,9 +264,11 @@ export class FetchInstrumentation extends InstrumentationBase<FetchInstrumentati
         this._addChildSpan(span, corsPreFlightRequest);
         this._markResourceAsUsed(corsPreFlightRequest);
       }
-      if (!this.getConfig().ignoreNetworkEvents) {
-        web.addSpanNetworkEvents(span, mainRequest);
-      }
+      web.addSpanNetworkEvents(
+        span,
+        mainRequest,
+        this.getConfig().ignoreNetworkEvents
+      );
     }
   }
 

diff --git a/experimental/packages/opentelemetry-instrumentation-fetch/test/fetch.test.ts b/experimental/packages/opentelemetry-instrumentation-fetch/test/fetch.test.ts
@@ -1212,5 +1212,18 @@ describe('fetch', () => {
       const events = span.events;
       assert.strictEqual(events.length, 0, 'number of events is wrong');
     });
+
+    it('should still add the CONTENT_LENGTH attribute', () => {
+      const span: tracing.ReadableSpan = exportSpy.args[1][0][0];
+      const attributes = span.attributes;
+      const responseContentLength = attributes[
+        SEMATTRS_HTTP_RESPONSE_CONTENT_LENGTH
+      ] as number;
+      assert.strictEqual(
+        responseContentLength,
+        30,
+        `attributes ${SEMATTRS_HTTP_RESPONSE_CONTENT_LENGTH} is <= 0`
+      );
+    });
   });
 });
diff --git a/experimental/packages/opentelemetry-instrumentation-xml-http-request/src/xhr.ts b/experimental/packages/opentelemetry-instrumentation-xml-http-request/src/xhr.ts
@@ -149,9 +149,11 @@ export class XMLHttpRequestInstrumentation extends InstrumentationBase<XMLHttpRe
       const childSpan = this.tracer.startSpan('CORS Preflight', {
         startTime: corsPreFlightRequest[PTN.FETCH_START],
       });
-      if (!this.getConfig().ignoreNetworkEvents) {
-        addSpanNetworkEvents(childSpan, corsPreFlightRequest);
-      }
+      addSpanNetworkEvents(
+        childSpan,
+        corsPreFlightRequest,
+        this.getConfig().ignoreNetworkEvents
+      );
       childSpan.end(corsPreFlightRequest[PTN.RESPONSE_END]);
     });
   }
@@ -300,9 +302,11 @@ export class XMLHttpRequestInstrumentation extends InstrumentationBase<XMLHttpRe
         this._addChildSpan(span, corsPreFlightRequest);
         this._markResourceAsUsed(corsPreFlightRequest);
       }
-      if (!this.getConfig().ignoreNetworkEvents) {
-        addSpanNetworkEvents(span, mainRequest);
-      }
+      addSpanNetworkEvents(
+        span,
+        mainRequest,
+        this.getConfig().ignoreNetworkEvents
+      );
     }
   }
 

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
@@ -857,6 +857,19 @@ describe('xhr', () => {
             const events = span.events;
             assert.strictEqual(events.length, 3, 'number of events is wrong');
           });
+
+          it('should still add the CONTENT_LENGTH attribute', () => {
+            const span: tracing.ReadableSpan = exportSpy.args[1][0][0];
+            const attributes = span.attributes;
+            const responseContentLength = attributes[
+              SEMATTRS_HTTP_RESPONSE_CONTENT_LENGTH
+            ] as number;
+            assert.strictEqual(
+              responseContentLength,
+              30,
+              `attributes ${SEMATTRS_HTTP_RESPONSE_CONTENT_LENGTH} is <= 0`
+            );
+          });
         });
       });
 

diff --git a/packages/opentelemetry-sdk-trace-web/src/utils.ts b/packages/opentelemetry-sdk-trace-web/src/utils.ts
@@ -89,28 +89,32 @@ export function addSpanNetworkEvent(
 }
 
 /**
- * Helper function for adding network events
+ * Helper function for adding network events and content length attributes
  * @param span
  * @param resource
+ * @param ignoreNetworkEvents
  */
 export function addSpanNetworkEvents(
   span: api.Span,
-  resource: PerformanceEntries
+  resource: PerformanceEntries,
+  ignoreNetworkEvents = false
 ): void {
-  addSpanNetworkEvent(span, PTN.FETCH_START, resource);
-  addSpanNetworkEvent(span, PTN.DOMAIN_LOOKUP_START, resource);
-  addSpanNetworkEvent(span, PTN.DOMAIN_LOOKUP_END, resource);
-  addSpanNetworkEvent(span, PTN.CONNECT_START, resource);
-  if (
-    hasKey(resource as PerformanceResourceTiming, 'name') &&
-    (resource as PerformanceResourceTiming)['name'].startsWith('https:')
-  ) {
-    addSpanNetworkEvent(span, PTN.SECURE_CONNECTION_START, resource);
+  if (!ignoreNetworkEvents) {
+    addSpanNetworkEvent(span, PTN.FETCH_START, resource);
+    addSpanNetworkEvent(span, PTN.DOMAIN_LOOKUP_START, resource);
+    addSpanNetworkEvent(span, PTN.DOMAIN_LOOKUP_END, resource);
+    addSpanNetworkEvent(span, PTN.CONNECT_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);
+    addSpanNetworkEvent(span, PTN.RESPONSE_END, resource);
   }
-  addSpanNetworkEvent(span, PTN.CONNECT_END, resource);
-  addSpanNetworkEvent(span, PTN.REQUEST_START, resource);
-  addSpanNetworkEvent(span, PTN.RESPONSE_START, resource);
-  addSpanNetworkEvent(span, PTN.RESPONSE_END, resource);
   const encodedLength = resource[PTN.ENCODED_BODY_SIZE];
   if (encodedLength !== undefined) {
     span.setAttribute(SEMATTRS_HTTP_RESPONSE_CONTENT_LENGTH, encodedLength);

diff --git a/packages/opentelemetry-sdk-trace-web/test/utils.test.ts b/packages/opentelemetry-sdk-trace-web/test/utils.test.ts
@@ -127,6 +127,35 @@ describe('utils', () => {
       } as PerformanceResourceTiming);
       assert.strictEqual(addEventSpy.callCount, 9);
     });
+
+    it('should ignore network events when ignoreNetworkEvents is true', () => {
+      const addEventSpy = sinon.spy();
+      const setAttributeSpy = sinon.spy();
+      const span = {
+        addEvent: addEventSpy,
+        setAttribute: setAttributeSpy,
+      } as unknown as tracing.Span;
+      const entries = {
+        [PTN.FETCH_START]: 123,
+        [PTN.DOMAIN_LOOKUP_START]: 123,
+        [PTN.DOMAIN_LOOKUP_END]: 123,
+        [PTN.CONNECT_START]: 123,
+        [PTN.SECURE_CONNECTION_START]: 123,
+        [PTN.CONNECT_END]: 123,
+        [PTN.REQUEST_START]: 123,
+        [PTN.RESPONSE_START]: 123,
+        [PTN.RESPONSE_END]: 123,
+        [PTN.DECODED_BODY_SIZE]: 123,
+        [PTN.ENCODED_BODY_SIZE]: 61,
+      } as PerformanceEntries;
+
+      assert.strictEqual(addEventSpy.callCount, 0);
+
+      addSpanNetworkEvents(span, entries, true);
+      assert.strictEqual(setAttributeSpy.callCount, 2);
+      //secure connect start should not be added to non-https resource
+      assert.strictEqual(addEventSpy.callCount, 0);
+    });
     it('should only include encoded size when content encoding is being used', () => {
       const addEventSpy = sinon.spy();
       const setAttributeSpy = sinon.spy();