slackapi / java-slack-sdk

Slack Developer Kit (including Bolt for Java) for any JVM language
https://slack.dev/java-slack-sdk/
MIT License
570 stars 209 forks source link

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

Closed gunrein closed 7 months ago

gunrein commented 7 months ago

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 [ ])

Requirements

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

seratch commented 7 months ago

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.

gunrein commented 7 months ago

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 commented 7 months ago

@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.

gunrein commented 7 months ago

@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.

codecov[bot] commented 7 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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

Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.