Skip to content

Feature request: add basic buffering logic #3590

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 11, 2025 · 5 comments · Fixed by #3617
Closed
1 of 2 tasks

Feature request: add basic buffering logic #3590

dreamorosi opened this issue Feb 11, 2025 · 5 comments · Fixed by #3617
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

As part of the implementation of the log buffering feature for Logger we need to implement 1/ the logic that allows customers to enable the buffering and 2/ the logic that makes the logger store and flush logs.

At this stage we won't expose the buffer configuration to customers, this means we'll hardcode the max buffer size as a constant and focus on the logic that actually does the buffering. Since this is a large feature I want to make sure we're able to merge incremental PRs without blocking ourselves from being able to make a release. By not exposing the feature we can work on the internals and optionally even release with an incomplete implementation of this specific feature, as long as it doesn't cause regressions in existing features.

This task depends on #3589.

For a full overview of the feature, please refer to this RFC (#3410) and specifically to this comment for the final spec.

Solution/User Experience

Most of the changes for this task will be localized to packages/logger/src/Logger.ts and pacakges/logger/src/types/Logger.ts, with possibly a few exceptions.

To start, we need to deal with the existing internal buffer that is already present in the Logger class (here). This is a service buffer that is not exposed to customers, and to explain what it is we need to take a step back and understand how Logger is configured and initialized.

The constructor of the class largely pipes all the options into a method called setOptions(). For now it's not important what this method does, but only that during the initialization of the logger there might be conditions under which we need to emit some warnings. These could be things like a misconfiguration or a certain setting being enabled. Because these warnings are emitted before the init phase is completed, we can't log them to stdout or we could risk doing so with a format that is not the one desired by the customer or with missing attributes. The order of how things are initialized matters and changing it is outside of the scope of this issue (but you can ask questions as needed). The key point is that we need to wait till the logger is initialized before emitting these warnings, so we have a buffer just for this.

Since we're now going to create an actual general purpose buffer, we're going to rename this buffer into something else, for example #initBuffer. At least for the time being we'll keep this and the new buffer separate, mainly in order to minimize disruptions or regressions. In the future we might remove this original buffer and use the new one instead.

Once the old buffer has been renamed and the unit tests are still green, we can begin the actual implementation.

As mentioned above, we don't want to expose the feature to customers just yet, so we'll create two new properties in the Logger class, one #maxBufferSizeBytes which is a number that starts at 128 and a protected isBufferEnabled that defaults to false.

This allows us to work on the implementation and even test it. When writing tests, at least for now we'll have to extend the Logger and manually flip the isBufferEnabled to true to enable the feature, for example:

class TestLogger extends Logger {
  public enableBuffering() {
    this.isBufferEnabled = true;
  }
}

const logger = new TestLogger();
logger.enableBuffering(); // now we can test the implementation 

With that out of the way, we need to also import the data structure defined in #3589 and initialize it as a private property of the Logger, i.e. #buffer. For now we won't use the optional callback method, we'll do so in a future task item.

Next, we need to create a new private method that adds item to a buffer. This method should not be concerned about when an item is added to the buffer but only add an item to the right buffer when it's being called. The signature for this method could be something like #bufferLogItem(xrayTraceId: string, log: LogItem, logLevel: number): void and its implementation should do the following:

  • call `log.prepareForPrint()
  • put the log in the buffer using xrayTraceId as key
  • when adding the log to the buffer, do so by stringifying in the same way as here

Next, moving up in the call stack, we'll have to modify the processLogItem() method so that it calls the #bufferLogItem() method under certain conditions. We want to buffer when:

  • the _X_AMZN_TRACE_ID env variable has a value (get it via this.envVarsService.getXrayTraceId();)
  • the log level being logged is one between DEBUG || TRACE1 && isBufferEnabled equals true

When these conditions are met, we want to call the this.createAndPopulateLogItem(logLevel, input, extraInput) method, and pass its result (a LogItem instance) to the newly added #bufferLogItem() method, along with the xrayTraceId, and then immediately return (aka short circuit). This call should probably be wrapped into a try/catch and in case of error we should log a warning informing that we were unable to buffer the log and just log it.

At this point we should have taken care of adding logs to the buffer. Next, we'll turn our attention to flushing the buffer.

We'll add a new protected method called flushBuffer() 2. When called, this method:

  • reads the _X_AMZN_TRACE_ID env variable has a value (get it via this.envVarsService.getXrayTraceId();)
  • if the env variable is not present, it returns immediately resulting in a no-op
  • iterates through the corresponding buffer and logs each item in order of appearance
  • for the logging, use a logic similar to this one and use the log level stored together with the actual log
  • once done logging, it clears the buffer

This is a high level description of what I reckon we need to change to do a basic buffering, however there might be more that I might've missed while writing. We can discuss further as needed.

Together with the implementation for this new logic we'll need new unit tests as well as making sure that existing ones are still working without changes. Coverage should stay the same as it's now.

If possible, let's also add descriptions for each new property and method we add.


Alternative solutions

Acknowledgment

Future readers

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

Footnotes

  1. For now we'll focus on these two log levels only, even tho the RFC indicates this will be configurable. We'll add the possibility to configure this behavior in a later issue.

  2. The method is protected only for now. In a later task we'll make it public but having it protected still allows us to test and use it.

@dreamorosi dreamorosi added feature-request This item refers to a feature request for an existing or new utility logger This item relates to the Logger Utility on-hold This item is on-hold and will be revisited in the future labels Feb 11, 2025
@dreamorosi dreamorosi moved this from Triage to On hold in Powertools for AWS Lambda (TypeScript) Feb 11, 2025
@ConnorKirk
Copy link
Contributor

I can make a PR for this

@dreamorosi
Copy link
Contributor Author

@ConnorKirk - great! Since this is dependent on #3589 let's keep an eye on the linked PR to see where it's going and when to start.

@dreamorosi dreamorosi added confirmed The scope is clear, ready for implementation and removed on-hold This item is on-hold and will be revisited in the future labels Feb 13, 2025
@dreamorosi dreamorosi moved this from Backlog to Working on it in Powertools for AWS Lambda (TypeScript) Feb 13, 2025
@dreamorosi
Copy link
Contributor Author

Hi @ConnorKirk, we merged #3589 so you're good to go for this issue.

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.

@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 19, 2025
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