Revisit AWS Lambda integration

[shalvah]

I'm using honeybadger-js on an AWS Lambda function. It works (wrapping my handler in Honeybadger.lambdaHandler() catches errors), but I'm noticing a few things:

1. Minor issue: the type definition requires the old callback parameter, when you can just return a Promise these days.
2. It swallows the return value of my handler. You can see from the source that the new handler never returns the return value of my original handler. This means I can't call my handler directly when testing. I have to update my code to do this:

   exports.myHandler = async (event, context) => {...}
   process.env.NODE_ENV === "production" && exports.myHandler = Honeybadger.lambdaHandler(exports.myHandler);

   Additionally, this can lead to subtle bugs in production, because I've written my code using the Promise Lambda version, but Honeybadger is using the callback version, and there are some important differences between the two.
3. I'm not so confident in using domains for the error management. It's mostly worked thus far, but the Node.js docs themselves say you shouldn't use it (also see these issues). A better replacement would be async_hooks, but I think we may not even need any of those—unhandledRejection and uncaughtException, or even a try/catch should be enough. (Also, Node.js 10 is no longer supported, so no need to worry about that.)
4. A nice feature addition would be capturing possible function timeouts, like Sentry does.

I'm willing to explore/implement these myself, just documenting them here for any thoughts.

[joshuap]

@subzero10 any thoughts on this one?

[subzero10]

I'm using honeybadger-js on an AWS Lambda function. It works (wrapping my handler in Honeybadger.lambdaHandler() catches errors), but I'm noticing a few things:

1. Minor issue: the type definition requires the old callback parameter, when you can just return a Promise these days.

Yeah, we could improve the type definition, it is quite basic at the moment.

type LambdaHandler = (event: unknown, context: unknown, callback: unknown) => void|Promise<unknown>

2. It swallows the return value of my handler. You can see from the source that the new handler never returns the return value of my original handler. This means I can't call my handler directly when testing. I have to update my code to do this:

   exports.myHandler = async (event, context) => {...}
   process.env.NODE_ENV === "production" && exports.myHandler = Honeybadger.lambdaHandler(exports.myHandler);

   Additionally, this can lead to subtle bugs in production, because I've written my code using the Promise Lambda version, but Honeybadger is using the callback version, and there are some important differences between the two.

Indeed, the callback vs promise adds additional complexities to the code. It’s harder to read and maintain. We even had to come up with some hacks to support this. I would say the evolution of this code would be to either deprecate the callback version entirely or split in two lambdaHandler functions.

3. I'm not so confident in using domains for the error management. It's mostly worked thus far, but the Node.js docs themselves say you shouldn't use it (also see these issues). A better replacement would be async_hooks, but I think we may not even need any of those—unhandledRejection and uncaughtException, or even a try/catch should be enough. (Also, Node.js 10 is no longer supported, so no need to worry about that.)

Domains have been deprecated for a long time and personally I have never used them in my projects. +1 to go for an alternative solution.

4. A nice feature addition would be capturing possible function timeouts, like Sentry does.

Yes, that would be nice!

Going forward, I suggest we create a few tickets out of this, since they can be tackled in parallel (or independently).

• Replace callbacks with promises (in entire package).
• Deprecate use of process.domain.
• Report aws lambda timeout warning.

What do you think?

[joshuap]

I'm forgetting exactly why we used domains before, but there was a really good reason (and I believe even though they're deprecated, they're still used by some similar packages). I'd want to research to make sure we're not losing anything/adding regressions by removing/replacing them.

I'm good with the other items. Regarding promises vs. callbacks, I'm fine with promises internally. For end users, I just want to make sure that we support whatever it is they're doing with minimal hassle (two functions would be OK).

Does @subzero10's suggestions cover your initial thoughts, @shalvah?

Feel free to create some additional issues so that we can continue these discussions separately (if it makes sense). We could also try using GitHub Discussions to further discuss these items before creating discrete issues. I think I'd like to try adding GitHub Discussions to our workflow in the future, to cut down on issues.

[shalvah]

Yeah, I'm on board.

Re the typedef, maybe we can add this lambda types package, (example post for reference) although that means every user of HB JS will have that included.

deprecate the callback version entirely

I'd prefer this, but it seems the callback version is still supported, though I rarely see it in newer code. Still, our current implementation does two things we need to fix:

1. Doesn't return the return value for an async handler (subtly changing user's intended semantics).
2. Creates a handler that takes 3 arguments (even if yours takes two), which can change behaviour if the AWS runtime checks handler.length before invoking.

Re domains, we could look around, but I believe their main use case was to track context in concurrent async code executed in the same process, but Lambda functions are one process per request, and I think process handlers should be enough for handling errors.

@joshuap I can convert this to a discussion if you think it's a better fit.

[joshuap]

@shalvah thanks, yeah let's try converting to a discussion!

[shalvah]

Ah, now that I think about it, I think domains were used not for error handling, but to store context local to a handler function and its async calls. They're used other places in the codebase, like this.

I think it is worth replacing with the current recommendation, AsyncLocalStorage, but that was only introduced in Node.js v13.10.0, v12.17.0, so that might limit our options. This complicates things.🤔

[subzero10]

Hey all,

As discussed, I created the following tickets:

• AWS Lambda Integration: async handler #677
• Codebase: Callback to Promise based #678
• AWS Lambda integration: Add timeout warning #679

I did not create a ticket about deprecating domains, since it's still under discussion/research.