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

feat: GuLambdaFunction should use JSON logging by default #2260

Merged
merged 5 commits into from
Apr 11, 2024

Conversation

NovemberTang
Copy link
Contributor

@NovemberTang NovemberTang commented Mar 21, 2024

What does this change?

I think JSON log formatting is a more sensible default value.

There are two log formats in AWS Lambda, Text, and JSON. Text is the default (possibly for historical reasons?), but has disadvantages, such as not being as easily machine-readable.

Using Text means that any changes made to the ApplicationLogLevel will be flat out ignored (defaulting to DEBUG as far as I can tell). Application log filtering only works when the log format is JSON.

I've verified that Lambdas using this configuration are still compatible with Central ELK. Repocop has been using JSON logging for several weeks without issue.

How to test

  • Verify default behaviour of lambdas has changed
  • Verify that providing the Text value results in the expected snapshot

How can we measure success?

Teams will be able to filter their application logs by severity level without having to do as much googling/asking around as I had to

Have we considered potential risks?

This will cause snapshot changes to everyone using a GuLambdaFunction or its descendants. This will be slightly disruptive, but no work is required beyond updating the snapshot tests.

To maintain the previous behaviour, users should add the following to their lambda props

logFormat: "Text"

Checklist

  • I have listed any breaking changes, along with a migration path 1
  • I have updated the documentation as required for the described changes 2

Footnotes

  1. Consider whether this is something that will mean changes to projects that have already been migrated, or to the CDK CLI tool. If changes are required, consider adding a checklist here and/or linking to related PRs.

  2. If you are adding a new construct or pattern, has new documentation been added? If you are amending defaults or changing behaviour, are the existing docs still valid?

Copy link

changeset-bot bot commented Mar 21, 2024

⚠️ No Changeset found

Latest commit: 114d055

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@NovemberTang NovemberTang marked this pull request as ready for review March 21, 2024 22:24
Copy link
Member

@akash1810 akash1810 left a comment

Choose a reason for hiding this comment

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

Just to double check - does https://github.com/guardian/cloudwatch-logs-management apply the log markers to JSON format logs?

src/constructs/lambda/lambda.ts Outdated Show resolved Hide resolved
@NovemberTang NovemberTang changed the title feat: GuLambdaFunction uses JSON logging by default feat: GuLambdaFunction should use JSON logging by default Apr 5, 2024
@NovemberTang
Copy link
Contributor Author

Just to double check - does https://github.com/guardian/cloudwatch-logs-management apply the log markers to JSON format logs?

It seems like it to me. Here's a link to the repocop logs, which have been using JSON formatting for months.

@NovemberTang NovemberTang force-pushed the nt/lambda-json-logging branch from 3e42224 to bb8f340 Compare April 11, 2024 07:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants