Add context to global error handler

[raycharius]

Summary

When building an app, it's great to be able to have a central error handler where you can centralize all error handling logic. Similar to the way Express handles error middleware. But without the context of the request, the client, and logger, it becomes difficult and requires workarounds.

Wasn't a big change, so went ahead and opened a PR per the guidelines.

As a Dev, I Want To:

• Log with the injected logger.
• Have access to request context to help understand the nature of the error.
• Push a default error view upon error at view submission.
• Respond with a default message upon error at slash command invocation.
• Do some other specific error handling when an error is of a certain type that can appear often in the app.

Now passing the same args passed to a listener, to the error handler. Passed in everything, similar to the way Express passes in the entire req object to all middleware.

I wasn't sure which naming would be appropriate, so I named it middlewareArgs, and seeing as the content can be very different from request to request, also typed it as Object. Happy to improve!

Requirements (place an x in each [ ])

• [x ] I've read and understood the Contributing Guidelines and have done my best effort to follow them.
• [ x] I've read and agree to the Code of Conduct.

[raycharius]

Realized that there is context available at the first call of handleError(), added passing it in.

[seratch]

Personally, I'm positive to have more args for the error handler but I'd like to know others' thoughts.

[seratch]

middlewareArgs's type can be a new type that consists of AnyMiddlewareArgs + context, logger, client (in other words, AllMiddlewareArgs - next)

[aoberoi]

thanks for this PR @raycharius. these use cases are great and these are impactful problems that we need to solve. let's land a change! my apologies for a really long answer here, i just wanted to share as much information as possible with you and the community.

i think the API you've built is nice and simple, but i have two specific concerns:

1. Positional arguments instead of destructured "keyword" arguments:

   Most of the existing API intentionally sends all arguments to callback/listener functions as properties of a single object, so the user can "pick out" just the arguments they want using object destructuring. This also allows maintainers to add new arguments in subsequent releases without affecting the position or order of arguments that users must use. With this change, that pattern is slightly bent. A user might write their error handler's arguments like this: (error, { logger }). That is just different enough from ({ error, logger }), which aligns with existing patterns, to be potentially confusing.

   Aligning with the existing pattern would be a breaking change, and I don't think we should block getting these problems solved on work (or time) it takes to produce @slack/bolt@3.0.0. I think we could release this change and still come back to "clean up" the potentially confusing part later when we're ready for v3.

   But it's worth asking, are there any alternatives we can think of that could be less potentially confusing? Below is one idea that came to mind.

2. Encouraging localized, composable error handling:

   The global error handler is intentionally designed to be useful as a "last resort" so that users are encouraged to handle specific errors closest to where they occur. Part of that design is to limit the amount of context available in the global error handler so that developers don't lump together lots of unrelated (and sometimes error prone) error handling in one place.

   For example, you cited the use case: Push a default error view upon error at view submission. Implementing this would require checking to see if a view argument is present, figure out if it was a view submission or a view closed event, read the default error view template, invoke the template with any error-specific information you have, and then call the views.push Web API method using the trigger_id read from the arguments. I could argue that it's better to put this functionality into a listener middleware function that is (re)used only when view submission listeners are being added. This alternative would save you the first few steps of figuring out if the arguments represent a view submission, and avoid easy mistakes (like trying to handle a view_closed which doesn't have a trigger_id) potentially generating an error in your global error handler - where there's no safety net to catch. As a helpful side-effect developers can write helpful middleware that's more easily reused across their own codebase and with the wider community. In fact, it could also be written as a global middleware. Here's a stab at a listener middleware for fun.

   I believe using middleware for some of the cases you mentioned is better because it can be safer (easier to write safely), and more composable, than to perform the same functionality inside the global error handler. I'd like to try to identify which of those use cases remain better to do in the global error handler. Here how I see them:

   • Log with an injected logger: This need is obvious to me. Even the default global error handler has access to this.logger. It's an oversight that this isn't included.
   • Have access to request context to help understand the nature of the error: This one is tricky. Bolt intentionally hides the idea of a "request" inside the Receiver, so that the app is written in terms of events rather than requests. If what you mean here is access to the body and context arguments, I agree this can be useful. For example, if you want to send errors out to a log aggregation service, you'd likely want as much context as possible available for debugging issues. But adding this might encourage the use of the global event handler in places where we think middleware is a better solution. I'd like to know more about what you'd like to do with this data within the global error handler.
   • Push a default error view upon error at view submission, Respond with a default message upon error at slash command invocation, Do some other specific error handling when an error is of a certain type that can appear often in the app: These are all behaviors I think are a better fit for global or listener middleware.

Alternative: Add data as properties of the error

Let's say we want to expose logger to the error handler for all errors, and we want to expose body and context on any errors that occur while handling an event (this will be most, but not all errors).

We could add these as properties of the Error type that an ErrorHandler receives instead of adding more parameters to an ErrorHandler.

With this in place, one could write a handler using destructuring like this:

app.error(({ message, logger, body }) => {
  logger.error(`global error: ${message}`);
  sendDataToLoggingService({
    type: 'error',
    message,
    body,
  });
});

An advantage is that if you don't care about a single argument (such as logger), but you care about another argument (body), you don't need to list both in your method signature, in the right order, to do it correctly. It also looks more similar to most listeners/middleware in example code. It also extends the pattern in the WebAPIPlatformError object (declared in @slack/web-api).

A disadvantage is that accessing the value as an Error instance is somewhat awkward. Luckily, errors in Node, and errors Bolt generates, don't have any useful methods on them, only data properties. It may still be awkward if you need to pass an error instance into another API. This disadvantage doesn't seem to have a large impact.

Another disadvantage is that previously written code may all of the sudden have access to potentially sensitive data. If I previously logged the entire error argument, and all of the sudden it contained the body property, which contained message data from a user, that would end up in my log stream. Generally we want to help developers avoid logging sensitive data like this when we can. There are a few ways we can mitigate this disadvantage: we can avoid adding the body property, or we can conditionally add the body property when a "developer mode" flag is turned on (this is something we've been casually brainstorming for some time).

[CLAassistant]

All committers have signed the CLA.

[seratch]

I am still wanting to move this forward if we can do it without any breaking changes that may affect existing apps.

[raycharius]

I am still wanting to move this forward if we can do it without any breaking changes that may affect existing apps.

Happy to revisit and get this up to par!

Per Ankur's comment, I think points are very valid when it comes to the trying to stick to the destructuring convention in middleware, and at the same time, out of the two approaches:

app.error((error, { logger, body }) => {
  // Log the error with context
});

And:

app.error(({ message, logger, body }) => {
  // Log the error with context
});

I personally prefer the first. Though it does break the destructuring convention, it definitely won't break any apps, and it's very similar to the way errors are handled in Express middleware:

app.use((error, req, res, next) => {
  // Log the error with context
});

Adding properties to the error object has some (limited) potential to create problems for existing apps, too. If there are any errors being caught in the middleware that are thrown as new errors with parameters from the request to the global handler.

One thing I do think, though, is that perhaps we should limit the middleware args being passed through so that top-level contents of the object is always consistent:

body – has all of the data from the request and is a part of every request. Could be passed into a factory function to retrieve which params should be logged based on the event type.

logger – to have access to the global logger.

client – to have the ability to respond with, say, an error or feedback modal.

context – for any data added during the middleware that comes before.

Let me know what you think, and I'll spend some time on in the next day or two, including the suggestion above to type it out a bit better.

Cheers!

[raycharius]

Also, just saw you added this to 4.0.0 milestone. Above applicable for smaller updates. If it were in a major, breaking release, I think that Ankur's suggestion is a super great and very Bolt-like way to do it:

app.error(({ error, logger, body, context }) => {
  // Logging with context
});

[seratch]

@raycharius Thanks for your prompt reply here! Let's name the two approaches.

A:

app.error((error, { logger, body }) => { });

B:

app.error(({ error, message, logger, body }) => { });

Both sounds nice! But, if I have to choose between these, I prefer B than A too.

As long as we can provide the new way along with the existing simple one by overloading, I think that we can have the changes in v3 series. The only reason why I set v4.0 as its milestone is that I was thinking we may not work on this in the short term, not for possible breaking changes.

[raycharius]

@seratch Totally agree that B is more ⚡ Bolt-ish and more elegant!

What do mean here with overloading in the context of the current handler and JS? Any ideas of how to implement that here without app-level configuration?

Within v3, without breaking changes, the only good options I've been able to come up with are approach A, or providing approach B when configured at app initialization:

const app = new App({
  token: process.env.SLACK_BOT_TOKEN,
  signingSecret: process.env.SLACK_SIGNING_SECRET,
  extendErrorHandler: true,
});

[seratch]

@raycharius I haven't checked the feasibility yet but I was thinking that it may be possible to accept yet another handler type (say, FullArgsErrorHandler - I know this is a terrible name though 😺 ) by having overloaded methods for app.error(...) in TS.

With this way, we may be able to enhance without asking existing apps to update their code. In the case where App gets two types of error handlers, the class may want to do warn-level logging saying "You set two error handlers but we'll be using the full-args one only in the case".

[raycharius]

@seratch Cool, I'll wait for feedback from you then, but from what I can fathom, the function that needs to be overloaded is the function passed into app.error(), but there are a couple of issues with that:

• The developer would have to be using TypeScript.
• Not exactly sure how the app core would know what to pass into the handler without a flag somewhere (either at app initialization, or possibly as a second parameter in the app.error() method).

[seratch]

Good point - yes, perhaps you're right. The flag to determine the type of the function argument would be required in JavaScript.

[raycharius]

Happy to put some work into this and the documentation to get this pushed out.

What do you think about:

const app = new App({
  token: process.env.SLACK_BOT_TOKEN,
  signingSecret: process.env.SLACK_SIGNING_SECRET,
  extendErrorHandler: true,
});

And if true:

app.error(({ error, body, logger }) => {
  // Logging magic
}); 

Including only error, body, and logger will guarantee that the top-level params passed in are consistent, and body should be enough to grab any necessary context for effective logging.

[seratch]

extendErrorHandler: true,

The flag name extendErrorHandler looks good to me but I would like to know other maintainers' thoughts or ideas if they have some.

Including only error, body, and logger will guarantee that the top-level params passed in are consistent, and body should be enough to grab any necessary context for effective logging.

I agree these attributes should be included at least. For the rest in the list of possible arguments, we are unable to have respond and ack without doubt. Regarding context, client, and say, having the access to these may be useful in some situations (they may be missing or incomplete depending on the timing of the error and the type of the incoming request payload).

Happy to put some work into this and the documentation to get this pushed out.

Thank you very much! 🙇‍♂️ We're currently on bolt-js 3.4 along with web-api 6.2 development. These versions focus on better developer experience in TypeScript and major bugfixes. The timing after completing these releases would be easier for us to focus on this task!

[raycharius]

Sounds good!

I'll go ahead and throw in suggested implementation (without documentation) in the next few days, as it's not a big change, so there's a bit more context for the discussion, and from there we can make changes based on any other feedback and comments on implementation.

[raycharius]

I've revamped the implementation as per our discussions above. Looking forward to hearing your thoughts!

• Extended error handler always has guaranteed properties of error, logger, context, body. Since the Bolt-ish way is to destructure args in middleware, in the first call of handleError(), context is passed in as an empty object to ensure there are no issues there, reading property of undefined.
• Added a private property of hasCustomErrorHandler to always know which type of arg to pass into the default error handler if a custom one has not been declared – no need for extended error handler args in the default handler.
• Figured best to leave say() and client to avoid checks in the error handler, if the error is thrown before the request is authed. With the context object, they can easily instantiate Slack SDK WebClient and get the same functionality, and it will be more predictable.

If implementation looks good and this is open for acceptance, I'll go ahead and edit the docs, too.

Cheers!

[seratch]

@raycharius Thank you very much for continuously working hard on this! At the first glance, the latest changes look great to me 👍 If other maintainers have thoughts, I would love to know them 👀

If implementation looks good and this is open for acceptance, I'll go ahead and edit the docs, too.

Regarding the timeline, we are still working on 3.4 development. Perhaps, we can merge this pull request into v3.5 release (as long as we don't bring any breaking changes). I cannot tell the schedule for v3.5 / v4 yet but we will update you before long!

[raycharius]

Hi @seratch! Any idea when this might be able to be merged and released? Thanks!

[seratch]

@raycharius
Thanks for the reminder! I was thinking that we have to wait for the next major (v4) but while checking this change again, I came to think that we can merge this in a minor version (with the default settings, we don't have any breaking changes to existing global error handlers). Let me change the milestone to v3.8. The team is going to release v3.7 soon.

Can you ask you to resolve the conflicts once the v3.7 is released?

[raycharius]

Absolutely! And will also update the docs to reflect the changes once v3.7 has been released – or is that something you like to keep in-house?

[seratch]

@raycharius

And will also update the docs to reflect the changes once v3.7 has been released

Thanks for saying this! Can you create a new pull request for the document updates (=don't include the document changes in this PR)? We would like to merge the document change after the release including your change.

or is that something you like to keep in-house?

As the error handling document section is relatively simple, we are happy to work together with you!

[raycharius]

Sounds good. I'll keep an eye out for 3.7.0, then. And once it's out, I'll resolve the conflicts and open a separate PR for the docs. Thanks!

[codecov]

Codecov Report

Merging #525 (bd68ca6) into main (3495b12) will increase coverage by 0.12%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #525      +/-   ##
==========================================
+ Coverage   71.71%   71.83%   +0.12%     
==========================================
  Files          15       15              
  Lines        1354     1360       +6     
  Branches      402      405       +3     
==========================================
+ Hits          971      977       +6     
  Misses        312      312              
  Partials       71       71              

Impacted Files	Coverage Δ
src/App.ts	83.90% <100.00%> (+0.25%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3495b12...bd68ca6. Read the comment docs.

[raycharius]

@seratch

✅ Conflicts handled

I also refactored the types and overloads. The error handler type tests that were introduced after resolving issue #925 brought to light that the original implementation would have caused compilation errors for those using TS and who hadn't explicitly declared the type of the arg in the error handler as CodedError.

Let me know if the changes look good to you, and I'll send over a separate PR with doc updates.

[seratch]

@raycharius Left one comment. Apart from that, everything looks great to me

[seratch]

Can this type a CodedError? You call asCodedError when passing the value in App.ts L973

[raycharius]

Good question. There are two places where an error is passed to the handler, L749 and L957.

Just above L749, a code is assigned to the error, but at L957, it's not clear whether or not it is a CodedError. Although it will work since there's the const e = error as any line in both blocks of code, my thoughts were that it could easily lead to issues down the road, like someone removing the call of the asCodedError function since the type suggests it is 100% a CodedError

What are your thoughts?

[seratch]

Thanks for your prompt reply. Both lines call this.handleError method, which internally calls asCodeError method to build a CodedError. In the case where an error does not have a code, asCodedError still returns UnknownError, which is a sub type of CodedError. Thus, we can safely assume that the error argument of this.errorHandler method call is always a CodedError. Am I missing something?

[raycharius]

The AllErrorHandlerArgs interface is only used for the type of the params received in this.handleError, before the asCodedError function is called, whereas ExtendedErrorHandlerArgs, which extends AllErrorHandlerArgs (L164) and types error as a CodedError is the type for the args passed into the this.errorHandler method

Maybe the naming needs some work on AllErrorHandlerArgs?

[seratch]

export interface ExtendedErrorHandlerArgs extends AllErrorHandlerArgs

Ah ha, now I understand it. Sorry for bothering you here. I overlooked that there are two args types.

Maybe the naming needs some work on AllErrorHandlerArgs?

I think that naming is fine but it seems that AllErrorHandlerArgs is used only inside App class. Thus, we can hold off having "export" for the types at this moment. Also, having some comments around the types / methods would be helpful for code readers (like me).

[raycharius]

I've added in some comments, let me know if it is back up to par

[seratch]

Looks great to me! Thanks 👍

[seratch]

@raycharius Thanks for working on this change for a long time! Also, we are grateful for your support on the document updates too 🙇

[raycharius]

@seratch Happy to! It was definitely a long one (with a really poor initial implementation) 😆

I'm going to send over a PR for the docs over the next few days. Was thinking of using the expandable content approach seen in this section or this section. It seems to be the default approach to explain more complex use cases

[Ulriichde1st]

Hi,

I'm doing some error handling for the Slack bot application. According to the Slack/Bolt doc, the error handling is as easy as adding extendedErrorHandler: true to the constructor and calling app.error

const app = new App({
  signingSecret: process.env.SLACK_SIGNING_SECRET,
  token: process.env.SLACK_BOT_TOKEN,
  extendedErrorHandler: true, <- Here
});

app.error(({ error, logger, context, body }) => {
  logger.error(error);
});

I applied the same to my application but there's still an error that slipped out from the handling function. Below is the error:

Here is my code:

export const Slackbot = new App({
  token,
  signingSecret,
  // receiver,
  // logLevel: LogLevel.DEBUG,
  socketMode: true, // enable the following to use socket mode
  appToken,
  extendedErrorHandler: true,
});

Slackbot.error(({ error, logger, context, body }) => {
  // Log the error using the logger passed into Bolt
  logger.error(error, "here");
});

Do you know what may be the cause?