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

Fix AzDo Throttling Timeouts #1715

Closed

Conversation

oliverisaac
Copy link
Contributor

Getting errors like the one below due to AzureDevops throttling of api requests:

Error: making pull request API call to Azure DevOps: Get "https://dev.azure.com/example/terraform/_apis/git/repositories/example-repo/pullrequests/1234?api-version=5.1-preview.1&includeWorkItemRefs=true": net/http: request canceled (Client.Timeout exceeded while awaiting headers)

Took two strategies to help with this:

  1. Add a flag to customize the client timeout
  2. Add a roundtripper to the azdo client which will honor the Retry-After header sent back from AzDo. It will then sleep that many seconds before it will do a second API request

Both of the options I added default to the present state of things (10 seconds timeout, no retry-after honoring) and will only affect end-users if they explicitly enable those features.

@oliverisaac oliverisaac requested a review from a team as a code owner July 23, 2021 20:32
…y-After header. Also added flag to increase timeouts for azdo client. Need both to deal with Azdo throttling
@LanceSandino
Copy link

bump

Copy link
Contributor

@nishkrishnan nishkrishnan left a comment

Choose a reason for hiding this comment

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

Some docs on these flags would be great ! Thanks for contributing!

Comment on lines +41 to +42
ADHonorRetryAfter = "azuredevops-honor-retry-after"
ADTimeoutSeconds = "azuredevops-timeout-seconds"
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be nice to put the word "client" in here to be more specific.

Comment on lines +50 to +56
retryAfterVal := resp.Header.Get("retry-after")
if retryAfterVal != "" {
retryAfterSec, err := strconv.Atoi(retryAfterVal)
if err == nil {
rt.RetryAfter = time.Duration(retryAfterSec) * time.Second
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add exponential backoff and jitter here? I know we do this in a lot of other spots but it's not great practice. Risk of thundering herd causing a chain reaction.

Also probably good to call out the risks of using this flag in the docs.

@nishkrishnan nishkrishnan added the waiting-on-response Waiting for a response from the user label Aug 30, 2021
@github-actions
Copy link

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

@github-actions github-actions bot added the Stale label Sep 30, 2021
@github-actions github-actions bot closed this Oct 6, 2021
@LanceSandino
Copy link

bump, can we get this back open? @nishkrishnan

@jamengual jamengual reopened this Mar 8, 2022
@jamengual
Copy link
Contributor

jamengual commented Mar 8, 2022

@LanceSandino are you going to fix the conflicts?

@LanceSandino
Copy link

Ah... sorry after speaking to my colleague this is no longer needed. This can be closed again :)

@github-actions github-actions bot removed the Stale label Mar 9, 2022
@github-actions
Copy link

github-actions bot commented Apr 8, 2022

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

@github-actions github-actions bot added the Stale label Apr 8, 2022
@github-actions github-actions bot closed this Apr 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting-on-response Waiting for a response from the user
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants