Skip to content

Commit

Permalink
Rate limits are now handled by the library
Browse files Browse the repository at this point in the history
  • Loading branch information
@ErikKalkoken
ErikKalkoken committed Apr 4, 2022
1 parent fed0df5 commit 84c675c
Show file tree
Hide file tree
Showing 4 changed files with 2 additions and 7 deletions.
3 changes: 2 additions & 1 deletion CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,13 @@ and this project adheres to [Semantic Versioning](http://semver.org/).

## [Unreleased] - yyyy-mm-dd

## [1.3.2] - tbd
## [1.3.2] - 2022-04-04

### Changed

- Upgraded to new Slack library (slack_sdk)
- Added official support for Python 3.10
- Rate limits are now handled by the slack_sdk library (#7)

## [1.3.1] - 2021-02-21

Expand Down
4 changes: 0 additions & 4 deletions slackchannel2pdf/slack_service.py
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
"""Handles all requests to Slack"""

import logging
from time import sleep

import slack_sdk
from babel.numbers import format_decimal
Expand Down Expand Up @@ -252,7 +251,6 @@ def _fetch_pages(
):
page += 1
logger.info("%s - page %s", output_str, page)
sleep(1) # need to wait 1 sec before next call due to rate limits
page_args = {
**base_args,
**{
Expand Down Expand Up @@ -307,8 +305,6 @@ def fetch_bot_names_for_messages(self, messages: list, threads: list) -> dict:
response = self._client.bots_info(bot=bot_id)
if response["ok"]:
bot_names[bot_id] = transform_encoding(response["bot"]["name"])
sleep(1) # need to wait 1 sec before next call due to rate limits

return bot_names

@staticmethod
Expand Down
1 change: 0 additions & 1 deletion tests/test_channel_exporter.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ def test_run_with_error(self):


@patch("slackchannel2pdf.slack_service.slack_sdk")
@patch("slackchannel2pdf.slack_service.sleep", lambda x: None)
class TestSlackChannelExporter(NoSocketsTestCase):
"""New test approach with API mocking, that allows full testing of the exporter"""

Expand Down
1 change: 0 additions & 1 deletion tests/test_slack_services.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ def test_3(self):


@patch(MODULE_NAME + ".slack_sdk")
@patch(MODULE_NAME + ".sleep", lambda x: None)
class TestSlackService(NoSocketsTestCase):
def test_should_return_all_user_names_1(self, mock_slack):
# given
Expand Down

4 comments on commit 84c675c

@hheimbuerger
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ErikKalkoken I cannot confirm that the rate limits are handled by the library, or at least not all of them.

I'm experiencing rate limit exceptions in conversations_replies() (and possibly elsewhere) in both 1.3.1 and 1.4.0. I'll submit my own proof-of-concept rate limiting solution as a PR soon.

@hheimbuerger
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not familiar with the Slack API or any of its clients, but a cursory search through the documentation might imply that this maybe needs to be explicitly enabled using the RateLimitErrorRetryHandler?

I couldn't find any reference to that in the slackchannel2pdf source, but I really only went for a simple grep, so maybe I'm totally off here.

@ErikKalkoken
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your feedback. There are two different concepts to distinguish here:

a) Causing a rate limit
b) Reacting to a rate limit error reporting by the API

As I understand the newer version of the slack library has a "smart rate limiter", which should prevent a) by automatically limiting how many request are send to the API. This is explained here: slackapi/python-slack-sdk#1101

I also made some local tests and was not able to cause any rate limits despite downloading thousands of messages with the current version. But yeah, it may not be perfect yet. I could look into implementing b) as a mitigation.

@hheimbuerger
Copy link

@hheimbuerger hheimbuerger commented on 84c675c May 3, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ErikKalkoken Thank you, that is helpful context, I indeed did not mentally differentiate between those two concepts.

I'm wondering if I'm doing something wrong, because I don't feel like the "smart rate limiter" is in effect for me. I'm just running 1.4.0 against a single channel, and I'm hitting the rate limits pretty reliably:
It makes about five requests for metadata, then 18 pages of messages (3,476), then the threads and after a few hundreds of them (<1min), I get a rate limiting exception. This isn't even one of my larger channels, but we've always been using threads a lot.

Is is possible that you're using a different token, which allows a higher frequency of requests? I created an app just to use slackchannel2pdf and basically googled my way there, with no understanding what I'm doing. So I almost certainly did it wrong. (Yet the token produces correct data.)

Or maybe you're just not using threads much and this is a threading problem? I haven't really surveyed multiple channels and compared notes, because I unfortunately need to get the export project done, and my (soon-to-be-submitted) fixes are eliminating the issue for me completely.

Please sign in to comment.