Autocomplete: Implement client-side timeouts #1355
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
eseliger
approved these changes
Oct 10, 2023
| const abortController = signal ? forkSignal(signal) : new AbortController() | ||
| return Promise.race([ | ||
| complete(params, onPartialResponse, abortController.signal), | ||
| createTimeout(params.timeoutMs).finally(() => { |
There was a problem hiding this comment.
can we make sure that these are not logged as errors in sentry?
There was a problem hiding this comment.
Why would we not want to log them in Sentry? The user is not getting a completion, seems like something we should have our eyes on to me?
There was a problem hiding this comment.
If it's a quota thing I think we should at least log this in our analytics somewhere.
There was a problem hiding this comment.
If we expect this to be low volume that’s fine 🙂
There was a problem hiding this comment.
Haha yeah if this happens often we have to revisit this approach anyways 🫡
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds client side timeouts as discussed in this Slack thread.
The reason for this are the following:
Our current server-side backend does request retrying for a fixed number of times. For Autocomplete however, latency is of upmost importance so we don't really want to retry requests for a long time. In some cases, we see requests being retried for up to 30 minutes which is just putting additional pressure on the downstream processes without providing a great UX. Since timeouts are highly client specific, we decided that adding those to the client is the best approach for now. All of our middleware respect aborted requests so all we need to do is to start a race between a timeout and abort the request if that is hit.
Note: This PR is best reviewed with whitespace changes ignored.
Test plan