fix(fetch): end span after body is received (#2203)

open-telemetry · May 19, 2021 · cc44db1 · cc44db1
1 parent e379e59
commit cc44db1
Show file tree

Hide file tree

Showing 2 changed files with 73 additions and 47 deletions.
diff --git a/packages/opentelemetry-instrumentation-fetch/src/fetch.ts b/packages/opentelemetry-instrumentation-fetch/src/fetch.ts
@@ -314,6 +314,27 @@ export class FetchInstrumentation extends InstrumentationBase<
         }
         const spanData = plugin._prepareSpanData(url);
 
+        function endSpanOnError(span: api.Span, error: FetchError) {
+          plugin._applyAttributesAfterFetch(span, options, error);
+          plugin._endSpan(span, spanData, {
+            status: error.status || 0,
+            statusText: error.message,
+            url,
+          });
+        }
+
+        function endSpanOnSuccess(span: api.Span, response: Response) {
+          plugin._applyAttributesAfterFetch(span, options, response);
+          if (response.status >= 200 && response.status < 400) {
+            plugin._endSpan(span, spanData, response);
+          } else {
+            plugin._endSpan(span, spanData, {
+              status: response.status,
+              statusText: response.statusText,
+              url,
+            });
+          }
+        }
         function onSuccess(
           span: api.Span,
           resolve: (
@@ -322,15 +343,28 @@ export class FetchInstrumentation extends InstrumentationBase<
           response: Response
         ) {
           try {
-            plugin._applyAttributesAfterFetch(span, options, response);
-            if (response.status >= 200 && response.status < 400) {
-              plugin._endSpan(span, spanData, response);
+            const resClone = response.clone();
+            const body = resClone.body;
+            if (body) {
+              const reader = body.getReader();
+              const read = (): void => {
+                reader.read().then(
+                  ({ done }) => {
+                    if (done) {
+                      endSpanOnSuccess(span, response);
+                    } else {
+                      read();
+                    }
+                  },
+                  error => {
+                    endSpanOnError(span, error);
+                  }
+                );
+              };
+              read();
             } else {
-              plugin._endSpan(span, spanData, {
-                status: response.status,
-                statusText: response.statusText,
-                url,
-              });
+              // some older browsers don't have .body implemented
+              endSpanOnSuccess(span, response);
             }
           } finally {
             resolve(response);
@@ -343,12 +377,7 @@ export class FetchInstrumentation extends InstrumentationBase<
           error: FetchError
         ) {
           try {
-            plugin._applyAttributesAfterFetch(span, options, error);
-            plugin._endSpan(span, spanData, {
-              status: error.status || 0,
-              statusText: error.message,
-              url,
-            });
+            endSpanOnError(span, error);
           } finally {
             reject(error);
           }

diff --git a/packages/opentelemetry-instrumentation-fetch/test/fetch.test.ts b/packages/opentelemetry-instrumentation-fetch/test/fetch.test.ts
@@ -51,15 +51,16 @@ class DummySpanExporter implements tracing.SpanExporter {
   }
 }
 
-const getData = (url: string, method?: string) =>
-  fetch(url, {
+const getData = (url: string, method?: string) => {
+  return fetch(url, {
     method: method || 'GET',
     headers: {
       foo: 'bar',
       Accept: 'application/json',
       'Content-Type': 'application/json',
     },
   });
+};
 
 const CUSTOM_ATTRIBUTE_KEY = 'span kind';
 const defaultResource = {
@@ -235,37 +236,33 @@ describe('fetch', () => {
     rootSpan = webTracerWithZone.startSpan('root');
     api.context.with(api.setSpan(api.context.active(), rootSpan), () => {
       fakeNow = 0;
-      getData(fileUrl, method).then(
-        response => {
-          // this is a bit tricky as the only way to get all request headers from
-          // fetch is to use json()
-          response.json().then(
-            json => {
-              lastResponse = json;
-              const headers: { [key: string]: string } = {};
-              Object.keys(lastResponse.headers).forEach(key => {
-                headers[key.toLowerCase()] = lastResponse.headers[key];
-              });
-              lastResponse.headers = headers;
-              // OBSERVER_WAIT_TIME_MS
-              sinon.clock.tick(300);
-              done();
-            },
-            () => {
-              lastResponse = undefined;
-              // OBSERVER_WAIT_TIME_MS
-              sinon.clock.tick(300);
-              done();
-            }
-          );
-        },
-        () => {
-          lastResponse = undefined;
-          // OBSERVER_WAIT_TIME_MS
-          sinon.clock.tick(300);
+      void getData(fileUrl, method)
+        .then(
+          response => {
+            // this is a bit tricky as the only way to get all request headers from
+            // fetch is to use json()
+            return response.json().then(
+              json => {
+                lastResponse = json;
+                const headers: { [key: string]: string } = {};
+                Object.keys(lastResponse.headers).forEach(key => {
+                  headers[key.toLowerCase()] = lastResponse.headers[key];
+                });
+                lastResponse.headers = headers;
+              },
+              () => {
+                lastResponse = undefined;
+              }
+            );
+          },
+          () => {
+            lastResponse = undefined;
+          }
+        )
+        .then(sinon.clock.runAllAsync)
+        .then(() => {
           done();
-        }
-      );
+        });
       fakeNow = 300;
     });
   };
@@ -529,7 +526,7 @@ describe('fetch', () => {
 
     it('should set trace headers with a request object', () => {
       const r = new Request('url');
-      window.fetch(r);
+      window.fetch(r).catch(() => {});
       assert.ok(typeof r.headers.get(X_B3_TRACE_ID) === 'string');
     });