Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(sdk-trace-base): pass BatchSpanProcessor#forceFlush() errors on visibilitychange/pagehide to globalErrorHandler #5143

Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 tabs with a failing exporter would cause an unhandled error
pichlermarc marked this conversation as resolved.
Show resolved Hide resolved

### :books: (Refine Doc)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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+
Expand All @@ -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', () => {
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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);
Expand Down
Loading