fix: exclude PageSpeed Insights API errors from retries.#2010
fix: exclude PageSpeed Insights API errors from retries.#2010bshaffer merged 4 commits intogoogleapis:masterfrom
Conversation
google-cla
bot
added
the
cla: yes
|
@adamsilverstein thank you for this change! I am not sure the implications of the reason |
bshaffer
left a comment
bshaffer
left a comment
There was a problem hiding this comment.
Looking more into it, Lighthouse is our tool for analyzing / auditing the webpages. When this downstream library throws an error, the wrapping API throws a 500 and sets the reason as "lighthouseError".
Unfortunately, looking at possible error responses, some of them do correspond to 500 errors that should be retried. For example:
Lighthouse returned error: INTERNAL: APP::1: Lost browser connection.
In these cases, it would be best if the libraries retried this request. So is it better to retry all requests so that the ones that need retrying get it, or to not retry any requests, to prevent unnecessary retries? A third option would be to allow for certain "error" values to be configured.
I think the error above is not a common error, and results from a bug that shouldn't be retried anyway. So I approve this change.
|
@bshaffer Thank you for reviewing and approving. |
jdpedrie
changed the title
2 participants
When retries are enabled in the client, the Runner class by default retries requests when the API returns a
500error code. Unfortunately, the PageSpeed Insights API returns 5XX's for errors that should really be 4XX (specifically in Lighthouse for the errors ERRORED_DOCUMENT_REQUEST, FAILED_DOCUMENT_REQUEST). So 500 errors from this API shouldn't be retried, they will only increase load and fail again.These errors are non-retryable user defined errors: for example requesting data for a url that doesn't go anywhere. This PR changes the default client behavior to avoid retrying requests the the reason
lighthouseError, preventing unnecessary retries.Steps to reproduce
https://notareal-websiteno.com, and run the report.Additional Context
We recently added this change the the defaults used by Site Kit which relies on this library. Since we found this condition important enough to explicitly exclude, I thought it might be worth contributing upstream, although I'm not sure you want to add these types of API specific exceptions.