Skip to content

feat(logger): Emit a warning on buffer flush #3639

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

Summary

This PR adds a warning to the flushBuffer method when the buffer being flushed has had logs evicted.

Changes

  • Add a new property, evictionCount to the SizedSet class. It is incremented when a log is evicted from the set.
  • Update flushBuffer method to emit a warning if there any logs have been evicted from the buffer being flushed
  • Add a test to verify that the warning is emitted when a buffer that contained an evicted log is flushed.

Things to note:

  1. Rather than track the presence/absence of an eviction, I thought it might be useful to track the count. It's trivial to swap back if you prefer. This is a slight deviation from the design. Might it be useful to see "100 logs have been evicted" rather than just "Logs have been evicted"?
  2. Each trace(/buffer) may or may not have evicted logs. Rather than storing a boolean on the CircularMap, I think it would be more accurate to store whether each buffer has had an eviction. I chose to add this to the SizedSet class, but another Map on the CircularMap mapping traceId to eviction status could also work.

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

Issue number: #3625


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.

This commit adds a warning to the flushBuffer method when the buffer being flushed has had logs evicted.
@boring-cyborg boring-cyborg bot added logger This item relates to the Logger Utility tests PRs that add or change tests labels Feb 20, 2025
@pull-request-size pull-request-size bot added the size/M PR between 30-99 LOC label Feb 20, 2025
@ConnorKirk ConnorKirk changed the title feat(Logger): Emit a warning on buffer flush feat(logger): Emit a warning on buffer flush Feb 20, 2025
@ConnorKirk ConnorKirk marked this pull request as ready for review February 20, 2025 21:02
@ConnorKirk ConnorKirk requested review from a team as code owners February 20, 2025 21:02
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.

Thank you for this PR!

Feel free to go on the issue list and leave a comment under the next one, so I can assign it to you.

Well done!

@dreamorosi dreamorosi merged commit f471552 into aws-powertools:main Feb 20, 2025
41 checks passed
@ConnorKirk ConnorKirk deleted the 3625-log-warning-on-buffer-overflow branch February 21, 2025 08:21
@github-actions github-actions bot added the feature PRs that introduce new features or minor changes label Feb 21, 2025
@dreamorosi dreamorosi linked an issue Feb 21, 2025 that may be closed by this pull request
2 tasks
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/M PR between 30-99 LOC tests PRs that add or change tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: log a warning when buffer overflows
2 participants