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

feat(tracing): auto flush BatchSpanProcessor on browser #2243

Merged
merged 13 commits into from
Jun 16, 2021
Merged
Show file tree
Hide file tree
Changes from all 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
4 changes: 3 additions & 1 deletion packages/opentelemetry-tracing/karma.conf.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ const karmaBaseConfig = require('../../karma.base');

module.exports = (config) => {
config.set(Object.assign({}, karmaBaseConfig, {
webpack: karmaWebpackConfig
webpack: karmaWebpackConfig,
files: ['test/browser/index-webpack.ts'],
preprocessors: { 'test/browser/index-webpack.ts': ['webpack'] }
}))
};
2 changes: 1 addition & 1 deletion packages/opentelemetry-tracing/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
"scripts": {
"compile": "tsc --build tsconfig.json tsconfig.esm.json",
"clean": "tsc --build --clean tsconfig.json tsconfig.esm.json",
"test": "nyc ts-mocha -p tsconfig.json 'test/**/*.test.ts' --exclude 'test/index-webpack.ts'",
"test": "nyc ts-mocha -p tsconfig.json 'test/**/*.test.ts' --exclude 'test/browser/**/*.ts'",
"test:browser": "nyc karma start --single-run",
"tdd": "npm run tdd:node",
"tdd:node": "npm run test -- --watch-extensions ts --watch",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ import { SDKRegistrationConfig, TracerConfig } from './types';
// eslint-disable-next-line @typescript-eslint/no-var-requires
const merge = require('lodash.merge');
import { SpanExporter } from './export/SpanExporter';
import { BatchSpanProcessor } from './export/BatchSpanProcessor';
import { BatchSpanProcessor } from './platform';

export type PROPAGATOR_FACTORY = () => TextMapPropagator;
export type EXPORTER_FACTORY = () => SpanExporter;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ import { SpanExporter } from './SpanExporter';
* Implementation of the {@link SpanProcessor} that batches spans exported by
* the SDK then pushes them to the exporter pipeline.
*/
export class BatchSpanProcessor implements SpanProcessor {
export abstract class BatchSpanProcessorBase<T extends BufferConfig> implements SpanProcessor {
private readonly _maxExportBatchSize: number;
private readonly _maxQueueSize: number;
private readonly _scheduledDelayMillis: number;
Expand All @@ -43,23 +43,23 @@ export class BatchSpanProcessor implements SpanProcessor {
private _isShutdown = false;
private _shuttingDownPromise: Promise<void> = Promise.resolve();

constructor(private readonly _exporter: SpanExporter, config?: BufferConfig) {
constructor(private readonly _exporter: SpanExporter, config?: T) {
const env = getEnv();
this._maxExportBatchSize =
typeof config?.maxExportBatchSize === 'number'
? config.maxExportBatchSize
: env.OTEL_BSP_MAX_EXPORT_BATCH_SIZE;
this._maxQueueSize =
typeof config?.maxQueueSize === 'number'
? config?.maxQueueSize
? config.maxQueueSize
: env.OTEL_BSP_MAX_QUEUE_SIZE;
this._scheduledDelayMillis =
typeof config?.scheduledDelayMillis === 'number'
? config?.scheduledDelayMillis
? config.scheduledDelayMillis
: env.OTEL_BSP_SCHEDULE_DELAY;
this._exportTimeoutMillis =
typeof config?.exportTimeoutMillis === 'number'
? config?.exportTimeoutMillis
? config.exportTimeoutMillis
: env.OTEL_BSP_EXPORT_TIMEOUT;
}

Expand Down Expand Up @@ -87,6 +87,9 @@ export class BatchSpanProcessor implements SpanProcessor {
this._isShutdown = true;
this._shuttingDownPromise = new Promise((resolve, reject) => {
Promise.resolve()
.then(() => {
return this.onShutdown();
})
.then(() => {
return this._flushAll();
})
Expand Down Expand Up @@ -190,4 +193,6 @@ export class BatchSpanProcessor implements SpanProcessor {
this._timer = undefined;
}
}

protected abstract onShutdown(): void;
}
2 changes: 1 addition & 1 deletion packages/opentelemetry-tracing/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@

export * from './Tracer';
export * from './BasicTracerProvider';
export * from './platform';
export * from './export/ConsoleSpanExporter';
export * from './export/BatchSpanProcessor';
export * from './export/InMemorySpanExporter';
export * from './export/ReadableSpan';
export * from './export/SimpleSpanProcessor';
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
/*
* Copyright The OpenTelemetry Authors
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

import { BatchSpanProcessorBase } from '../../../export/BatchSpanProcessorBase';
import { SpanExporter } from '../../../export/SpanExporter';
import { BatchSpanProcessorBrowserConfig } from '../../../types';

export class BatchSpanProcessor extends BatchSpanProcessorBase<BatchSpanProcessorBrowserConfig> {
private _visibilityChangeListener?: () => void
private _pageHideListener?: () => void

constructor(_exporter: SpanExporter, config?: BatchSpanProcessorBrowserConfig) {
super(_exporter, config)
this.onInit(config)
}

private onInit(config?: BatchSpanProcessorBrowserConfig): void {
if (config?.disableAutoFlushOnDocumentHide !== true && document != null) {
this._visibilityChangeListener = () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should also trigger flush on a pagehide event - Safari doesn't fully support visibilitychange: https://caniuse.com/?search=visibilitychange

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. I added support for the pagehide event.
I didn't add any check to prevent double call of forceFlush when both pagehide and visibilitychange events are dispatched to make the code simpler. Let me know if you want such mechanism.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think there's any need for that level of complexity - flush implementations should be smart enough.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

smart enough to be called twice concurrently you mean?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is also beforeunload and unload (depending on the browser support matrix that we want to support -- specifically older IE instances (which are probably already excluded by other usages)) that you may want to also consider adding.

Additionally, there is actually a set of corner cases that we should support around pages hooking theses same events after the SDK has initialized but still creating / sending telemetry. The simplest solution for this is to set a flag during these flush events (or more specifically the unload events) and then during the onEnd() as well as batching you also trigger the same auto flush.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I described my concerns about using unload event here #2205 .

Also I was testing JS auto-instrumentation on different browsers and on IE11 I got a Script error so I guess we don't support it.

This PR is about auto flushing created spans when page become hidden, so I don't think there is something wrong with sending telemetry when page is not active, or I didn't get your point right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-1 for unload: https://developer.mozilla.org/en-US/docs/Web/API/Navigator/sendBeacon has a full writeup of the recommended approach. What's in the PR now looks good.

If the current BatchSpanExporter.forceFlush isn't smart enough to deal with two calls in succession, I think it should be improved as a separate PR rather than complicating state/logic here. A surface skim looks like it might have some issues...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My comment is not a blocking comment for this PR -- as @johnbley says What's in the PR now looks good as it's making this better

if (document.visibilityState === 'hidden') {
void this.forceFlush();
}
}
this._pageHideListener = () => {
void this.forceFlush()
}
document.addEventListener('visibilitychange', this._visibilityChangeListener);

// use 'pagehide' event as a fallback for Safari; see https://bugs.webkit.org/show_bug.cgi?id=116769
document.addEventListener('pagehide', this._pageHideListener);
}
}

protected onShutdown(): void {
if (this._visibilityChangeListener) {
document.removeEventListener('visibilitychange', this._visibilityChangeListener);
}
if (this._pageHideListener) {
document.removeEventListener('pagehide', this._pageHideListener);
}
}
}
17 changes: 17 additions & 0 deletions packages/opentelemetry-tracing/src/platform/browser/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
/*
* Copyright The OpenTelemetry Authors
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

export * from './export/BatchSpanProcessor';
17 changes: 17 additions & 0 deletions packages/opentelemetry-tracing/src/platform/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
/*
* Copyright The OpenTelemetry Authors
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

export * from './node';
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
/*
* Copyright The OpenTelemetry Authors
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

import { BatchSpanProcessorBase } from '../../../export/BatchSpanProcessorBase';
import { BufferConfig } from '../../../types';

export class BatchSpanProcessor extends BatchSpanProcessorBase<BufferConfig> {
protected onShutdown(): void {}
}
17 changes: 17 additions & 0 deletions packages/opentelemetry-tracing/src/platform/node/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
/*
* Copyright The OpenTelemetry Authors
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

export * from './export/BatchSpanProcessor';
7 changes: 7 additions & 0 deletions packages/opentelemetry-tracing/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -89,3 +89,10 @@ export interface BufferConfig {
* The default value is 2048. */
maxQueueSize?: number;
}

/** Interface configuration for BatchSpanProcessor on browser */
export interface BatchSpanProcessorBrowserConfig extends BufferConfig {
/** Disable flush when a user navigates to a new page, closes the tab or the browser, or,
* on mobile, switches to a different app. Auto flush is enabled by default. */
disableAutoFlushOnDocumentHide?: boolean;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
/*
* Copyright The OpenTelemetry Authors
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

import * as assert from 'assert';
import * as sinon from 'sinon';
import { SpanExporter } from '../../../src';
import { BatchSpanProcessor } from '../../../src/platform/browser/export/BatchSpanProcessor';
import { TestTracingSpanExporter } from '../../common/export/TestTracingSpanExporter';

describe('BatchSpanProcessor - web', () => {
let visibilityState: VisibilityState = 'visible';
let exporter: SpanExporter
let processor: BatchSpanProcessor;
let forceFlushSpy: sinon.SinonStub;
let visibilityChangeEvent: Event;
let pageHideEvent: Event

beforeEach(() => {
sinon.replaceGetter(document, 'visibilityState', () => visibilityState);
visibilityState = 'visible';
exporter = new TestTracingSpanExporter();
processor = new BatchSpanProcessor(exporter, {});
forceFlushSpy = sinon.stub(processor, 'forceFlush');
visibilityChangeEvent = new Event('visibilitychange');
pageHideEvent = new Event('pagehide');
});

afterEach(async () => {
sinon.restore();
});

describe('when document becomes hidden', () => {
const testDocumentHide = (hideDocument: () => void) => {
it('should force flush spans', () => {
assert.strictEqual(forceFlushSpy.callCount, 0);
hideDocument()
assert.strictEqual(forceFlushSpy.callCount, 1);
});

describe('AND shutdown has been called', () => {
it('should NOT force flush spans', async () => {
assert.strictEqual(forceFlushSpy.callCount, 0);
await processor.shutdown();
hideDocument()
assert.strictEqual(forceFlushSpy.callCount, 0);
});
})

describe('AND disableAutoFlushOnDocumentHide configuration option', () => {
it('set to false should force flush spans', () => {
processor = new BatchSpanProcessor(exporter, { disableAutoFlushOnDocumentHide: false });
forceFlushSpy = sinon.stub(processor, 'forceFlush');
assert.strictEqual(forceFlushSpy.callCount, 0);
hideDocument()
assert.strictEqual(forceFlushSpy.callCount, 1);
})

it('set to true should NOT force flush spans', () => {
processor = new BatchSpanProcessor(exporter, { disableAutoFlushOnDocumentHide: true });
forceFlushSpy = sinon.stub(processor, 'forceFlush');
assert.strictEqual(forceFlushSpy.callCount, 0);
hideDocument()
assert.strictEqual(forceFlushSpy.callCount, 0);
})
})
}

describe('by the visibilitychange event', () => {
testDocumentHide(() => {
visibilityState = 'hidden';
document.dispatchEvent(visibilityChangeEvent);
})
})

describe('by the pagehide event', () => {
testDocumentHide(() => {
document.dispatchEvent(pageHideEvent);
})
})
})

describe('when document becomes visible', () => {
it('should NOT force flush spans', () => {
assert.strictEqual(forceFlushSpy.callCount, 0);
document.dispatchEvent(visibilityChangeEvent);
assert.strictEqual(forceFlushSpy.callCount, 0);
});
})
});
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,11 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
const testsContext = require.context('.', true, /test$/);
const testsContext = require.context('../browser', true, /test$/);
testsContext.keys().forEach(testsContext);

const testsContextCommon = require.context('../common', true, /test$/);
testsContextCommon.keys().forEach(testsContextCommon);

const srcContext = require.context('.', true, /src$/);
srcContext.keys().forEach(srcContext);
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ import {
InMemorySpanExporter,
SpanExporter,
BatchSpanProcessor,
} from '../src';
} from '../../src';

describe('BasicTracerProvider', () => {
let removeEvent: Function | undefined;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,12 @@ import {
SimpleSpanProcessor,
Span,
SpanProcessor,
} from '../src';
} from '../../src';
import {
setGlobalErrorHandler,
loggingErrorHandler,
} from '@opentelemetry/core';
import { MultiSpanProcessor } from '../src/MultiSpanProcessor';
import { MultiSpanProcessor } from '../../src/MultiSpanProcessor';

class TestProcessor implements SpanProcessor {
spans: Span[] = [];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ import {
} from '@opentelemetry/core';
import { SemanticAttributes } from '@opentelemetry/semantic-conventions';
import * as assert from 'assert';
import { BasicTracerProvider, Span, SpanProcessor } from '../src';
import { BasicTracerProvider, Span, SpanProcessor } from '../../src';

const performanceTimeOrigin = hrTime();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ import {
suppressTracing
} from '@opentelemetry/core';
import * as assert from 'assert';
import { BasicTracerProvider, Span, Tracer } from '../src';
import { BasicTracerProvider, Span, Tracer } from '../../src';
import { TestStackContextManager } from './export/TestStackContextManager';
import * as sinon from 'sinon';

Expand Down
Loading