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

fix(parser): CloudWatch Log Envelope handles non-JSON #3505

Merged
merged 3 commits into from
Jan 22, 2025

Conversation

dreamorosi
Copy link
Contributor

@dreamorosi dreamorosi commented Jan 21, 2025

Summary

Changes

Please provide a summary of what's being changed

This PR fixes theCloudWatchEnvelope built-in envelope so that it correctly handles log messages that are not JSON strings. The previous implementation forced the parsing to always attempt a JSON.parse which would fail in the case of non-JSON-structured log messages.

For example, parsing a series of log messages from an AWS Lambda function invocation would fail when encountering messages like this:

{
  "id": "38569604798453153764115268074363468419149517103543812096",
  "timestamp": 1729520895158,
  "message": "2024-10-21T14:28:15.158Z\teadc91c8-8273-4a2d-b170-b0a7883b619e\tINFO\tHello from other.ts\n"
},
{
  "id": "38569604798453153764115268074363468419149517103543812097",
  "timestamp": 1729520895158,
  "message": "START RequestId: eadc91c8-8273-4a2d-b170-b0a7883b619e Version: $LATEST\n"
},
{
  "id": "38569604798475454509313798697505004137422165465049792514",
  "timestamp": 1729520895159,
  "message": "{\"level\":\"INFO\",\"message\":\"Hello from other.ts\",\"sampling_rate\":0,\"service\":\"service_undefined\",\"timestamp\":\"2024-10-21T14:28:15.158Z\",\"xray_trace_id\":\"1-671664ff-69b0ec7624423e04544c023f\"}\n"
},

This PR fixes this behavior by removing the forced JSON parsing and instead letting the customer set the parsing if they know their message is JSON-structured, i.e.

const result = CloudWatchEnvelope.safeParse(
  event,
  JSONStringified(
    z.object({
      level: z.string(),
      message: z.string(),
      sample_rate: z.number(),
    })
  )
);

The PR also refactored the unit tests for both the schema & the envelope, as well as removing the usage of Envelope in favor of an implementation in the CloudWatchEnvelope that now includes the correct full path in the error, i.e.

new ParseError(
  'Failed to parse CloudWatch Log message at index 0', // works also for multiple indexes
  {
    cause: new ZodError([
      {
        code: 'custom',
        message: 'Invalid JSON',
        path: ['awslogs', 'data', 'logEvents', 0, 'message'],
      },
    ]),
  }
)

The PR also adds a new custom error that is thrown when either the unzip or decode operations fail, since the error thrown by the standard library is rather opaque and a generic Error.

While working on this PR I also removed another batch of test event files that were not used nor referenced anywhere.

Please add the issue number below, if no issue is present the PR might get blocked and not be reviewed

Issue number: fixes #3225


By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.

@dreamorosi dreamorosi self-assigned this Jan 21, 2025
@dreamorosi dreamorosi requested review from a team as code owners January 21, 2025 15:09
@boring-cyborg boring-cyborg bot added parser This item relates to the Parser Utility tests PRs that add or change tests labels Jan 21, 2025
@pull-request-size pull-request-size bot added the size/XXL PRs with 1K+ LOC, largely documentation related label Jan 21, 2025
@github-actions github-actions bot added bug Something isn't working labels Jan 21, 2025
@dreamorosi dreamorosi requested a review from am29d January 21, 2025 15:18
Copy link
Contributor

@am29d am29d left a comment

Choose a reason for hiding this comment

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

Minor qustion on why we deleted other events that has nothing to do with CloudWatch.

@dreamorosi dreamorosi requested a review from am29d January 22, 2025 13:46
@am29d am29d merged commit 781a14e into main Jan 22, 2025
38 checks passed
@am29d am29d deleted the fix/cloudwatch_envelope branch January 22, 2025 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working parser This item relates to the Parser Utility size/XXL PRs with 1K+ LOC, largely documentation related tests PRs that add or change tests
Projects
Development

Successfully merging this pull request may close these issues.

Bug: CloudWatch envelope assumes data is always a JSON
2 participants