Skip to content

Commit 64308ce

Browse files
AbhiPrasadAbhiPrasad
authored
ref(browser/core): Move all log flushing logic into clients (#15831)
I experimented with a bunch of different approaches to register log flushing implementations, but I found the cleanest and most performant solution was to have log flushing logic live in clients. This refactors the browser client to do that, and adds tests for all of our flushing logic everywhere. The bundle size increase is not ideal, but I recognize we have no choice. After this gets merged in, I'll add the console logging integration to core. I'll also add some quick integrations for pino and winston.
1 parent bcb15ba commit 64308ce

File tree

9 files changed

+314
-201
lines changed

9 files changed

+314
-201
lines changed

‎packages/browser/src/client.ts

+31-3
Original file line numberDiff line numberDiff line change
@@ -15,11 +15,14 @@ import {
1515
addAutoIpAddressToUser,
1616
applySdkMetadata,
1717
getSDKSource,
18+
_INTERNAL_flushLogsBuffer,
1819
} from '@sentry/core';
1920
import { eventFromException, eventFromMessage } from './eventbuilder';
2021
import { WINDOW } from './helpers';
2122
import type { BrowserTransportOptions } from './transports/types';
2223

24+
const DEFAULT_FLUSH_INTERVAL = 5000;
25+
2326
/**
2427
* Configuration options for the Sentry Browser SDK.
2528
* @see @sentry/core Options for more information.
@@ -65,6 +68,7 @@ export type BrowserClientOptions = ClientOptions<BrowserTransportOptions> &
6568
* @see SentryClient for usage documentation.
6669
*/
6770
export class BrowserClient extends Client<BrowserClientOptions> {
71+
private _logFlushIdleTimeout: ReturnType<typeof setTimeout> | undefined;
6872
/**
6973
* Creates a new Browser SDK instance.
7074
*
@@ -81,17 +85,41 @@ export class BrowserClient extends Client<BrowserClientOptions> {
8185

8286
super(opts);
8387

88+
// eslint-disable-next-line @typescript-eslint/no-this-alias
89+
const client = this;
90+
const { sendDefaultPii, _experiments } = client._options;
91+
const enableLogs = _experiments?.enableLogs;
92+
8493
if (opts.sendClientReports && WINDOW.document) {
8594
WINDOW.document.addEventListener('visibilitychange', () => {
8695
if (WINDOW.document.visibilityState === 'hidden') {
8796
this._flushOutcomes();
97+
if (enableLogs) {
98+
_INTERNAL_flushLogsBuffer(client);
99+
}
88100
}
89101
});
90102
}
91103

92-
if (this._options.sendDefaultPii) {
93-
this.on('postprocessEvent', addAutoIpAddressToUser);
94-
this.on('beforeSendSession', addAutoIpAddressToSession);
104+
if (enableLogs) {
105+
client.on('flush', () => {
106+
_INTERNAL_flushLogsBuffer(client);
107+
});
108+
109+
client.on('afterCaptureLog', () => {
110+
if (client._logFlushIdleTimeout) {
111+
clearTimeout(client._logFlushIdleTimeout);
112+
}
113+
114+
client._logFlushIdleTimeout = setTimeout(() => {
115+
_INTERNAL_flushLogsBuffer(client);
116+
}, DEFAULT_FLUSH_INTERVAL);
117+
});
118+
}
119+
120+
if (sendDefaultPii) {
121+
client.on('postprocessEvent', addAutoIpAddressToUser);
122+
client.on('beforeSendSession', addAutoIpAddressToSession);
95123
}
96124
}
97125

‎packages/browser/src/log.ts

+3-58
Original file line numberDiff line numberDiff line change
@@ -1,53 +1,5 @@
1-
import type { LogSeverityLevel, Log, Client, ParameterizedString } from '@sentry/core';
2-
import { getClient, _INTERNAL_captureLog, _INTERNAL_flushLogsBuffer } from '@sentry/core';
3-
4-
import { WINDOW } from './helpers';
5-
6-
/**
7-
* TODO: Make this configurable
8-
*/
9-
const DEFAULT_FLUSH_INTERVAL = 5000;
10-
11-
let timeout: ReturnType<typeof setTimeout> | undefined;
12-
13-
/**
14-
* This is a global timeout that is used to flush the logs buffer.
15-
* It is used to ensure that logs are flushed even if the client is not flushed.
16-
*/
17-
function startFlushTimeout(client: Client): void {
18-
if (timeout) {
19-
clearTimeout(timeout);
20-
}
21-
22-
timeout = setTimeout(() => {
23-
_INTERNAL_flushLogsBuffer(client);
24-
}, DEFAULT_FLUSH_INTERVAL);
25-
}
26-
27-
let isClientListenerAdded = false;
28-
/**
29-
* This is a function that is used to add a flush listener to the client.
30-
* It is used to ensure that the logger buffer is flushed when the client is flushed.
31-
*/
32-
function addFlushingListeners(client: Client): void {
33-
if (isClientListenerAdded || !client.getOptions()._experiments?.enableLogs) {
34-
return;
35-
}
36-
37-
isClientListenerAdded = true;
38-
39-
if (WINDOW.document) {
40-
WINDOW.document.addEventListener('visibilitychange', () => {
41-
if (WINDOW.document.visibilityState === 'hidden') {
42-
_INTERNAL_flushLogsBuffer(client);
43-
}
44-
});
45-
}
46-
47-
client.on('flush', () => {
48-
_INTERNAL_flushLogsBuffer(client);
49-
});
50-
}
1+
import type { LogSeverityLevel, Log, ParameterizedString } from '@sentry/core';
2+
import { _INTERNAL_captureLog } from '@sentry/core';
513

524
/**
535
* Capture a log with the given level.
@@ -63,14 +15,7 @@ function captureLog(
6315
attributes?: Log['attributes'],
6416
severityNumber?: Log['severityNumber'],
6517
): void {
66-
const client = getClient();
67-
if (client) {
68-
addFlushingListeners(client);
69-
70-
startFlushTimeout(client);
71-
}
72-
73-
_INTERNAL_captureLog({ level, message, attributes, severityNumber }, client, undefined);
18+
_INTERNAL_captureLog({ level, message, attributes, severityNumber });
7419
}
7520

7621
/**

‎packages/browser/test/client.test.ts

+120
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,120 @@
1+
/**
2+
* @vitest-environment jsdom
3+
*/
4+
5+
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
6+
import * as sentryCore from '@sentry/core';
7+
import { BrowserClient } from '../src/client';
8+
import { WINDOW } from '../src/helpers';
9+
import { getDefaultBrowserClientOptions } from './helper/browser-client-options';
10+
11+
vi.mock('@sentry/core', async requireActual => {
12+
return {
13+
...((await requireActual()) as any),
14+
_INTERNAL_flushLogsBuffer: vi.fn(),
15+
};
16+
});
17+
18+
describe('BrowserClient', () => {
19+
let client: BrowserClient;
20+
const DEFAULT_FLUSH_INTERVAL = 5000;
21+
22+
afterEach(() => {
23+
vi.useRealTimers();
24+
vi.clearAllMocks();
25+
});
26+
27+
it('does not flush logs when logs are disabled', () => {
28+
client = new BrowserClient(
29+
getDefaultBrowserClientOptions({
30+
_experiments: { enableLogs: false },
31+
sendClientReports: true,
32+
}),
33+
);
34+
35+
// Add some logs
36+
sentryCore._INTERNAL_captureLog({ level: 'info', message: 'test log 1' }, client);
37+
sentryCore._INTERNAL_captureLog({ level: 'info', message: 'test log 2' }, client);
38+
39+
// Simulate visibility change to hidden
40+
if (WINDOW.document) {
41+
Object.defineProperty(WINDOW.document, 'visibilityState', { value: 'hidden' });
42+
WINDOW.document.dispatchEvent(new Event('visibilitychange'));
43+
}
44+
45+
expect(sentryCore._INTERNAL_flushLogsBuffer).not.toHaveBeenCalled();
46+
});
47+
48+
describe('log flushing', () => {
49+
beforeEach(() => {
50+
vi.useFakeTimers();
51+
client = new BrowserClient(
52+
getDefaultBrowserClientOptions({
53+
_experiments: { enableLogs: true },
54+
sendClientReports: true,
55+
}),
56+
);
57+
});
58+
59+
it('flushes logs when page visibility changes to hidden', () => {
60+
const flushOutcomesSpy = vi.spyOn(client as any, '_flushOutcomes');
61+
62+
// Add some logs
63+
sentryCore._INTERNAL_captureLog({ level: 'info', message: 'test log 1' }, client);
64+
sentryCore._INTERNAL_captureLog({ level: 'info', message: 'test log 2' }, client);
65+
66+
// Simulate visibility change to hidden
67+
if (WINDOW.document) {
68+
Object.defineProperty(WINDOW.document, 'visibilityState', { value: 'hidden' });
69+
WINDOW.document.dispatchEvent(new Event('visibilitychange'));
70+
}
71+
72+
expect(flushOutcomesSpy).toHaveBeenCalled();
73+
expect(sentryCore._INTERNAL_flushLogsBuffer).toHaveBeenCalledWith(client);
74+
});
75+
76+
it('flushes logs on flush event', () => {
77+
// Add some logs
78+
sentryCore._INTERNAL_captureLog({ level: 'info', message: 'test log 1' }, client);
79+
sentryCore._INTERNAL_captureLog({ level: 'info', message: 'test log 2' }, client);
80+
81+
// Trigger flush event
82+
client.emit('flush');
83+
84+
expect(sentryCore._INTERNAL_flushLogsBuffer).toHaveBeenCalledWith(client);
85+
});
86+
87+
it('flushes logs after idle timeout', () => {
88+
// Add a log which will trigger afterCaptureLog event
89+
sentryCore._INTERNAL_captureLog({ level: 'info', message: 'test log' }, client);
90+
91+
// Fast forward the idle timeout
92+
vi.advanceTimersByTime(DEFAULT_FLUSH_INTERVAL);
93+
94+
expect(sentryCore._INTERNAL_flushLogsBuffer).toHaveBeenCalledWith(client);
95+
});
96+
97+
it('resets idle timeout when new logs are captured', () => {
98+
// Add initial log
99+
sentryCore._INTERNAL_captureLog({ level: 'info', message: 'test log 1' }, client);
100+
101+
// Fast forward part of the idle timeout
102+
vi.advanceTimersByTime(DEFAULT_FLUSH_INTERVAL / 2);
103+
104+
// Add another log which should reset the timeout
105+
sentryCore._INTERNAL_captureLog({ level: 'info', message: 'test log 2' }, client);
106+
107+
// Fast forward the remaining time
108+
vi.advanceTimersByTime(DEFAULT_FLUSH_INTERVAL / 2);
109+
110+
// Should not have flushed yet since timeout was reset
111+
expect(sentryCore._INTERNAL_flushLogsBuffer).not.toHaveBeenCalled();
112+
113+
// Fast forward the full timeout
114+
vi.advanceTimersByTime(DEFAULT_FLUSH_INTERVAL);
115+
116+
// Now should have flushed both logs
117+
expect(sentryCore._INTERNAL_flushLogsBuffer).toHaveBeenCalledWith(client);
118+
});
119+
});
120+
});

0 commit comments

Comments
 (0)