Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Correct the rate limit tier setting for users.profile.set API calls #1268

Conversation

gunrein
Copy link
Contributor

@gunrein gunrein commented Jan 22, 2024

Thanks for providing this client library!

We have recently had some trouble with rate limiting on the users.profile.set method. This PR makes two changes.

  1. It fixes an apparent bug in the async methods client where the queue depth for a method is reset to 0 when AsyncRateLimitExecutor.execute is called. There is a separate thread that will update the queue depth metric, but ideally it is never reset to 0 incorrectly. When the metric is reset it can lead to no (or incorrect) backoff being calculated in WaitTimeCalculator.calculateWaitTime.
  2. It changes the rate limit tier for users.profile.set to more closely match the documentation. https://api.slack.com/apis/rate-limits#profile_updates indicates the rate limit is 30 requests per minute so Tier2 seems more appropriate / safer.

Category (place an x in each of the [ ])

  • bolt (Bolt for Java)
  • bolt-{sub modules} (Bolt for Java - optional modules)
  • slack-api-client (Slack API Clients)
  • slack-api-model (Slack API Data Models)
  • slack-api-*-kotlin-extension (Kotlin Extensions for Slack API Clients)
  • slack-app-backend (The primitive layer of Bolt for Java)

Requirements

Please read the Contributing guidelines and Code of Conduct before creating this issue or pull request. By submitting, you agree to those rules.

This happens when `AsyncRateLimitExecutor.execute` is called. There is a separate thread that will update the queue depth metric, but ideally it is never reset to 0 incorrectly. When the metric is reset it can lead to no (or incorrect) backoff being calculated in `WaitTimeCalculator.calculateWaitTime`.
…atch the documentation

https://api.slack.com/apis/rate-limits#profile_updates indicates the rate limit is 30 requests per minute so `Tier2` seems more appropriate / safer.
@seratch seratch added enhancement M-T: A feature request for new functionality project:slack-api-client project:slack-api-client labels Jan 22, 2024
@seratch
Copy link
Contributor

seratch commented Jan 22, 2024

Hi @gunrein, thank you so much for taking the time to send this PR. It appears that the rate limit for users.profile.set may have loosened from Tier 4 to Tier 2 has been changed from Tier 4 to Tier 3 at some point (or it might be wrong on this SDK side from the beginning...), therefore the outdated configuration in the code should be corrected. However, your change to the internal method for calculating queue size isn't necessary at this time. Would you mind reverting that change? Indeed, the method signature could be somewhat confusing, but it does work without any issues.

@seratch seratch self-assigned this Jan 22, 2024
@gunrein
Copy link
Contributor Author

gunrein commented Jan 22, 2024

Great, thanks @seratch.

I'll back out the change to the internal method. One question on that, though (feel free to tell me it isn't worth explaining; no worries)... the original code was

        if (this.isStatsEnabled()) {
            CopyOnWriteArrayList<String> messageIds = getOrCreateMessageIds(executorName, teamId, methodName);
            Integer totalSize = messageIds.size();
            RateLimitQueue<SUPPLIER, MSG> queue = getRateLimitQueue(executorName, teamId);
            if (queue != null) {
                totalSize += queue.getCurrentActiveQueueSize(methodName);
            }
            getOrCreateTeamLiveStats(executorName, teamId).getCurrentQueueSize().put(methodName, totalSize);
            getOrCreateTeamLiveStats(executorName, teamId).getCurrentQueueSize().put(methodName, size);

Doesn't the second call getOrCreateTeamLiveStats(executorName, teamId).getCurrentQueueSize().put(methodName, size); reset the value to size (and make the calculation of totalSize not worth the cycles? In the case of the call from execute, size is always 0.

@seratch
Copy link
Contributor

seratch commented Jan 22, 2024

@gunrein Ah, you're right on the point. It seems the code should be improved, but this SDK has similar code in other modules too, so I will do more investigation on it later on. Thus, could you make this PR as simple as possible for now? Thank you so much for pointing the issue out.

@seratch seratch added this to the 1.37.1 milestone Jan 22, 2024
@seratch seratch changed the title Async API methods client queue depth metric fix and a tier change for users.profile.set Correct the rate limit tier setting for users.profile.set API calls Jan 22, 2024
@gunrein
Copy link
Contributor Author

gunrein commented Jan 22, 2024

@seratch - of course. The tier is now 3 and the other changes are reverted from the branch.

Thanks for taking a look at the issue.

Copy link
Contributor

@seratch seratch left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

codecov bot commented Jan 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (8e94171) 74.28% compared to head (94b3f67) 74.33%.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #1268      +/-   ##
============================================
+ Coverage     74.28%   74.33%   +0.04%     
- Complexity     4122     4124       +2     
============================================
  Files           443      443              
  Lines         13092    13092              
  Branches       1324     1324              
============================================
+ Hits           9726     9732       +6     
+ Misses         2592     2588       -4     
+ Partials        774      772       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@seratch seratch merged commit 44676f5 into slackapi:main Jan 22, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla:signed enhancement M-T: A feature request for new functionality project:slack-api-client project:slack-api-client
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants