feat: improve slack app addon scalability #4284
Conversation
Sonatype Lift is retiringSonatype Lift will be retiring on Sep 12, 2023, with its analysis stopping on Aug 12, 2023. We understand that this news may come as a disappointment, and Sonatype is committed to helping you transition off it seamlessly. If you’d like to retain your data, please export your issues from the web console. |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
src/lib/addons/slack-app.ts
Outdated
| } else if (error.code === ErrorCode.RateLimitedError) { | ||
| this.logger.error( | ||
| `Error handling event ${event.type}. A rate limit error occurred: retry after ${error.retryAfter} seconds`, | ||
| error, | ||
| ); |
There was a problem hiding this comment.
IIRC, WebClient will handle all these for us, right? If that's the case, should the log be an error or just a warn? Maybe it could be an info message saying that "It will be retried" more than "Retry later" which seems to indicate the client/user has to do something
There was a problem hiding this comment.
In this case, this is still an error and should only happen in case WebClient does not handle rate limiting for us.
https://slack.dev/node-slack-sdk/web-api#rate-limits
You might not want to the WebClient to handle rate limits in this way. Perhaps the operation was time sensitive, and it won’t be useful by the time Slack is ready for another request. Or, you have a more sophisticated approach. In these cases, you can set the rejectRateLimitedCalls option on the client to true. Once you set this option, method calls can fail with rate limiting related errors. These errors have a code property set to ErrorCode.RateLimitedError. See error handling for more details.
I've added a callback for the RATE_LIMITED event so we can log it. I'm keeping this error handling for now, but we can remove it if you think it's better. Addressed in 1d2db10
There was a problem hiding this comment.
Yeah, I wasn't concerned about logging when WebClient receives a rate limit and handles it internally, I was rather thinking that the error might not happen.
It's ok what you did in 1d2db10 I'm not sure if client.on(WebClientEvent.RATE_LIMITED is needed (or if it has to be a warn, could be maybe a debug), but we can change this later, so no worries for now
There was a problem hiding this comment.
Ah yeah, fair point. I'll keep it for now then, but I changed it to debug for now: a8a7410
Co-authored-by: Gastón Fournier <gaston@getunleash.io>
| it('should refresh Slack channels cache after 30 seconds', async () => { | ||
| await addon.handleEvent(event, { accessToken }); | ||
|
|
||
| jest.advanceTimersByTime(30000); | ||
|
|
||
| await addon.handleEvent(event, { accessToken }); | ||
|
|
||
| expect(slackApiCalls.length).toBe(2); | ||
| expect(conversationsList).toHaveBeenCalledTimes(2); | ||
| }); |
https://linear.app/unleash/issue/2-1237/explore-slack-app-addon-scalability-and-limitations
Relevant document: https://linear.app/unleash/document/894e12b7-802c-4bc5-8c22-75af0e66fa4b