fix(logger): do not synthesize error stack on console.error in logger…

[shettyvarun268]

Description

Fixes #1945

Background:

When users import firebase-functions/logger/compat, it monkeypatches global console.* methods to format logs into structured Cloud Logging JSON lines.

Previously, patchedConsole("ERROR") unconditionally created a synthetic Error stack for all console.error calls via message = new Error(message).stack || message.

This caused two problems:

1. Benign Node.js runtime warnings escalated into false application crashes: Node.js internal warning handlers (such as MaxListenersExceededWarning triggered when connection pooling with libraries like axios) print warnings via console.error. logger/compat intercepted these string warnings and attached a fake stack trace pointing to patchedConsole inside compat.js. Google Cloud Error Reporting then interpreted these harmless warnings as active application crashes in the function runtime.
2. Error instance stack corruption: When actual Error objects were passed to console.error(err), util.format(err) already rendered the callsite stack trace. Wrapping it again in new Error(message).stack resulted in a distorted Error: Error: ... double-wrapped stack trace.

Changes:

1. src/logger/compat.ts: Removed new Error(message).stack wrapper from patchedConsole. String logs and Node runtime warnings to console.error are now output directly with ERROR severity without synthetic stack traces, while actual Error objects retain their authentic callsite stack.
2. spec/logger.spec.ts: Added a describe("compat", ...) test block covering console.log, console.info, console.debug, console.warn, and console.error (handling strings, formatted templates, and Error objects).

Code sample

require("firebase-functions/logger/compat");

// Output before:
// {"severity":"ERROR","message":"Error: (node:...) MaxListenersExceededWarning...\n    at patchedConsole (/compat.js:31:17)..."}

// Output after:
// {"severity":"ERROR","message":"(node:...) MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 testEvent listeners added to [EventEmitter]..."}

Release notes

relnote: Fixed an issue where firebase-functions/logger/compat added synthetic Error stack traces to string console.error calls and Node.js runtime warnings.

[gemini-code-assist]

Code Review

This pull request removes the synthetic Error stack trace generation for string messages in console.error to prevent standard logs and runtime warnings from being misclassified as unhandled exceptions in Cloud Error Reporting. It also introduces a test suite to verify the console patching behavior. The review feedback points out a critical test isolation issue where Node's require cache and pre-existing patches could cause tests to interfere with each other, and provides a robust solution to clear the cache and restore the console using guaranteed unpatched references.

[shettyvarun268]

/gemini review

[gemini-code-assist]

Code Review

This pull request updates the console compatibility layer in src/logger/compat.ts to stop synthesizing Error stacks for console.error string logs, ensuring standard Node.js console behavior and preventing misclassification in Cloud Error Reporting. It also adds a comprehensive test suite in spec/logger.spec.ts. The review feedback highlights a bug where calling console methods with no arguments outputs the string "undefined" instead of an empty string, suggesting a fix using rest parameters. Additionally, the feedback recommends adding a test case for empty arguments and resetting global test variables in a beforeEach hook to prevent test pollution.

[IzaakGough]

lgtm!

[wandamora]

LGTM, just one nit.

[ajperel]

Are there any cases where we still want this behavior if the message was from an actual stack trace or something? I'm not sure if in addition to fixing a real problem we're regressing in some cases where we actually wanted this behavior. Someone wrote it this way for a reason the first time but I don't know why. I'd be curious if you did any research into this and what you found.

[shettyvarun268]

I looked into the git history to see why it was written this way. logger/compat was originally added in v3 (PR #701) purely as a drop-in shim to output structured JSON without any stack synthesis. In v4 (PR #1161), when the main SDK logger (functions.logger.error) was updated to synthesize stacks for Cloud Error Reporting, that line was copied over to compat.ts as well (and without the instanceof Error check).

While synthesizing a stack makes sense when someone explicitly calls functions.logger.error("..."), doing it in console.error causes Node runtime warnings (like MaxListenersExceededWarning from connection pools) to get decorated with a fake stack trace and misclassified as application crashes in Cloud Error Reporting. It also double-wrapped actual Error objects passed to console.error(err).

Taking this out doesn't regress intended behavior: console.error(err) with real Error objects still outputs the genuine stack trace through util.format, string logs will still be categorized as severity: "ERROR", and the main SDK logger (src/logger/index.ts) remains untouched. This just restores logger/compat to its original intended behavior.

[ajperel]

OK. Thanks for digging a bit more. I'm convinced.

[ajperel]

OK. Thanks for digging a bit more. I'm convinced.