Skip to content

feat: disable error logging stacktrace in message #1705

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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open
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
45 changes: 45 additions & 0 deletions spec/logger.spec.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
import { expect } from "chai";

import * as logger from "../src/logger";
import { setGlobalOptions } from "../src/v2";
import { getGlobalOptions } from "../src/v2/options";

describe("logger", () => {
const stdoutWrite = process.stdout.write.bind(process.stdout);
Expand Down Expand Up @@ -218,4 +220,47 @@ describe("logger", () => {
});
});
});

describe("error logging stacktrace", () => {
const defaultOptions = getGlobalOptions();

beforeEach(() => {
setGlobalOptions(defaultOptions);
});

afterEach(() => {
setGlobalOptions(defaultOptions);
});
Comment on lines +227 to +233
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

The use of setGlobalOptions within beforeEach and afterEach can lead to repeated calls to this function, potentially resulting in noisy test logs. Consider introducing a test-only helper function in src/v2/options.ts to reset the internal globalOptions variable to avoid this warning.


it("default behavior is to include stacktrace in error logs", () => {
// If this test fails, it means there's a breaking change.
const message = "Test error with stacktrace";
logger.error(message);
const messageOutput = JSON.parse(lastErr.trim()).message;
expect(messageOutput).to.include(`Error: ${message}`);
});

it("when disableErrorLoggingStacktrace is set to false, should include stacktrace in error logs", () => {
const message = "Test error with stacktrace";
setGlobalOptions({
disableErrorLoggingStacktrace: false,
});
logger.error(message);
const messageOutput = JSON.parse(lastErr.trim()).message;

expect(messageOutput).to.include(`Error: ${message}`);
});

it("when disableErrorLoggingStacktrace is set to true, should not include stacktrace in error logs", () => {
const message = "Test error with stacktrace disabled";
setGlobalOptions({
disableErrorLoggingStacktrace: true,
});
logger.error(message);
expectStderr({
severity: "ERROR",
message: message,
});
});
});
});
7 changes: 6 additions & 1 deletion src/logger/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import { format } from "util";
import { traceContext } from "../common/trace";

import { CONSOLE_SEVERITY, UNPATCHED_CONSOLE } from "./common";
import { getGlobalOptions } from "../v2/options";

/**
* `LogSeverity` indicates the detailed severity of the log entry. See [LogSeverity](https://cloud.google.com/logging/docs/reference/v2/rest/v2/LogEntry#logseverity).
Expand Down Expand Up @@ -158,7 +159,11 @@ function entryFromArgs(severity: LogSeverity, args: any[]): LogEntry {

// mimic `console.*` behavior, see https://nodejs.org/api/console.html#console_console_log_data_args
let message = format(...args);
if (severity === "ERROR" && !args.find((arg) => arg instanceof Error)) {
if (
severity === "ERROR" &&
!getGlobalOptions().disableErrorLoggingStacktrace &&
!args.find((arg) => arg instanceof Error)
) {
message = new Error(message).stack || message;
}
const out: LogEntry = {
Expand Down
9 changes: 9 additions & 0 deletions src/v2/options.ts
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,15 @@ export interface GlobalOptions {
* may inadvertently be wiped out.
*/
preserveExternalChanges?: boolean;

/**
* Controls whether error logging should include the stacktrace of the error automatically.
* Defaults to false.
*
Comment on lines +238 to +239
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

The default value for disableErrorLoggingStacktrace is set to false. It would be more intuitive if the default was true, aligning with the description that states 'When true, the error message will not include the stacktrace'.

Suggested change
* Defaults to false.
*
* Defaults to true.

* @remarks
* When true, the error message will not include the stacktrace of the error.
*/
disableErrorLoggingStacktrace?: boolean;
}

let globalOptions: GlobalOptions | undefined;
Expand Down
Loading