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 4 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 3 additions & 1 deletion packages/opentelemetry-tracing/karma.conf.js
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
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
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
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,24 +43,26 @@ 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;

this.onInit(config);
}

forceFlush(): Promise<void> {
Expand All @@ -87,6 +89,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 +195,7 @@ export class BatchSpanProcessor implements SpanProcessor {
this._timer = undefined;
}
}

abstract onInit(config?: T): void;
Copy link
Member

Choose a reason for hiding this comment

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

Why make these abstract if they're only used by browser? Wouldn't it make more sense to just override the constructor in the browser impl then call onInit after super?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

abstract onShutdown(): void;
}
2 changes: 1 addition & 1 deletion packages/opentelemetry-tracing/src/index.ts
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
@@ -0,0 +1,39 @@
/*
* 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 { BatchSpanProcessorBrowserConfig } from '../../../types';

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

onInit(config?: BatchSpanProcessorBrowserConfig): void {
if (config?.flushOnDocumentBecomesHidden !== false && 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();
}
}
document.addEventListener('visibilitychange', this._visibilityChangeListener);
}
}

onShutdown(): void {
if (this._visibilityChangeListener) {
document.removeEventListener('visibilitychange', this._visibilityChangeListener);
}
}
}
17 changes: 17 additions & 0 deletions packages/opentelemetry-tracing/src/platform/browser/index.ts
@@ -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
@@ -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';
@@ -0,0 +1,24 @@
/*
* 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> {
onInit(): void {}

onShutdown(): void {}
}
17 changes: 17 additions & 0 deletions packages/opentelemetry-tracing/src/platform/node/index.ts
@@ -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
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 {
/** Force flush when a user navigates to a new page, closes the tab or the browser, or,
* on mobile, switches to a different app. The default value is `true`. */
flushOnDocumentBecomesHidden?: boolean;
}
kkruk-sumo marked this conversation as resolved.
Show resolved Hide resolved
@@ -0,0 +1,89 @@
/*
* 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 { BatchSpanProcessor, SpanExporter } from '../../../src';
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;

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

afterEach(() => {
processor.onShutdown();
sinon.restore();
});

describe('when document becomes hidden', () => {
it('should force flush spans', () => {
assert.strictEqual(forceFlushSpy.callCount, 0);
visibilityState = 'hidden';
document.dispatchEvent(visibilityChangeEvent);
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();
visibilityState = 'hidden';
document.dispatchEvent(visibilityChangeEvent);
assert.strictEqual(forceFlushSpy.callCount, 0);
});
})

describe('AND flushOnDocumentBecomesHidden configuration option', () => {
it('set to true should force flush spans', () => {
processor = new BatchSpanProcessor(exporter, { flushOnDocumentBecomesHidden: true });
forceFlushSpy = sinon.stub(processor, 'forceFlush');
assert.strictEqual(forceFlushSpy.callCount, 0);
visibilityState = 'hidden';
document.dispatchEvent(visibilityChangeEvent);
assert.strictEqual(forceFlushSpy.callCount, 1);
})

it('set to false should NOT force flush spans', () => {
processor = new BatchSpanProcessor(exporter, { flushOnDocumentBecomesHidden: false });
forceFlushSpy = sinon.stub(processor, 'forceFlush');
assert.strictEqual(forceFlushSpy.callCount, 0);
visibilityState = 'hidden';
document.dispatchEvent(visibilityChangeEvent);
assert.strictEqual(forceFlushSpy.callCount, 0);
})
})
})

describe('when document becomes visible', () => {
it('should NOT force flush spans', () => {
assert.strictEqual(forceFlushSpy.callCount, 0);
document.dispatchEvent(visibilityChangeEvent);
assert.strictEqual(forceFlushSpy.callCount, 0);
});
})
});
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);
Expand Up @@ -43,7 +43,7 @@ import {
InMemorySpanExporter,
SpanExporter,
BatchSpanProcessor,
} from '../src';
} from '../../src';

describe('BasicTracerProvider', () => {
let removeEvent: Function | undefined;
Expand Down
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
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
Expand Up @@ -30,7 +30,7 @@ import {
suppressTracing,
} from '@opentelemetry/core';
import * as assert from 'assert';
import { BasicTracerProvider, Span, Tracer } from '../src';
import { BasicTracerProvider, Span, Tracer } from '../../src';

describe('Tracer', () => {
const tracerProvider = new BasicTracerProvider();
Expand Down
Expand Up @@ -20,7 +20,7 @@ import {
TraceIdRatioBasedSampler,
} from '@opentelemetry/core';
import * as assert from 'assert';
import { buildSamplerFromEnv } from '../src/config';
import { buildSamplerFromEnv } from '../../src/config';

describe('config', () => {
const envSource = (typeof window !== 'undefined'
Expand Down