Skip to content
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

error #649

Merged
merged 3 commits into from
Nov 26, 2024
Merged

error #649

Show file tree
Hide file tree
Changes from 2 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 .changeset/chilly-cycles-own.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
---
"@opennextjs/aws": minor
---

feat: Add support for OPEN_NEXT_ERROR_LOG_LEVEL

OPEN_NEXT_ERROR_LOG_LEVEL is the minimal error level from which internal errors are logged.
It can be set to:

- "0" / "debug"
- "1" / "warn" (default)
- "2" / "error"
57 changes: 41 additions & 16 deletions packages/open-next/src/adapters/logger.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import type { BaseOpenNextError } from "utils/error";
import { type BaseOpenNextError, isOpenNextError } from "utils/error";
Copy link
Contributor

@sommeeeer sommeeeer Nov 26, 2024

Choose a reason for hiding this comment

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

that BaseOpenNextError is not used anywhere in this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch -> #653


export function debug(...args: any[]) {
if (globalThis.openNextDebug) {
Expand Down Expand Up @@ -42,28 +42,33 @@ const isDownplayedErrorLog = (errorLog: AwsSdkClientCommandErrorLog) =>

export function error(...args: any[]) {
// we try to catch errors from the aws-sdk client and downplay some of them
if (
args.some((arg: AwsSdkClientCommandErrorLog) => isDownplayedErrorLog(arg))
) {
if (args.some((arg) => isDownplayedErrorLog(arg))) {
debug(...args);
} else if (args.some((arg) => arg.__openNextInternal)) {
} else if (args.some((arg) => isOpenNextError(arg))) {
// In case of an internal error, we log it with the appropriate log level
const error = args.find(
(arg) => arg.__openNextInternal,
) as BaseOpenNextError;
if (error.logLevel === 0) {
debug(...args);
const error = args.find((arg) => isOpenNextError(arg))!;
if (error.logLevel < getOpenNextErrorLogLevel()) {
return;
}
if (error.logLevel === 0) {
// Display the name and the message instead of full Open Next errors.
return debug(
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably add some kind of warning just before this line in case openNextDebug is not set, otherwise there will be no error logged and it might be confusing for people.
Or we just use console.log or console.error and rely entirely on OPEN_NEXT_ERROR_LOG_LEVEL

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea.
+1 to drop openNextDebug

...args.map((arg) =>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@conico974 the code does not display the full exception here and for level 1. Let me know what you think about that.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's good, as long as we have the message it's fine

isOpenNextError(arg) ? `${arg.name}: ${arg.message}` : arg,
),
);
}
if (error.logLevel === 1) {
warn(...args);
return;
// Display the name and the message instead of full Open Next errors.
return warn(
...args.map((arg) =>
isOpenNextError(arg) ? `${arg.name}: ${arg.message}` : arg,
),
);
}
console.error(...args);
return;
} else {
console.error(...args);
return console.error(...args);
}
console.error(...args);
}

export const awsLogger = {
Expand All @@ -73,3 +78,23 @@ export const awsLogger = {
warn,
error,
};

/**
* Retrieves the log level for internal errors from the
* OPEN_NEXT_ERROR_LOG_LEVEL environment variable.
*
* @returns The numerical log level 0 (debug), 1 (warn), or 2 (error)
*/
function getOpenNextErrorLogLevel(): number {
const strLevel = process.env.OPEN_NEXT_ERROR_LOG_LEVEL ?? "1";
switch (strLevel.toLowerCase()) {
case "debug":
case "0":
return 0;
case "error":
case "2":
return 2;
default:
return 1;
}
}
8 changes: 8 additions & 0 deletions packages/open-next/src/utils/error.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,3 +39,11 @@ export class FatalError extends Error implements BaseOpenNextError {
this.name = "FatalError";
}
}

export function isOpenNextError(e: any): e is BaseOpenNextError & Error {
try {
return "__openNextInternal" in e;
} catch {
return false;
}
}
5 changes: 1 addition & 4 deletions packages/tests-unit/tests/adapters/cache.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -70,10 +70,7 @@ describe("S3Cache", () => {
beforeEach(() => {
vi.clearAllMocks();

cache = new S3Cache({
_appDir: false,
_requestHeaders: undefined as never,
});
cache = new S3Cache();

globalThis.disableIncrementalCache = false;
globalThis.isNextAfter15 = false;
Expand Down
92 changes: 92 additions & 0 deletions packages/tests-unit/tests/adapters/logger.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
import * as logger from "@opennextjs/aws/adapters/logger.js";
import {
FatalError,
IgnorableError,
RecoverableError,
} from "@opennextjs/aws/utils/error.js";
import { vi } from "vitest";

describe("logger adapter", () => {
describe("Open Next errors", () => {
const debug = vi.spyOn(console, "log").mockImplementation(() => null);
const warn = vi.spyOn(console, "warn").mockImplementation(() => null);
const error = vi.spyOn(console, "error").mockImplementation(() => null);

beforeEach(() => {
// Direct debug to console.log
globalThis.openNextDebug = true;
debug.mockClear();
warn.mockClear();
error.mockClear();
});

afterAll(() => {
globalThis.openNextDebug = false;
});

const ignorableError = new IgnorableError("ignorable");
const recoverableError = new RecoverableError("recoverable");
const fatalError = new FatalError("fatal");

it("default to warn when OPEN_NEXT_ERROR_LOG_LEVEL is undefined", () => {
delete process.env.OPEN_NEXT_ERROR_LOG_LEVEL;
logger.error(ignorableError);
logger.error(recoverableError);
logger.error(fatalError);
expect(debug).not.toHaveBeenCalled();
expect(warn).toHaveBeenCalledWith("RecoverableError: recoverable");
expect(error).toHaveBeenCalledWith(fatalError);
});

it("OPEN_NEXT_ERROR_LOG_LEVEL is 'debug'/'0'", () => {
process.env.OPEN_NEXT_ERROR_LOG_LEVEL = "0";
logger.error(ignorableError);
logger.error(recoverableError);
logger.error(fatalError);
expect(debug).toHaveBeenCalledWith("IgnorableError: ignorable");
expect(warn).toHaveBeenCalledWith("RecoverableError: recoverable");
expect(error).toHaveBeenCalledWith(fatalError);
process.env.OPEN_NEXT_ERROR_LOG_LEVEL = "debug";
logger.error(ignorableError);
logger.error(recoverableError);
logger.error(fatalError);
expect(debug).toHaveBeenCalledWith("IgnorableError: ignorable");
expect(warn).toHaveBeenCalledWith("RecoverableError: recoverable");
expect(error).toHaveBeenCalledWith(fatalError);
});

it("OPEN_NEXT_ERROR_LOG_LEVEL is 'warn'/'1'", () => {
process.env.OPEN_NEXT_ERROR_LOG_LEVEL = "1";
logger.error(ignorableError);
logger.error(recoverableError);
logger.error(fatalError);
expect(debug).not.toHaveBeenCalled();
expect(warn).toHaveBeenCalledWith("RecoverableError: recoverable");
expect(error).toHaveBeenCalledWith(fatalError);
process.env.OPEN_NEXT_ERROR_LOG_LEVEL = "warn";
logger.error(ignorableError);
logger.error(recoverableError);
logger.error(fatalError);
expect(debug).not.toHaveBeenCalled();
expect(warn).toHaveBeenCalledWith("RecoverableError: recoverable");
expect(error).toHaveBeenCalledWith(fatalError);
});

it("OPEN_NEXT_ERROR_LOG_LEVEL is 'error'/'2'", () => {
process.env.OPEN_NEXT_ERROR_LOG_LEVEL = "2";
logger.error(ignorableError);
logger.error(recoverableError);
logger.error(fatalError);
expect(debug).not.toHaveBeenCalled();
expect(warn).not.toHaveBeenCalled();
expect(error).toHaveBeenCalledWith(fatalError);
process.env.OPEN_NEXT_ERROR_LOG_LEVEL = "error";
logger.error(ignorableError);
logger.error(recoverableError);
logger.error(fatalError);
expect(debug).not.toHaveBeenCalled();
expect(warn).not.toHaveBeenCalled();
expect(error).toHaveBeenCalledWith(fatalError);
});
});
});