-
Notifications
You must be signed in to change notification settings - Fork 153
Feature request: flush buffer on uncaught error - Middy.js middleware #3633
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
Comments
Hi @leandrodamascena, we discussed catching the errors and flushing the buffer before re-throwing the customer error - this is clear. I don't remember however if we discuss whether or not we should emit an error log when this happens. At least how I understand it in its current form, we're just going to flush the buffer but we are not doing anything with the error. Should we consider adding a log error with a wording similar to "An uncaught error occurred, flushing the buffer" with the error info on it? |
I'm going to reply it today later. |
I like this idea and I think make sense. In Python I can raise the exception and probably solve this, but idk. |
I'll work on this next |
@ConnorKirk - take a look at this related PR (#3676) while working on this and make sure to rebase. There's a couple of things that you'll have to include. |
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. |
Use case
In #3617 we have implemented a mechanism to buffer logs. The next step in the implementation is to allow customers to flush the buffer whenever there's an uncaught error thrown in the handler scope. This will be an opt-in feature and this item focuses on implementing this behavior for the Middy.js middleware.
Solution/User Experience
As mentioned, this will be an opt-in feature. Customers will be able to enable it by passing a new option to the middleware called
flushBufferOnUncaughtError
:In terms of implementation, we'll have to refactor the current middleware which is implemented here. To help with the implementation, let me give you a quick primer on Middy.js
Middy.js allows middleware authors to set callback functions for 3 events:
onBefore
)onAfter
) but only if the handler does not throwonError
)Currently, since we don't have any special treatment for errors, we implemented it in a way that the same callback is shared between the
onAfter
andonError
phases. As part of this item we'll have to refactor this.The first step would be to change the current
injectLambdaContextAfterOrOnError
so that it's called only foronAfter
. Then, the second would be to define a new callback function specific to theonError
callback. While we are at it, let's also rename all the callbacks to something shorter likeonBefore
,onAfter
, andonError
.The
onAfter
callback will continue to do what it does today, but will also empty (note not flush) the current buffer. This is a proactive measure to free up memory since after the request has finished there's no reason to keep the logs from that request buffered.Focusing on the
onError
one, we'll have to create a callback that simply calls thelogger.flushBuffer()
method whenflushBufferOnUncaughtError
was set totrue
. If log buffering was not enabled in the Logger this should result in a no-op so it's safe to call.In order to add the new config option to the middleware, you'll have to add it to the corresponding type here. Depending on whether you work on this or #3635 first, you might already have the type available.
Other than that, we should make sure the new code is covered by unit tests and the new type in the options is documented with comments.
Alternative solutions
Acknowledgment
Future readers
Please react with 👍 and your use case to help us understand customer demand.
The text was updated successfully, but these errors were encountered: