fix(alerts): implement Slack channel pagination to resolve timeout issues#35832
Conversation
dosubot
Bot
added
the
alert-reports
![korbit-ai[bot]](https://avatars.githubusercontent.com/in/322216?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review by Korbit AI
Korbit automatically attempts to detect when you fix issues in new commits.
| Category | Issue | Status |
|---|---|---|
 |
Missing limit validation constraints ▹ view | |
 |
Unbounded cursor cache growth ▹ view | |
|
Redundant string lowercasing in search loop ▹ view | |
|
Premature schema loading before search filtering ▹ view |
Files scanned
| File Path | Reviewed |
|---|---|
| superset/utils/slack.py | ✅ |
| superset/reports/schemas.py | ✅ |
| superset/reports/api.py | ✅ |
| superset-frontend/src/features/alerts/components/NotificationMethod.tsx | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Check out our docs on how you can make Korbit work best for you and your team.
Interaction Diagram by BitosequenceDiagram
participant User as User<br/>
participant Modal as AlertReportModal
participant NotifComp as NotificationMethod<br/>🔄 Updated | ●●● High
participant AsyncSel as AsyncSelect<br/>🔄 Updated | ●●● High
participant API as REST API<br/>🔄 Updated | ●●○ Medium
participant SlackUtil as get_channels_with_search<br/>🔄 Updated | ●●● High
participant SlackSDK as Slack SDK
User->>Modal: Open alert/report settings
Modal->>NotifComp: Render notification method selector
User->>NotifComp: Select SlackV2 method
NotifComp->>AsyncSel: Initialize with fetchSlackChannels callback
User->>AsyncSel: Search or scroll for channels
AsyncSel->>NotifComp: Call fetchSlackChannels(search, page, pageSize)
NotifComp->>API: GET /api/v1/report/slack_channels/?q=...
API->>SlackUtil: get_channels_with_search(search_string, cursor, limit)
SlackUtil->>SlackSDK: conversations_list(cursor, limit, types)
SlackSDK-->>SlackUtil: Return channels page + next_cursor
SlackUtil-->>API: {result, next_cursor, has_more}
API-->>NotifComp: Paginated channel list
NotifComp-->>AsyncSel: {data: channels, totalCount}
AsyncSel-->>User: Display channels with pagination
Critical path: User->AlertReportModal->NotificationMethod->AsyncSelect->REST API->get_channels_with_search->Slack SDK
|
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #7b7415
Actionable Suggestions - 2
-
superset-frontend/src/features/alerts/components/NotificationMethod.tsx - 1
- Missing method update on SlackV2 error fallback · Line 267-268
-
tests/unit_tests/utils/slack_test.py - 1
- Broken pagination for large limits in non-search case · Line 420-420
Additional Suggestions - 13
-
tests/unit_tests/utils/slack_test.py - 5
-
Magic number 50 used in comparison · Line 455-455Magic number `50` used in comparison. This occurs in multiple locations. Consider defining a constant like `MAX_PAGES = 50` for better maintainability.
Code suggestion
@@ -435,3 +435,4 @@ def test_streaming_search_max_pages_safety_limit(self, mocker): """Test that streaming search stops after 50 pages to prevent runaway requests.""" + MAX_PAGES = 50 -
Unused variable result assigned but never used · Line 610-610Variable `result` is assigned but never used in `test_cursor_format_with_special_characters`. Consider removing the assignment or using the variable in assertions.
Code suggestion
@@ -609,2 +609,1 @@ # Call with special cursor - result = get_channels_with_search(cursor=special_cursor, limit=100) + get_channels_with_search(cursor=special_cursor, limit=100) -
Missing trailing comma in dictionary literal · Line 43-43Dictionary literal is missing trailing comma after `"is_member": True`. This issue occurs in multiple dictionary literals throughout the file. Consider adding trailing commas for consistency.
Code suggestion
@@ -42,2 +42,2 @@ - "is_member": True, - } + "is_member": True, + },
-
Docstring missing ending period punctuation · Line 322-322Docstring `"Test pagination returns single page with cursor"` should end with a period. This issue occurs in multiple docstrings throughout the file.
Code suggestion
@@ -322,1 +322,1 @@ - """Test pagination returns single page with cursor""" + """Test pagination returns single page with cursor."""
-
Line exceeds maximum length of 88 characters · Line 436-436Line exceeds maximum length of 88 characters (89 characters). Consider breaking the docstring into multiple lines or shortening the text.
Code suggestion
@@ -436,1 +436,2 @@ - """Test that streaming search stops after 50 pages to prevent runaway requests""" + """Test that streaming search stops after 50 pages to prevent + runaway requests."""
-
-
superset/utils/slack.py - 5
-
Exception message formatting issues · Line 188-188Avoid f-string in exception and long messages. Assign f-string to variable first, then use in exception.
Code suggestion
@@ -187,1 +187,2 @@ - except (SlackClientError, SlackApiError) as ex: - raise SupersetException(f"Failed to list channels: {ex}") from ex + except (SlackClientError, SlackApiError) as ex: + error_msg = f"Failed to list channels: {ex}" + raise SupersetException(error_msg) from ex
-
Import Callable from collections.abc instead · Line 20-20Import `Callable` from `collections.abc` instead of `typing` for better compatibility with Python 3.9+.
Code suggestion
@@ -20,1 +20,1 @@ -from typing import Any, Callable, Optional +from typing import Any, Optional +from collections.abc import Callable
-
Use union syntax for cursor parameter · Line 95-95Use `str | None` syntax for type annotation instead of `Optional[str]`.
Code suggestion
@@ -95,1 +95,1 @@ - cursor: Optional[str] = None, + cursor: str | None = None,
-
Docstring format and imperative mood issues · Line 98-98Move docstring summary to first line and use imperative mood: start with 'Fetch' instead of 'Fetches'.
Code suggestion
@@ -97,2 +97,1 @@ -) -> dict[str, Any]: - """ - Fetches Slack channels with pagination and search support. +) -> dict[str, Any]: + """Fetch Slack channels with pagination and search support.
-
Use elif instead of else if · Line 163-163Use `elif` instead of `else` followed by `if` to reduce indentation level.
Code suggestion
@@ -162,4 +162,3 @@ - matches.append(channel) - else: - if ( - search_string.lower() in channel["name"].lower() + matches.append(channel) + elif ( + search_string.lower() in channel["name"].lower()
-
-
tests/integration_tests/reports/api_tests.py - 3
-
Missing return type annotation for test method · Line 2054-2054Test method `test_slack_channels_api_without_pagination` is missing return type annotation. Add `-> None` to follow type annotation standards. Multiple similar issues exist in other test methods.
Code suggestion
@@ -2054,1 +2054,1 @@ - def test_slack_channels_api_without_pagination(self, mock_get_channels): + def test_slack_channels_api_without_pagination(self, mock_get_channels) -> None:
-
Docstring formatting issues need correction · Line 2055-2055Docstring should be a one-line format and end with a period. Consider: `"Test /api/v1/report/slack_channels/ endpoint without pagination."`
Code suggestion
@@ -2055,3 +2055,1 @@ - """ - Test /api/v1/report/slack_channels/ endpoint without pagination - """ + """Test /api/v1/report/slack_channels/ endpoint without pagination."""
-
Line length exceeds maximum allowed characters · Line 2063-2063Lines 2063-2064 exceed 88 character limit. Consider breaking long dictionary entries across multiple lines for better readability.
Code suggestion
@@ -2062,4 +2062,6 @@ - "result": [ - {"id": "C001", "name": "general", "is_private": False, "is_member": True}, - {"id": "C002", "name": "random", "is_private": False, "is_member": True}, - ], + "result": [ + {"id": "C001", "name": "general", "is_private": False, + "is_member": True}, + {"id": "C002", "name": "random", "is_private": False, + "is_member": True}, + ],
-
Review Details
-
Files reviewed - 7 · Commit Range:
2738d23..2738d23- superset-frontend/src/features/alerts/components/NotificationMethod.test.tsx
- superset-frontend/src/features/alerts/components/NotificationMethod.tsx
- superset/reports/api.py
- superset/reports/schemas.py
- superset/utils/slack.py
- tests/integration_tests/reports/api_tests.py
- tests/unit_tests/utils/slack_test.py
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Default Agent You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #35832 +/- ##
===========================================
+ Coverage 0 68.21% +68.21%
===========================================
Files 0 629 +629
Lines 0 46314 +46314
Branches 0 5030 +5030
===========================================
+ Hits 0 31591 +31591
- Misses 0 13472 +13472
- Partials 0 1251 +1251
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
gabotorresruiz
force-pushed
the
fix/alerts-reports-unable-to-edit-slack-recipients
branch
9 times, most recently
from
c2e80c3 to
84b3930
Compare
gabotorresruiz
force-pushed
the
fix/alerts-reports-unable-to-edit-slack-recipients
branch
2 times, most recently
from
982c807 to
3668f8e
Compare
gabotorresruiz
force-pushed
the
fix/alerts-reports-unable-to-edit-slack-recipients
branch
from
d30c7c6 to
3f85dcf
Compare
gabotorresruiz
force-pushed
the
fix/alerts-reports-unable-to-edit-slack-recipients
branch
3 times, most recently
from
d2b0ae3 to
82ba234
Compare
gabotorresruiz
force-pushed
the
fix/alerts-reports-unable-to-edit-slack-recipients
branch
2 times, most recently
from
8a00bbd to
bed6355
Compare
|
hey @gabotorresruiz thanks for working on this! I left a couple of comments. Currently, this is the flow for an API request to get Slack channels:
This would cause issues mainly because Slack has rate limit on this endpoint, so for instances with a considerable amount of channels, it could take several API calls, hitting rate limits and eventually timeouts. for smaller instances, this works pretty well and keeps the full list of channels cached. The benefit of your PR, is that we're not trying to fetch the full list. Instead, we're requesting either filtered/unfiltered values via the API, and would paginate only up until the With that in mind, I wonder if we might want to explore other alternatives. Do we know if this is still an issue after this PR? #35622 One potential alternative would be to have a toggle in the Alert/Report modal UI to choose between using the Slack APIs to search for the name and get an ID (which is subject to the issues discussed here) or manually enter a channel ID (we could point the UI to docs showing how to get that info from Slack via the UI). It's not as intuitive as search, but I think until Slack supports filtering on the endpoint we might not be able to avoid rate limits to larger enterprises. |
|
Hey @Vitor-Avila, thanks for the thorough review! Let me address your concerns: Rate Limits ConcernYour concern about "more API calls from typing" is valid but largely mitigated by the caching architecture: With cache warm (normal operation):
API calls only happen:
So the typing scenario you described (each keystroke = new API call) doesn't happen when cache is available. The cache stores all channels and filtering happens entirely in-memory How Search WorksSince Slack's Cache path (normal operation):
Search features:
Cache ArchitectureThe implementation uses a background Celery task to warm the cache:
Non-Blocking Cache WarmupThe cache warmup is designed to never block the UI or degrade user experience: Asynchronous execution:
User experience flow:
Cold cache behavior:
No hanging or timeouts:
How This PR Relates to #35622:They're complementary, not alternatives:
#35622 helps us recover from rate limits; this PR helps avoid them entirely |
gabotorresruiz
force-pushed
the
fix/alerts-reports-unable-to-edit-slack-recipients
branch
from
bed6355 to
dff40c6
Compare
gabotorresruiz
force-pushed
the
fix/alerts-reports-unable-to-edit-slack-recipients
branch
from
dff40c6 to
4f8a504
Compare
| SLACK_CHANNELS_CONTINUATION_CURSOR_KEY = ( | ||
| f"{SLACK_CHANNELS_CACHE_KEY}_continuation_cursor" | ||
| ) |
There was a problem hiding this comment.
Is this still being used? I don't see a set event for this key anymore
| next_cursor: Optional[str] | ||
| if not has_more and continuation_cursor: | ||
| # Return special cursor that signals transition to API | ||
| next_cursor = "api:continue" |
There was a problem hiding this comment.
Same here: is this still being used? I think this was related with the SLACK_CHANNELS_CONTINUATION_CURSOR_KEY usage.
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
| CACHE_WARMUP_TIME_LIMIT = 300 # 5 minutes |
There was a problem hiding this comment.
One thing I thought about is if we could have this constant to receive a value from a constant from config.py? This way users could set a custom value in there that would get assigned here (considering that this local constant works).
Do you know if that would work? I'm just asking because I imagine there are some larger organizations that might need more than 5 minutes. Having the ability to customize from there would be better.
| if force: | ||
| cache_manager.cache.delete(SLACK_CHANNELS_CACHE_KEY) | ||
| cache_manager.cache.delete( | ||
| SLACK_CHANNELS_CONTINUATION_CURSOR_KEY | ||
| ) | ||
| logger.info("Slack channels cache cleared due to force=True") | ||
|
|
||
| # Trigger async cache warmup if caching is enabled | ||
| if current_app.config.get("SLACK_ENABLE_CACHING", True): | ||
| cache_channels.delay() | ||
| logger.info("Triggered async cache warmup task") | ||
|
|
||
| channels_data = get_channels_with_search( | ||
| search_string=search_string, | ||
| types=types, | ||
| exact_match=exact_match, | ||
| force=force, | ||
| cursor=cursor, | ||
| limit=limit, | ||
| ) |
There was a problem hiding this comment.
This might be an issue. When force is True, here we're calling cache_channels.delay() to warm up the cache + also calling get_channels_with_search(). This will hit the Slack APIs in parallel (one thread for async cache warm up and another for the frontend search) which will likely cause a lot more rate limits.
There was a problem hiding this comment.
Would it make sense to have the force button to show a toast that the channel list is being updated and prevent the user from searching until the cache is warmed up? @eschutho what do you think?
|
thanks @gabotorresruiz, left a few more comments with the latest changes 🙏 one important detail is that currently we support caching the list of channels without celery workers. This PR on the other hand limits caching to only instances that have Celery workers configured -- @eschutho do you think this a concern? I would imagine all large organizations that would really need several calls to the Slack APIs would have celery configured, but wondering if we want to keep this functionality. |
|
+1, this is affecting our instance and forcing us to use v1 instead. |
5 participants
SUMMARY
Problem
Users were unable to edit Slack recipients in Alerts & Reports when their Slack workspace had a large number of channels. The dropdown would fail to load, showing a timeout error after 30+ seconds, making the feature completely unusable for large organizations.
Root Cause
The previous implementation attempted to load all Slack channels at once without proper pagination:
Solution
This PR implements proper pagination with client-side search filtering:
Backend:
get_channels_with_search()function with two optimized paths:get_channels()function and migratedcache_channelsCelery task to use new paginationFrontend:
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before
After
TESTING INSTRUCTIONS
Prerequisites
superset/config.py:SLACK_API_TOKENinsuperset/config.pyADDITIONAL INFORMATION