fix(sdk-trace-base): pass BatchSpanProcessor#forceFlush() errors on v…

…isibilitychange/pagehide to globalErrorHandler (#5143) Co-authored-by: Hector Hernandez <[email protected]>
open-telemetry · Nov 14, 2024 · 4afc190 · 4afc190
1 parent 699b919
commit 4afc190
Show file tree

Hide file tree

Showing 3 changed files with 42 additions and 5 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -22,6 +22,8 @@ For semantic convention package changes, see the [semconv CHANGELOG](packages/se
 ### :bug: (Bug Fix)
 
 * fix(sdk-metrics): await exports in `PeriodicExportingMetricReader` when async resource attributes have not yet settled [#5119](https://github.com/open-telemetry/opentelemetry-js/pull/5119/) @pichlermarc
+* fix(sdk-trace-base): pass BatchSpanProcessor#forceFlush() errors on visibilitychange/pagehide to globalErrorHandler [#5143](https://github.com/open-telemetry/opentelemetry-js/pull/5143) @pichlermarc
+  * fixes a bug where switching browser tabs with a failing exporter would cause an unhandled error
 
 ### :books: (Refine Doc)
 

diff --git a/packages/opentelemetry-sdk-trace-base/src/platform/browser/export/BatchSpanProcessor.ts b/packages/opentelemetry-sdk-trace-base/src/platform/browser/export/BatchSpanProcessor.ts
@@ -17,6 +17,7 @@
 import { BatchSpanProcessorBase } from '../../../export/BatchSpanProcessorBase';
 import { SpanExporter } from '../../../export/SpanExporter';
 import { BatchSpanProcessorBrowserConfig } from '../../../types';
+import { globalErrorHandler } from '@opentelemetry/core';
 
 export class BatchSpanProcessor extends BatchSpanProcessorBase<BatchSpanProcessorBrowserConfig> {
   private _visibilityChangeListener?: () => void;
@@ -37,11 +38,15 @@ export class BatchSpanProcessor extends BatchSpanProcessorBase<BatchSpanProcesso
     ) {
       this._visibilityChangeListener = () => {
         if (document.visibilityState === 'hidden') {
-          void this.forceFlush();
+          this.forceFlush().catch(error => {
+            globalErrorHandler(error);
+          });
         }
       };
       this._pageHideListener = () => {
-        void this.forceFlush();
+        this.forceFlush().catch(error => {
+          globalErrorHandler(error);
+        });
       };
       document.addEventListener(
         'visibilitychange',

diff --git a/packages/opentelemetry-sdk-trace-base/test/browser/export/BatchSpanProcessor.test.ts b/packages/opentelemetry-sdk-trace-base/test/browser/export/BatchSpanProcessor.test.ts
@@ -19,6 +19,10 @@ import * as sinon from 'sinon';
 import { SpanExporter } from '../../../src';
 import { BatchSpanProcessor } from '../../../src/platform/browser/export/BatchSpanProcessor';
 import { TestTracingSpanExporter } from '../../common/export/TestTracingSpanExporter';
+import {
+  loggingErrorHandler,
+  setGlobalErrorHandler,
+} from '@opentelemetry/core';
 
 /**
  * VisibilityState has been removed from TypeScript 4.6.0+
@@ -37,19 +41,25 @@ describeDocument('BatchSpanProcessor - web main context', () => {
   let forceFlushSpy: sinon.SinonStub;
   let visibilityChangeEvent: Event;
   let pageHideEvent: Event;
+  let globalErrorHandlerStub: sinon.SinonStub;
 
   beforeEach(() => {
     sinon.replaceGetter(document, 'visibilityState', () => visibilityState);
     visibilityState = 'visible';
     exporter = new TestTracingSpanExporter();
     processor = new BatchSpanProcessor(exporter, {});
-    forceFlushSpy = sinon.stub(processor, 'forceFlush');
+    forceFlushSpy = sinon
+      .stub(processor, 'forceFlush')
+      .returns(Promise.resolve());
     visibilityChangeEvent = new Event('visibilitychange');
     pageHideEvent = new Event('pagehide');
+    globalErrorHandlerStub = sinon.stub();
+    setGlobalErrorHandler(globalErrorHandlerStub);
   });
 
   afterEach(async () => {
     sinon.restore();
+    setGlobalErrorHandler(loggingErrorHandler());
   });
 
   describe('when document becomes hidden', () => {
@@ -60,6 +70,26 @@ describeDocument('BatchSpanProcessor - web main context', () => {
         assert.strictEqual(forceFlushSpy.callCount, 1);
       });
 
+      it('should catch any error thrown by forceFlush', done => {
+        const forceFlushError = new Error('forceFlush failed');
+        forceFlushSpy.rejects(forceFlushError);
+        hideDocument();
+        sinon.assert.calledOnce(forceFlushSpy);
+
+        // queue a microtask since hideDocument() returns before forceFlush() rejects
+        queueMicrotask(() => {
+          try {
+            sinon.assert.calledOnceWithExactly(
+              globalErrorHandlerStub,
+              forceFlushError
+            );
+            done();
+          } catch (e) {
+            done(e);
+          }
+        });
+      });
+
       describe('AND shutdown has been called', () => {
         it('should NOT force flush spans', async () => {
           assert.strictEqual(forceFlushSpy.callCount, 0);
@@ -74,7 +104,7 @@ describeDocument('BatchSpanProcessor - web main context', () => {
           processor = new BatchSpanProcessor(exporter, {
             disableAutoFlushOnDocumentHide: false,
           });
-          forceFlushSpy = sinon.stub(processor, 'forceFlush');
+          forceFlushSpy = sinon.stub(processor, 'forceFlush').resolves();
           assert.strictEqual(forceFlushSpy.callCount, 0);
           hideDocument();
           assert.strictEqual(forceFlushSpy.callCount, 1);
@@ -84,7 +114,7 @@ describeDocument('BatchSpanProcessor - web main context', () => {
           processor = new BatchSpanProcessor(exporter, {
             disableAutoFlushOnDocumentHide: true,
           });
-          forceFlushSpy = sinon.stub(processor, 'forceFlush');
+          forceFlushSpy = sinon.stub(processor, 'forceFlush').resolves();
           assert.strictEqual(forceFlushSpy.callCount, 0);
           hideDocument();
           assert.strictEqual(forceFlushSpy.callCount, 0);