Skip to content

Feature request: log a warning when buffer overflows #3625

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

Closed
1 of 2 tasks
dreamorosi opened this issue Feb 19, 2025 · 3 comments · Fixed by #3639
Closed
1 of 2 tasks

Feature request: log a warning when buffer overflows #3625

dreamorosi opened this issue Feb 19, 2025 · 3 comments · Fixed by #3639
Assignees
Labels
completed This item is complete and has been merged/shipped feature-request This item refers to a feature request for an existing or new utility logger This item relates to the Logger Utility

Comments

@dreamorosi
Copy link
Contributor

Use case

In #3617 we have implemented a mechanism to buffer logs. The log buffer is designed to be a circular buffer that evicts from the buffer the oldest items when adding a log that is too big to fit.

As a customer I want to have an indication that this eviction happened when I flush the logs from the buffer. This is because when reading the buffer I might notice missing log entries, so to avoid confusion there should be a warning log that informs me of the eviction.

Solution/User Experience

At first, when designing the feature, we considered emitting a warning every time a log is removed from the buffer because of an overflow. At the end we opted to not go in this direction because we realized that this could cause a lot of warning logs in certain noisy systems and also, if the buffering threshold was set to warn this log could've gone missing.

To get around both issues we instead decided to emit this log only when the log buffer is flushed. This is where we think customers will expect to see this log. If the buffer is never flushed, then there's no reason for customers to care that the log buffer overflowed. If instead the log is flushed, we think that this is when customers might go look at logs, and at that point knowing why some of the logs might be missing would be the most helpful.

To implement this mechanism we'll have to introduce a new internal flag in the CircularMap class, perhaps a public hasBufferOverflown (feel free to find a better name) that starts out as false and is then flipped to true when the buffer has actually overflown.

To know when this has happened, we will likely have to use the #deleteFromBufferUntilSizeIsLessThanMax (what a mouthful aha) method in CircularMap to flip the hasBufferOverflown flag to true.

Then, we could use this hasBufferOverflown in the flush() method of Logger and in case it's true, emit a warning after having flushed the buffer that says something along the lines of "Some of the log entries might be missing because the buffer overflew and some got removed" (feel free to reword this or check how Powertools for AWS Lambda (Python) worded it).

So to recap:

  • add a new property to CircularMap that keeps a state about whether the buffer has ever overflowed
  • if the buffer ever deletes a record from the buffer because it was full (but not when it's emptied), then flip the state to true
  • refactor the flushBuffer method in Logger to read this new flag from the circular map, and if true, emit a warning that informs the customer that some logs might be missing. This warning should go after the logs in the buffer

As a side note, if the implementation above works, the #onBufferOverflow is now likely obsolete/redundant. Initially I thought we could let the Logger set a callback, but then realized I was not considering the fact that each buffer must keep its own state, so the logger doesn't need a callback in this case. For now I'd be inclined to leave the option there, perhaps in the future it can be used for something else if we want to use the buffer elsewhere.

Alternative solutions

Acknowledgment

Future readers

Please react with 👍 and your use case to help us understand customer demand.

@dreamorosi dreamorosi added confirmed The scope is clear, ready for implementation feature-request This item refers to a feature request for an existing or new utility logger This item relates to the Logger Utility labels Feb 19, 2025
@dreamorosi dreamorosi moved this from Triage to Backlog in Powertools for AWS Lambda (TypeScript) Feb 19, 2025
@ConnorKirk
Copy link
Contributor

I can do this

@dreamorosi dreamorosi moved this from Backlog to Working on it in Powertools for AWS Lambda (TypeScript) Feb 19, 2025
@github-actions github-actions bot added pending-release This item has been merged and will be released soon and removed confirmed The scope is clear, ready for implementation labels Feb 21, 2025
@dreamorosi dreamorosi linked a pull request Feb 21, 2025 that will close this issue
@dreamorosi dreamorosi moved this from Working on it to Coming soon in Powertools for AWS Lambda (TypeScript) Feb 21, 2025
Copy link
Contributor

⚠️ COMMENT VISIBILITY WARNING ⚠️

This issue is now closed. Please be mindful that future comments 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.

Copy link
Contributor

This is now released under v2.15.0 version!

@github-actions github-actions bot added completed This item is complete and has been merged/shipped and removed pending-release This item has been merged and will be released soon labels Feb 25, 2025
@dreamorosi dreamorosi moved this from Coming soon to Shipped in Powertools for AWS Lambda (TypeScript) Feb 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
completed This item is complete and has been merged/shipped feature-request This item refers to a feature request for an existing or new utility logger This item relates to the Logger Utility
Projects
Development

Successfully merging a pull request may close this issue.

2 participants