Skip to content

fix(pino): support proxied loggers like cls-proxify #18943

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.

Sign up for GitHub

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

Closed
naaa760 wants to merge 3 commits into from
+127 −7
Closed

fix(pino): support proxied loggers like cls-proxify #18943

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a028e22
added proxy target detection for manual tracking scenarios
naaa760 Jan 23, 2026
fa40481
added test case simulating cls-proxify proxy behavior
naaa760 Jan 23, 2026
7224cdc
test configuration for proxied logger scenarios
naaa760 Jan 23, 2026
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
9 changes: 9 additions & 0 deletions dev-packages/node-integration-tests/suites/pino/instrument-proxied.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
import * as Sentry from '@sentry/node';

Sentry.init({
dsn: process.env.SENTRY_DSN,
release: '1.0',
tracesSampleRate: 1.0,
enableLogs: true,
integrations: [Sentry.pinoIntegration({ autoInstrument: false })],
});
36 changes: 36 additions & 0 deletions dev-packages/node-integration-tests/suites/pino/scenario-proxied.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
import * as Sentry from '@sentry/node';
import pino from 'pino';

function createProxiedLogger(originalLogger) {
return new Proxy(originalLogger, {
get(target, prop) {
const value = target[prop];
if (typeof value === 'function') {
return (...args) => {
return value.apply(target, args);
};
}
return value;
}
});
}

const baseLogger = pino({ name: 'proxied-app' });
const proxiedLogger = createProxiedLogger(baseLogger);

Sentry.pinoIntegration.trackLogger(proxiedLogger);

Sentry.withIsolationScope(() => {
Sentry.startSpan({ name: 'startup' }, () => {
proxiedLogger.info({ user: 'user-id', context: 'proxied' }, 'hello from proxied logger');
});
});

setTimeout(() => {
Sentry.withIsolationScope(() => {
Sentry.startSpan({ name: 'later' }, () => {
const child = proxiedLogger.child({ module: 'authentication' });
child.error(new Error('oh no from proxied logger'));
});
});
}, 1000);
89 changes: 82 additions & 7 deletions packages/node-core/src/integrations/pino.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,9 @@ import { addInstrumentationConfig } from '../sdk/injectLoader';

const SENTRY_TRACK_SYMBOL = Symbol('sentry-track-pino-logger');

const trackedLoggers = new WeakSet<Pino>();
const ignoredLoggers = new WeakSet<Pino>();

type LevelMapping = {
// Fortunately pino uses the same levels as Sentry
labels: { [level: number]: LogSeverityLevel };
Expand All @@ -24,6 +27,29 @@ type Pino = {
[SENTRY_TRACK_SYMBOL]?: 'track' | 'ignore';
};

function isPinoLogger(obj: unknown): obj is Pino {
if (!obj || typeof obj !== 'object') {
return false;
}

if ('levels' in obj && obj.levels && typeof obj.levels === 'object' && 'labels' in obj.levels) {
return true;
}

const hasPinoSymbols = Object.getOwnPropertySymbols(obj).some(sym =>
sym.toString().includes('pino')
);
let proto = Object.getPrototypeOf(obj);
while (proto && proto !== Object.prototype) {
if ('levels' in proto && proto.levels && typeof proto.levels === 'object' && 'labels' in proto.levels) {
return true;
}
proto = Object.getPrototypeOf(proto);
}

return hasPinoSymbols;
}

/**
* Gets a custom Pino key from a logger instance by searching for the symbol.
* Pino uses non-global symbols like Symbol('pino.messageKey'): https://github.com/pinojs/pino/blob/8a816c0b1f72de5ae9181f3bb402109b66f7d812/lib/symbols.js
Expand Down Expand Up @@ -118,9 +144,22 @@ const _pinoIntegration = defineIntegration((userOptions: DeepPartial<PinoOptions
log: { ...DEFAULT_OPTIONS.log, ...userOptions.log },
};

function shouldTrackLogger(logger: Pino): boolean {
const override = logger[SENTRY_TRACK_SYMBOL];
return override === 'track' || (override !== 'ignore' && options.autoInstrument);
function shouldTrackLogger(logger: unknown): boolean {
if (!isPinoLogger(logger)) {
return false;
}

const pinoLogger = logger as Pino;

if (ignoredLoggers.has(pinoLogger)) {
return false;
}

if (trackedLoggers.has(pinoLogger)) {
return true;
}

return options.autoInstrument;
Copy link

@cursor cursor bot Jan 23, 2026

Choose a reason for hiding this comment

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

Child loggers not tracked when parent tracked

High Severity

Child loggers created from tracked parent loggers aren't automatically tracked with autoInstrument: false. When logger.child() is called, it returns a new logger instance not present in trackedLoggers. The shouldTrackLogger function checks if the child is in the WeakSet, finds it missing, and returns options.autoInstrument (false), causing child loggers to not be captured. Existing tests like scenario-track.mjs expect child loggers to inherit tracking status from parents but this functionality is missing.

Fix in Cursor Fix in Web

}

return {
Expand Down Expand Up @@ -235,15 +274,51 @@ interface PinoIntegrationFunction {
*
* Requires Pino >=v8.0.0 and Node >=20.6.0 or >=18.19.0
*/
function trackLoggerInstance(logger: Pino): void {
trackedLoggers.add(logger);
ignoredLoggers.delete(logger);

try {
// @ts-expect-error - Checking proxy internals
if (logger.constructor === Proxy) {
const target = (logger as any).__target__ || (logger as any).target;
if (target && isPinoLogger(target)) {
trackedLoggers.add(target);
ignoredLoggers.delete(target);
}
}
Comment on lines +283 to +289
Copy link

@sentry sentry bot Jan 23, 2026

Choose a reason for hiding this comment

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

Bug: The proxy detection logic using logger.constructor === Proxy is incorrect. It will always fail, preventing manually tracked proxied loggers from being instrumented.
Severity: HIGH

Suggested Fix

The proxy detection logic needs to be rewritten. Instead of checking logger.constructor, a more reliable method should be used. Since direct introspection of proxies is limited, consider if the tracking function can be designed to always receive the target object, or if both the proxy and target need to be tracked explicitly. The current implementation attempting to access __target__ or target properties is also incorrect and should be removed.

Prompt for AI Agent 
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: packages/node-core/src/integrations/pino.ts#L283-L289

Potential issue: The mechanism to detect and handle proxied Pino loggers is
fundamentally flawed. The condition `logger.constructor === Proxy` will never evaluate
to true because a proxy's `constructor` property resolves to its target's constructor.
Consequently, the logic to extract the underlying target logger and add it to the
`trackedLoggers` `WeakSet` is dead code. When a user manually tracks a proxied logger
with `autoInstrument: false`, the diagnostic hook receives the target object, but only
the proxy is in the `WeakSet`. This mismatch causes `shouldTrackLogger` to return false,
and no logs are captured.

Did we get this right? 👍 / 👎 to inform future reviews.

Copy link

@cursor cursor bot Jan 23, 2026

Choose a reason for hiding this comment

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

Proxy detection check will never execute

High Severity

The check logger.constructor === Proxy will never be true because accessing constructor on a proxy goes through the proxy's handler, which returns the target's constructor, not Proxy. JavaScript provides no standard way to detect proxies. When trackLogger(proxiedLogger) is called, only the proxy gets added to trackedLoggers, but the diagnostic channel receives the target logger (due to .apply(target, args) in the proxy handler binding this to the target). Since the target isn't tracked, proxied loggers fail to capture logs when autoInstrument: false.

Additional Locations (1)

Fix in Cursor Fix in Web

} catch {
// Ignore errors when trying to access proxy internals
}
}

function untrackLoggerInstance(logger: Pino): void {
ignoredLoggers.add(logger);
trackedLoggers.delete(logger);

try {
// @ts-expect-error - Checking proxy internals
if (logger.constructor === Proxy) {
const target = (logger as any).__target__ || (logger as any).target;
if (target && isPinoLogger(target)) {
ignoredLoggers.add(target);
trackedLoggers.delete(target);
}
}
} catch {
// Ignore errors
}
}

export const pinoIntegration = Object.assign(_pinoIntegration, {
trackLogger(logger: unknown): void {
if (logger && typeof logger === 'object' && 'levels' in logger) {
(logger as Pino)[SENTRY_TRACK_SYMBOL] = 'track';
if (isPinoLogger(logger)) {
trackLoggerInstance(logger as Pino);
}
},
untrackLogger(logger: unknown): void {
if (logger && typeof logger === 'object' && 'levels' in logger) {
(logger as Pino)[SENTRY_TRACK_SYMBOL] = 'ignore';
if (isPinoLogger(logger)) {
untrackLoggerInstance(logger as Pino);
}
},
}) as PinoIntegrationFunction;
Loading