fix(slack): improve rate limit error handling and logging #35588
Conversation
Addresses the issue where Slack API rate limit errors were being swallowed or masked by generic error messages. This change improves observability and reliability of Slack integrations. Changes: - Increased retry count from 2 to 4 to provide more buffer before failure - Added explicit error handling for HTTP 429 rate limit responses - Enhanced logging with Retry-After header and retry handler status - Added client initialization logging for debugging - Fixed edge case where ex.response could be a string instead of object This will make rate limit errors clearly visible in monitoring systems like Datadog, making it easier to diagnose and respond to Slack API rate limiting issues.
![korbit-ai[bot]](https://avatars.githubusercontent.com/in/322216?s=60&v=4){kind=small}
There was a problem hiding this comment.
I've completed my review and didn't find any issues.
Files scanned
| File Path | Reviewed |
|---|---|
| superset/utils/slack.py | ✅ |
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.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #35588 +/- ##
===========================================
+ Coverage 0 71.85% +71.85%
===========================================
Files 0 589 +589
Lines 0 43608 +43608
Branches 0 4718 +4718
===========================================
+ Hits 0 31335 +31335
- Misses 0 11035 +11035
- Partials 0 1238 +1238
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:
|
dosubot
bot
added
the
logging
| ) | ||
| else: | ||
| logger.error("Slack API error: %s", ex) | ||
| raise |
There was a problem hiding this comment.
can we raise a Superset error here instead of the Slack one?
| raise | ||
| except Exception: | ||
| logger.exception("Unexpected error fetching Slack channels") | ||
| raise |
There was a problem hiding this comment.
I'd suggest the same here- raise a Superset error.
|
looks like there were already updates to the logging there. abandoing this PR for now |
2 participants
SUMMARY
Fixes the issue where Slack API rate limit errors (HTTP 429) were being swallowed or masked by generic error messages in the
slack-sdklibrary's retry handler. This change improves observability and reliability of Slack integrations by adding explicit error detection and detailed logging.Root Cause: The
RateLimitErrorRetryHandlerin the slack-sdk library can re-raise errors without proper context when certain conditions are met. When these errors bubble up to generic exception handlers, they get logged as "Failed to send a request to Slack API server" instead of clearly indicating a rate limit issue.Changes:
get_channels()ex.responsecould be a string instead of response object usinghasattr()checkImpact: Rate limit errors will now be clearly visible in monitoring systems like Datadog with actionable information (Retry-After values, retry counts), making it much easier to diagnose and respond to Slack API rate limiting issues.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A - Backend logging improvement
TESTING INSTRUCTIONS
pytest tests/unit_tests/utils/slack_test.pypre-commit run --files superset/utils/slack.pyADDITIONAL INFORMATION