Bug: logging fails if input contains bigint values

[samuelsulo]

Bug (logger): logging fails if input contains bigint values

Since JSON.stringify is unable to serialize bigint values, the logging function raises an exception if the input contains a bigint value.

Expected Behavior

The logger should handle bigints to prevent JSON.stringify from raising an exception.

Log Request:

import { Logger } from "@aws-lambda-powertools/logger";

const logger = new Logger();
logger.info('Object with bigint value', { myBigInt: BigInt(1234) })

Log Result:

{
  "level": "INFO",
  "message": "Object with bigint value",
  "service": "service_undefined",
  "timestamp": "2022-10-12T22:27:17.067Z",
  "myBigInt": "1234"
}

Current Behavior

The logger does not handle bigint values which causes JSON.stringify to raise an exception.

Log Request:

import { Logger } from "@aws-lambda-powertools/logger";

const logger = new Logger();
logger.info('Object with bigint value', { myBigInt: BigInt(1234) })

Raised exception:

TypeError: Do not know how to serialize a BigInt
    at JSON.stringify (<anonymous>)
    ...

Possible Solution

Handling bigint values in the removeCircularDependencies method of the Logger class should fix the issue.

Before

private removeCircularDependencies(): (key: string, value: LogAttributes | Error) => void {
    const references = new WeakSet();

    return (key, value) => {
      let item = value;
      if (item instanceof Error) {
        item = this.getLogFormatter().formatError(item);
      }
      if (typeof item === 'object' && value !== null) {
        if (references.has(item)) {
          return;
        }
        references.add(item);
      }

      return item;
    };
  }

After

private removeCircularDependencies(): (key: string, value: LogAttributes | Error | bigint) => void {
    const references = new WeakSet();

    return (key, value) => {
      let item = value;
      if (typeof item === 'bigint') {
        return item.toString();
      }
      if (item instanceof Error) {
        item = this.getLogFormatter().formatError(item);
      }
      if (typeof item === 'object' && value !== null) {
        if (references.has(item)) {
          return;
        }
        references.add(item);
      }

      return item;
    };
  }

Environment

• Powertools version used: 1.2.1
• Packaging format (Layers, npm): npm
• AWS Lambda function runtime: nodejs16.x
• Debugging logs: none

Related issues, RFCs

[dreamorosi]

Hi @Samuel-Sulo, thank you for reporting this issue.

Please allow us some time to review it and get back to you.

From a quick search it seems that according to the tc39 proposal that introduced support for BigInt, the object is not serializable to JSON on purpose (see Other Exceptions section). The document also advises against coercion between Number and BigInt mentioning a potential lack of precision.

I'm not sure how I feel about changing the type to String, but based on the above I would be more inclined in categorizing this as Feature request rather than a Bug.

@saragerion, would appreciate your POV on this one before deciding if/how to prioritize it.

[dreamorosi]

For future implementer, unless there's a better option, we have settled on implementing this as converting the number to a string (BigInt.toString()).

If anyone wants to pick this up, please leave a comment here to discuss the changes before opening a PR.

[shdq]

Hey @dreamorosi

I made a quick fix for this and wrote a couple of tests, and I also suggest renaming removeCircularDependencies() to handleTypeErrors() or even more generic jsonReplacerFn() and updating the description since it does more than just remove circular dependencies. I think we covered both of the possible typeErrors. Can I open a PR?

I also want to do a small refactor of removeEmptyKeys() function. We can get rid of the pickBy dependency by rewriting it. It seems like the case when LogItem has empty, undefined, and null values is not tested at all. Should I open a new issue about that?

[dreamorosi]

Hi @shdq thanks for looking into this one!

This is great to hear, please feel free to open a PR with both changes, since they seem to be relatively connected.

Just like other instances, I'm more than in favor of reducing/removing dependencies as long as the functionality remains the same, which in this case means the output is the same.

Happy to hear also about the extra test cases.

Looking forward to review the PR with both changes, no need to do more "paperwork" on this one.

But thanks a lot for being considerate and asking!

[github-actions]

⚠️ COMMENT VISIBILITY WARNING ⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.