Add Spotify 429 (too many requests) API error handling #4370
Conversation
There was a problem hiding this comment.
This is definitely a step in the right direction! Handling the error and retrying seems much better than crashing when we exceed the rate-limit.
Can we remove the other time.sleep call here with this in place?
Line 582 in c25e7ad
As I mentioned in #4369 (comment), however, I think it's possible to do even better (perhaps in a subsequent PR?). The rough idea would be to explicitly account for Spotify's 30-second rolling window on the client side by counting up the number of requests we've made recently and delaying only ones that exceed the limit. But like I said above, I this reactive-rather-than-proactive approach in this PR is a great 80% solution.
beetsplug/spotify.py
Outdated
| elif response.status_code == 429: | ||
| seconds = response.headers['Retry-After'] | ||
| time.sleep(int(seconds)) | ||
| self._log.info('Too many API requests. Retrying after {} \ |
There was a problem hiding this comment.
I think log messages about retrying, etc., should be debug-level rather than info-level messages: they are not "actionable" from the user's perspective and are kind of an internal implementation detail.
There was a problem hiding this comment.
My thought was to keep the user informed about the delay. It will also be nice to get those reports if users see those messages so that we can adjust the timeout. In any case, I have changed it to debug.
There was a problem hiding this comment.
It looks like this changed back to info?
I think we can, but it may generate frequent 429 errors. We can reduce the |
|
Looking better and better! But I guess I would like to ask a question about this:
Have you observed that this happens? It might be a really useful experiment to run: what does the plugin behave like (how many errors does it see) if there is no extra I ask for two reasons: first, intuitively, it seems like the 429 error's "please back off by N seconds" response could, in fact, work to avoid repeating the error too many times. That is, we'd inevitably get one such error after doing many tracks, but then things would settle down after the error tells us how long to sleep for. Second, reading that aforelinked Spotify API documentation page made it sound like following the 429 errors was the recommended way to do rate limiting. It doesn't even say what the rate limits are, which would be critical to avoiding 429 errors! So maybe this the technique we're supposed to use? |
|
I did receive 429 with the other (now closed) PR (#4102 ). While Spotify does not explicitly specify the limits, I read that it should be able to handle 2000 calls per hour (so slightly less than 1 call per second). However, as you suggested, using the |
|
Great!! Just out of curiosity, have you gotten a chance to try it out on a large-ish number of files to see how well/poorly it performs? |
|
So far, I have not hit 429 (but that may be because I don't have my entire library on my test setup). Once we have this merged and hopefully released, I can test it out extensively. I have other features in mind for Spotify and Plex integration, so I will be sure to fix any issues that may come up :) |
beetsplug/spotify.py
Outdated
| elif response.status_code == 429: | ||
| seconds = response.headers['Retry-After'] | ||
| time.sleep(int(seconds)) | ||
| self._log.info('Too many API requests. Retrying after {} \ |
There was a problem hiding this comment.
It looks like this changed back to info?
docs/changelog.rst
Outdated
| * Added Spotify 429 (too many requests) API error handling | ||
| :bug:`4370` |
There was a problem hiding this comment.
Maybe we should also say the bottom line for what the user can expect here: namely, we now respect the Spotify API's rate limiting, which avoids crashing when the API reports code 429 (too many requests).
There was a problem hiding this comment.
I changed it back to info so that the users are not guessing. See my argument here.
There was a problem hiding this comment.
Changed logging back to debug.
There was a problem hiding this comment.
Thanks!!! I do think this is the right thing. The philosophy is that now, with this new strategy, 429 responses are part of the normal operation of the command—it's how we're enforcing the rate limit logic. So there's nothing actionable for the user to interpret when these errors occur.
Description
Added ability to handle API error (too many requests).
Fixes #4369
To Do
docs/to describe it.)docs/changelog.rstnear the top of the document.)