Feature request: allow enabling log buffering via constructor

[dreamorosi]

Use case

In #3617 we have implemented a mechanism to buffer logs. The next step in the implementation is to make the feature configurable via constructor options.

Solution/User Experience

Customers should be able to configure log buffering via constructor options.

The ConstructorOptions type/object is already pretty crowded (link), so we should add the new settings as a nested object.

In practice, we should add a new optional bufferConfig object to ConstructorOptions and this object should have the following keys all optional:

• enabled: boolean (defaults to true)
• maxBytes - this allows customers to configure the max size of the buffer in bytes (optional defaults to 1024)
• flushOnErrorLog - this allows customers to toggle whether the buffer is flushed when calling logger.error() (optional defaults to true)
• bufferAtVerbosity - this allows customers to configure the threshold at which we'll buffer logs (optional defaults to DEBUG | debug, accepts TRACE, DEBUG, INFO, and WARN plus their corresponding small case versions - you can never buffer ERROR logs)

The configuration logic of the Logger can be found in the setOptions() method. The order in which things are initialized in this method matters, but for this feature I can't think of any dependency so I'd just add it at the end here.

To follow the existing pattern, we should add a new #setLogBuffering() method (feel free to use a better name as long as it starts with the #set prefix). This method:

• is called only if ConstructorOptions['bufferConfig'] is present
• accepts the ConstructorOptions['bufferConfig'] object
• configures the appropriate fields that were added in feat(logger): Add log buffer and flush method #3617 (we might need to move the instantiation of the #buffer here, since the max size is know only at this point)

Then, after this is added, we should also:

• make the flushBuffer() method public
• add the newly public method to the LoggerInterface
• modify the error() method to flush the buffer if log buffering is enabled && flushOnErrorLog is true

Besides the implementation, we should update the unit tests that use the feature to now use both the public config and new public methods as well as add tests for the new config + flush-on-error behavior. Likewise, we should make sure the new types are added and documented properly (see other types in the same file for reference).

Alternative solutions

N/A

Acknowledgment

• This feature request meets Powertools for AWS Lambda (TypeScript) Tenets
• Should this be considered in other Powertools for AWS Lambda languages? i.e. Python, Java, and .NET

Future readers

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

[dreamorosi]

Hey @leandrodamascena, I can't remember if we agreed on a default size for the log buffer in bytes. I checked the RFC but I didn't write it down.

What do you think we should put?

[leandrodamascena]

Hello @dreamorosi! I was testing and I came up with an arbitrary value of 20480 in Python. I saw that the information I save in buffer has on average 200 bytes, so I calculated more or less 100 lines of debug. It seems to be a reasonable number, but since this is still a long way off, what do you think?

Also:

enabled: boolean and required

In Python, I consider that if the Buffer object is passed to the constructor, it will always be true, so I removed this field. I think having the enabled field might be unnecessary, WDYT?

[dreamorosi]

enabled: boolean and required

In Python, I consider that if the Buffer object is passed to the constructor, it will always be true, so I removed this field. I think having the enabled field might be unnecessary, WDYT?

I initially thought the same, but what if customers want to enable the buffering but leave every other setting with default values? They'd have to do this in JS

const logger = new Logger({
  logLevel: 'WARN',
  bufferConfig: {} // buffer is enabled, with default max size, and other default options
});

Imo this is kind of ugly, so I thought of making it explicit by adding enabled But I guess we could also just make it optional so that customers can do this:

const logger = new Logger({
  logLevel: 'WARN',
  bufferConfig: { // this means enabled
    maxBytes: 12345,
  }
});

You're right - I'll update the issue.

For the size, I don't have any strong feelings and will go with the number that you got from the tests.

Thank you for the quick reply!

[ConnorKirk]

I'll work on this next

[leandrodamascena]

Hi @dreamorosi and @ConnorKirk! I don't know if I'm wrong, but I don't see hasEvicted being reset here, which means that if the buffer overflow happens once, it will persist hasEvicted as True and on the next invocation the buffer will be empty - but the CircularMap instance is already in memory - and hasEvicted will be always True, leading to displaying this message every time the buffer is flushed when using the same environment.

Am I wrong? By the way, excellent implementation of hasEvicted 👏

[dreamorosi]

I think the buffer is specific to the request/trace id, so it will start from scratch at the next request.

Or at least that was the intent; I need to check the implementation again on Monday.

[leandrodamascena]

Another question: when you can't add an item to the buffer, probably because it is larger than the entire buffer size, you are adding a logger warning level. Should this be a system warning message instead of a log? I would rather raise a waning, the same as we do with metrics and others, instead of a logger warning.

In Python it makes more sense to be a warning, I don't know about the TS/JS ecosystem, but it seems to be the same thing.

[dreamorosi]

There's a warning module similar to what you're referring to in the Node.js standard lib, but in Lambda it gets treated as a thrown error.

If we don't handle it, customers will see an actual error in their metrics and logs.

If we handle it, functionally speaking, we would end up to the same result of just us calling console.warn (which is what we do now).

Since it'd be just an entry in CloudWatch (current implementation) or an actual unhandled error, I opted for the app warning.

[ConnorKirk]

Hi @dreamorosi and @ConnorKirk! I don't know if I'm wrong, but I don't see hasEvicted being reset here, which means that if the buffer overflow happens once, it will persist hasEvicted as True and on the next invocation the buffer will be empty - but the CircularMap instance is already in memory - and hasEvicted will be always True, leading to displaying this message every time the buffer is flushed when using the same environment.

Am I wrong? By the way, excellent implementation of hasEvicted 👏

You're correct, hasEvicted doesn't get reset when the buffer flushed. Instead, the entire buffer gets deleted. If more logs are buffered after an initial flush, a new buffer is created for it. The hasEvicted property is associated with the buffer, so it gets reset by the deletion/recreation of the buffer.

This might be slightly off-spec - in which case I'm happy to change it.

[leandrodamascena]

You're correct, hasEvicted doesn't get reset when the buffer flushed. Instead, the entire buffer gets deleted. If more logs are buffered after an initial flush, a new buffer is created for it. The hasEvicted property is associated with the buffer, so it gets reset by the deletion/recreation of the buffer.

This might be slightly off-spec - in which case I'm happy to change it.

Hi @ConnorKirk! Thanks for the detailed explanation! I read the code again and I was able to understand the implementation better! In Python (it's true for TS too), I assume that we will never have 2 keys in the buffer due to the Lambda execution model, that is, when the invocation arrives, I need to clear the entire instance and not just the specific key. But this TypeScript implementation makes more sense because it better defines the responsibility of the CircularMap and the Items inside it. I will change the Python implementation for this as well.

Thanks a lot for your contributions here!

[github-actions]

⚠️ 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.