Skip to content

fix(node): Make log flushing logic more robust #15991

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

Merged
merged 2 commits into from
Apr 7, 2025
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
12 changes: 12 additions & 0 deletions packages/core/src/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -670,6 +670,13 @@ export abstract class Client<O extends ClientOptions = ClientOptions> {
*/
public on(hook: 'afterCaptureLog', callback: (log: Log) => void): () => void;

/**
* A hook that is called when the client is flushing logs
*
* @returns {() => void} A function that, when executed, removes the registered callback.
*/
public on(hook: 'flushLogs', callback: () => void): () => void;

/**
* Register a hook on this client.
*/
Expand Down Expand Up @@ -827,6 +834,11 @@ export abstract class Client<O extends ClientOptions = ClientOptions> {
*/
public emit(hook: 'afterCaptureLog', log: Log): void;

/**
* Emit a hook event for client flush logs
*/
public emit(hook: 'flushLogs'): void;

/**
* Emit a hook that was previously registered via `on()`.
*/
Expand Down
2 changes: 2 additions & 0 deletions packages/core/src/logs/exports.ts
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,8 @@ export function _INTERNAL_flushLogsBuffer(client: Client, maybeLogBuffer?: Array
// Clear the log buffer after envelopes have been constructed.
logBuffer.length = 0;

client.emit('flushLogs');

// sendEnvelope should not throw
// eslint-disable-next-line @typescript-eslint/no-floating-promises
client.sendEnvelope(envelope);
Expand Down
14 changes: 11 additions & 3 deletions packages/core/src/server-runtime-client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,9 @@ import { resolvedSyncPromise } from './utils-hoist/syncpromise';
import { _INTERNAL_flushLogsBuffer } from './logs/exports';
import { isPrimitive } from './utils-hoist';

// TODO: Make this configurable
const DEFAULT_LOG_FLUSH_INTERVAL = 5000;
Copy link
Member Author

Choose a reason for hiding this comment

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

This also exists in the browser SDK.

I'm going to create another PR after this that unifies this as a logging option, and update the develop docs as well.


export interface ServerRuntimeClientOptions extends ClientOptions<BaseTransportOptions> {
platform?: string;
runtime?: { name: string; version?: string };
Expand All @@ -37,6 +40,7 @@ export interface ServerRuntimeClientOptions extends ClientOptions<BaseTransportO
export class ServerRuntimeClient<
O extends ClientOptions & ServerRuntimeClientOptions = ServerRuntimeClientOptions,
> extends Client<O> {
private _logFlushIdleTimeout: ReturnType<typeof setTimeout> | undefined;
private _logWeight: number;

/**
Expand All @@ -54,9 +58,9 @@ export class ServerRuntimeClient<
if (this._options._experiments?.enableLogs) {
// eslint-disable-next-line @typescript-eslint/no-this-alias
const client = this;
client.on('flush', () => {
_INTERNAL_flushLogsBuffer(client);
client.on('flushLogs', () => {
client._logWeight = 0;
clearTimeout(client._logFlushIdleTimeout);
});

client.on('afterCaptureLog', log => {
Expand All @@ -67,7 +71,11 @@ export class ServerRuntimeClient<
// the payload gets too big.
if (client._logWeight >= 800_000) {
_INTERNAL_flushLogsBuffer(client);
client._logWeight = 0;
} else {
// start an idle timeout to flush the logs buffer if no logs are captured for a while
client._logFlushIdleTimeout = setTimeout(() => {
_INTERNAL_flushLogsBuffer(client);
}, DEFAULT_LOG_FLUSH_INTERVAL);
}
});
}
Expand Down
6 changes: 3 additions & 3 deletions packages/core/test/lib/server-runtime-client.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { describe, expect, it, test, vi } from 'vitest';
import { Scope, createTransport } from '../../src';
import type { ServerRuntimeClientOptions } from '../../src/server-runtime-client';
import { ServerRuntimeClient } from '../../src/server-runtime-client';
import { _INTERNAL_captureLog } from '../../src/logs/exports';
import { _INTERNAL_captureLog, _INTERNAL_flushLogsBuffer } from '../../src/logs/exports';

const PUBLIC_DSN = 'https://username@domain/123';

Expand Down Expand Up @@ -256,8 +256,8 @@ describe('ServerRuntimeClient', () => {
_INTERNAL_captureLog({ message: 'test1', level: 'info' }, client);
_INTERNAL_captureLog({ message: 'test2', level: 'info' }, client);

// Trigger flush event
client.emit('flush');
// Trigger flush directly
_INTERNAL_flushLogsBuffer(client);

expect(sendEnvelopeSpy).toHaveBeenCalledTimes(1);
expect(client['_logWeight']).toBe(0); // Weight should be reset after flush
Expand Down
15 changes: 14 additions & 1 deletion packages/node/src/sdk/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { trace } from '@opentelemetry/api';
import { registerInstrumentations } from '@opentelemetry/instrumentation';
import type { BasicTracerProvider } from '@opentelemetry/sdk-trace-base';
import type { DynamicSamplingContext, Scope, ServerRuntimeClientOptions, TraceContext } from '@sentry/core';
import { SDK_VERSION, ServerRuntimeClient, applySdkMetadata, logger } from '@sentry/core';
import { _INTERNAL_flushLogsBuffer, SDK_VERSION, ServerRuntimeClient, applySdkMetadata, logger } from '@sentry/core';
import { getTraceContextForScope } from '@sentry/opentelemetry';
import { isMainThread, threadId } from 'worker_threads';
import { DEBUG_BUILD } from '../debug-build';
Expand All @@ -18,6 +18,7 @@ export class NodeClient extends ServerRuntimeClient<NodeClientOptions> {
private _tracer: Tracer | undefined;
private _clientReportInterval: NodeJS.Timeout | undefined;
private _clientReportOnExitFlushListener: (() => void) | undefined;
private _logOnExitFlushListener: (() => void) | undefined;

public constructor(options: NodeClientOptions) {
const clientOptions: ServerRuntimeClientOptions = {
Expand All @@ -40,6 +41,14 @@ export class NodeClient extends ServerRuntimeClient<NodeClientOptions> {
);

super(clientOptions);

if (this.getOptions()._experiments?.enableLogs) {
this._logOnExitFlushListener = () => {
_INTERNAL_flushLogsBuffer(this);
};

process.on('beforeExit', this._logOnExitFlushListener);
}
}

/** Get the OTEL tracer. */
Expand Down Expand Up @@ -84,6 +93,10 @@ export class NodeClient extends ServerRuntimeClient<NodeClientOptions> {
process.off('beforeExit', this._clientReportOnExitFlushListener);
}

if (this._logOnExitFlushListener) {
process.off('beforeExit', this._logOnExitFlushListener);
}

return super.close(timeout);
}

Expand Down
Loading