Handle rate limit errors from Slack on new alert creation #1986
Conversation
|
WalkthroughThe update consolidates Slack conversation pagination logic into a single function by removing the helper Changes
Sequence Diagram(s)sequenceDiagram
participant Client as WebClient
participant Presenter as getAllSlackConversations
loop While more pages
Presenter->>Client: conversations.list (limit: 999, cursor)
Client-->>Presenter: Response (channels, next_cursor)
end
alt Rate limited error
Presenter->>Presenter: Log warning, return partial channels
else Other error
Presenter->>Presenter: Throw error with details
end
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/webapp/app/presenters/v3/NewAlertChannelPresenter.server.ts (1)
107-147: Solid implementation of rate limit handling for Slack API callsThis approach effectively handles rate limit errors by returning partial results instead of failing completely. The code is well-commented and explains the issue, workaround, and future plans clearly.
Consider adding the metric tracking mentioned in your comment on line 138 to monitor occurrences of this rate limiting issue, if not implemented elsewhere:
// proper searching. Until then, we track occurrences of this issue using a metric. + // TODO: Add metric tracking for rate limiting occurrences return channels;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/webapp/app/presenters/v3/NewAlertChannelPresenter.server.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: typecheck / typecheck
🔇 Additional comments (2)
apps/webapp/app/presenters/v3/NewAlertChannelPresenter.server.ts (2)
110-116: Good optimization with the maximum page sizeUsing the maximum allowed page size of 999 is an excellent approach to reduce the number of API requests needed and mitigate rate limiting issues.
142-147: Well-structured error handlingThe error handling is comprehensive, with appropriate error type checking and informative error messages.
myftija
force-pushed
the
fix-slack-channel-fetching
branch
from
9e406e6 to
0639d8b
Compare
This PR creates a workaround to handle rate limiting issues from Slack when listing all channels of a workspace.
This is a workaround to the current way we handle Slack channels for creating alerts. For workspaces with a lot of channels (>10000) we might hit the rate limit for this slack endpoint, as multiple requests are needed to fetch all channels. We use the largest allowed page size of 999 to reduce the chance of hitting the rate limit.
This is mainly due to workspaces with a large number of archived channels, which this slack endpoint unfortunately filters out only after fetching the page of channels without applying any filters first:
https://api.slack.com/methods/conversations.list#markdown
We expect most workspaces not to run into this issue, but if they do, we at least return some channels.
In the future, we might revisit the way we handle Slack channels and cache them on our side to support
proper searching. Until then, we track occurrences of this issue using a metric.
Summary by CodeRabbit
Bug Fixes
Performance