Feature Request: support BigInt serialization in segment metadata

[misterjoshua]

Expected Behaviour

When I add metadata to my trace, I expect the tracer to handle any data I send to it without causing the lambda to crash.

Current Behaviour

It crashes the lambda when I use putMetadata to add a data structure that contains a BigInt or any other format that JSON.stringify cannot encode.

Code snippet

import middy from '@middy/core';
import { Tracer, captureLambdaHandler } from '@aws-lambda-powertools/tracer';

const tracer = new Tracer({ serviceName: 'stream-handler-test' });

export const handler = middy(async (_event) => {
  console.log('1. Call someLogic.doSomething()');
  const response = await someLogic.doSomething();

  console.log('2. Add the response to the trace metadata');

  // Add the response to the trace, so we can see it in the observability
  // platform in case anything goes wrong.
  //
  // This is a very common pattern for me. I don't usually think about
  // what's in the response unless the data is likely to be giant or
  // contains sensitive information in it. It just so happens that
  // this response contains a BigInt, and that's enough to cause the
  // lambda to crash.
  tracer.putMetadata('response', response);

  console.log('3. Return a response from the handler');

  return `Here is a number: ${response.number}`;
}).use(captureLambdaHandler(tracer));

class SomeLogic {
  async doSomething() {
    return {
      number: 1234n,
    };
  }
}

const someLogic = new SomeLogic();

Steps to Reproduce

1. Bundle and install the script above in an AWS Lambda function with tracing enabled.
2. Run the function from the AWS Lambda console.

Possible Solution

I think there are two factors to consider.

1. We should make an effort to encode the types putMetadata receives when JSON.stringify cannot handle them - perhaps we can use a replacer function to encode exotic types.
2. Even if the tracer fails to encode, I don't think it should ever crash the lambda. Perhaps the tracer should be swallowing all exceptions so it doesn't affect the user's code when something goes wrong.

Powertools for AWS Lambda (TypeScript) version

latest

AWS Lambda function runtime

16.x

Packaging format used

npm

Execution logs

START RequestId: c879e6a8-5a2f-4e1b-9240-7dfce8c41719 Version: $LATEST
2023-09-26T17:29:22.684Z	c879e6a8-5a2f-4e1b-9240-7dfce8c41719	INFO	1. Call someLogic.doSomething()
2023-09-26T17:29:22.704Z	c879e6a8-5a2f-4e1b-9240-7dfce8c41719	INFO	2. Add the response to the trace metadata
2023-09-26T17:29:22.704Z	c879e6a8-5a2f-4e1b-9240-7dfce8c41719	INFO	3. Return a response from the handler
2023-09-26T17:29:22.765Z	c879e6a8-5a2f-4e1b-9240-7dfce8c41719	ERROR	Invoke Error 	{"errorType":"TypeError","errorMessage":"Do not know how to serialize a BigInt","originalError":{"errorType":"TypeError","errorMessage":"Do not know how to serialize a BigInt","stack":["TypeError: Do not know how to serialize a BigInt","    at JSON.stringify (<anonymous>)","    at Subsegment.format (/var/task/index.js:2103:19)","    at Object.send (/var/task/index.js:1434:33)","    at Subsegment.flush (/var/task/index.js:2073:26)","    at Subsegment.streamSubsegments (/var/task/index.js:2083:14)","    at Subsegment.close (/var/task/index.js:2046:18)","    at close (/var/task/index.js:9565:24)","    at captureLambdaHandlerAfter (/var/task/index.js:9581:11)","    at runMiddlewares (/var/task/index.js:9799:23)","    at runRequest (/var/task/index.js:9775:13)"]},"stack":["TypeError: Do not know how to serialize a BigInt","    at JSON.stringify (<anonymous>)","    at Subsegment.format (/var/task/index.js:2103:19)","    at Object.send (/var/task/index.js:1434:33)","    at Subsegment.flush (/var/task/index.js:2073:26)","    at Subsegment.streamSubsegments (/var/task/index.js:2083:14)","    at Subsegment.close (/var/task/index.js:2046:18)","    at close (/var/task/index.js:9565:24)","    at captureLambdaHandlerError (/var/task/index.js:9587:11)","    at runMiddlewares (/var/task/index.js:9799:23)","    at runRequest (/var/task/index.js:9782:13)"]}
END RequestId: c879e6a8-5a2f-4e1b-9240-7dfce8c41719
REPORT RequestId: c879e6a8-5a2f-4e1b-9240-7dfce8c41719	Duration: 139.59 ms	Billed Duration: 140 ms	Memory Size: 128 MB	Max Memory Used: 63 MB	Init Duration: 199.81 ms	
XRAY TraceId: 1-651314f2-6e1cb4753036f8091d8a133e	SegmentId: 1937588641412fd8	Sampled: true	

This issue came from Twitter. https://twitter.com/heitor_lessa/status/1706563622694981916

[dreamorosi]

Hi @misterjoshua thanks for taking the time to open the issue.

As you have correctly pointed out , the JSON.stringify() function doesn't know how to serialize the BigInt and so it crashes. Based on this I'd be more inclined in considering this a feature request given that the behavior observed is idiomatic to the language/runtime.

With that said, I do agree that we should do something about this, so I propose a two steps approach:

Step 1

Add an internal try/catch logic to the putMetadata() method so that in case the SDK throws an error, we catch it and log a warning.

This is a change that we can do right away and that we should do regardless of the serialization discussion.

Step 2

Add support for BigInt objects. The change is relatively simple if made on the X-Ray SDK side, however if made on the Powertools side it requires a more roundabout way given that the SDK implementation doesn't extract the serialization in its own method. For this reason we would have to essentially provide a new implementation for two methods of the Segment object.

I'd like to avoid this option if possible since it'll mean we'll have to make sure our implementation doesn't go out of sync with theirs, so for this reason I have opened an issue on the X-Ray SDK repo and offered to contribute a fix: aws/aws-xray-sdk-node#616.

To PoC the fix, I was able write this passing test:

it('handles BigInt types when adding metadata to a segment', () => {
  const segment = new Subsegment('## foo.bar');

  segment.addMetadata('foo', {
    bar: BigInt(123),
  });

  (
    segment as Subsegment & {
      type: string;
      parent_id: string;
      trace_id: string;
    }
  ).format = function format() {
    this.type = 'subsegment';
    if (this.parent) {
      this.parent_id = this.parent.id;
    }
    if (this.segment) {
      this.trace_id = this.segment.trace_id;
    }

    return JSON.stringify(this, (_, value) => {
      if (typeof value === 'bigint') {
        return value.toString();
      }

      return value;
    });
  };
  segment.toString = function toString() {
    return JSON.stringify(this, (_, value) => {
      if (typeof value === 'bigint') {
        return value.toString();
      }

      return value;
    });
  };

  expect(segment).toEqual(
    expect.objectContaining({
      metadata: {
        default: {
          foo: {
            bar: 123n,
          },
        },
      },
    })
  );
});

As mentioned I'll open a PR for the first step and try to include it in the next release that should go out today/tomorrow at the latest. In the meanwhile, we'll give some time to the X-Ray SDK folks to review my issue and reply.

We'll keep monitoring the progress of that issue and hopefully contribute a PR; however if things take too long we'll reconsider implementing the change on our side either temporarily or permanently.

[dreamorosi]

The PR (#1716) that introduces the try/catch logic has been merged. Ultimately we ended up going in a slightly different direction than the one anticipated. Specifically we didn't add the try/catch to the putMetadata() method but instead we added it to our decorators and middleware.

The reason why we went this way is that after debugging the error and following the stack trace, we noticed that the error described in your issue is thrown only when a segment is emitted, specifically when its serialized, and not when the data is added to the segment. Adding the try/catch logic to the putMetadata() method wouldn't have helped mitigating this.

So we have added try/catch in those places where we call segment.close() on segments that we opened, namely the @captureMethod() and @captureLambdaHandler() decorators and the captureLambdaHandler() Middy middleware.

Unfortunately, until X-Ray merges the fix we are proposing, calling segment.close() on a segment directly will still throw an error.

At the time of writing, we are still waiting on the X-Ray team for the issue we've opened so I'm leaving the issue open and changing its state to status/blocked. I'll report back here once there's movement.

[dreamorosi]

I just wanted to report that I have managed to get through the X-Ray team and we have agreed on a fix. I'll be opening a PR in their repo in the next few days.

[dreamorosi]

The PR is up: aws/aws-xray-sdk-node#619

[misterjoshua]

The PR is up: aws/aws-xray-sdk-node#619

Thanks, Andrea. I pulled down a local copy, tested it, and left a small review.

[misterjoshua]

@dreamorosi Looks like The X-Ray SDK for Node released the BigInt serialization change in v3.5.3.

I updated my personal reproduction to: @aws-lambda-powertools/[email protected], [email protected]

After re-running the lambda, my trace now contains the bigint.

So, I think we're done here unless you can think of anything else?

[dreamorosi]

Hi @misterjoshua thanks for reporting back on the topic.

I'll bump the version of the X-Ray SDK in the Tracer dependencies to the latest and close the issue.

[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.

[github-actions]

This is now released under v1.14.1 version!