Skip to content

feat(logger): Add log buffer and flush method #3617

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

Merged

Conversation

ConnorKirk
Copy link
Contributor

@ConnorKirk ConnorKirk commented Feb 18, 2025

Summary

This PR implements basic buffering logic to the logger utility, and the flushLogger method for flushing the buffered logs.

Changes

  • Move logBuffer test files to logBufferStructures test file.
  • Add the bufferLog and flushBuffer methods to the Logger class. The buffer functionality is disabled at the moment.

Other things to note:

  • I added a property to set the log buffer threshold (log buffer level?). I needed to do this to test an edge case and meet coverage requirement. In this PR it isn't configurable but could be in future. The feature request originally specified buffer logs equal or less than debug.
  • I was wondering about a possible edge case - At the moment each set in the logger buffer is limited to a max size. There isn't a mechanism to limit the number of sets in the buffer. Probably a contrived issue, but might be worth considering.

Issue number: closes #3590


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.

@pull-request-size pull-request-size bot added the size/L PRs between 100-499 LOC label Feb 18, 2025
@boring-cyborg boring-cyborg bot added logger This item relates to the Logger Utility tests PRs that add or change tests labels Feb 18, 2025
This commit implements basic buffering logic
to the logger utility, and the `flushLogger`
method for flushing the buffered logs.
@ConnorKirk ConnorKirk force-pushed the 3590-add-basic-buffering-logic branch from c4c1f3d to fb896e8 Compare February 18, 2025 16:13
@dreamorosi dreamorosi self-requested a review February 18, 2025 17:00
Copy link
Contributor

@dreamorosi dreamorosi left a comment

Choose a reason for hiding this comment

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

Great job, Connor!

The PR is in a really good state already, I have left a few minor comments mainly about project conventions and similar, but the implementation looks good overall.

I haven't focused too much on tests beyond looking at coverage since I expect that we'll need to refactor some of them after we make the methods public.

While you address the comments, also please look at these from the SonarCloud scanning.

Tomorrow morning I'll do a quick test in a Lambda function mainly to validate the logic around the X-Ray trace ID and if everything looks good we'll merge it as soon as the feedback below is addressed.

Great work again!

@github-actions github-actions bot added feature PRs that introduce new features or minor changes labels Feb 18, 2025
@ConnorKirk ConnorKirk marked this pull request as ready for review February 19, 2025 11:29
@ConnorKirk ConnorKirk requested review from a team as code owners February 19, 2025 11:29
@ConnorKirk ConnorKirk requested a review from hjgraca February 19, 2025 11:29
@dreamorosi dreamorosi requested review from dreamorosi and removed request for hjgraca February 19, 2025 12:33
Copy link
Contributor

@dreamorosi dreamorosi left a comment

Choose a reason for hiding this comment

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

Great work on this Connor, and congrats for getting your first PR merged in this repo!

@dreamorosi dreamorosi merged commit 6968ca8 into aws-powertools:main Feb 19, 2025
41 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature PRs that introduce new features or minor changes logger This item relates to the Logger Utility size/L PRs between 100-499 LOC tests PRs that add or change tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: add basic buffering logic
2 participants