Add retry logic to client's Validate method #150

GrantSheehan · 2018-04-07T19:52:43Z

In order to get around Datadog's API throwing 503 errors with some regularity, adds retry logic for retryable errors.

Background:
I use the Datadog Terraform provider and it's written in a way that most of the failed requests will be retried without much issue, but refreshing the credentials on the provider itself will run the Validate() method, which is currently not able to be retried. As a result, a respectable percentage of our CI/CD builds end up failing due to a returned 503. This seemed like the most pragmatic way to resolve some of those errors (as Datadog's support has been somewhat less than helpful in resolving the cause of the 503's themselves, cough). If you don't like the way it's implemented here, just let me know what changes you'd like made I'll update it.

Thanks!

In order to get around Datadog's API throwing 503 errors with some regularity, adds retry logic for retryable errors.

ojongerius

Out of general curiosity, have you looked at https://github.com/hashicorp/go-retryablehttp ? I haven't used it but it looks pretty clean and might fit the use case here.

ojongerius · 2018-04-09T23:52:59Z

client.go


-	defer resp.Body.Close()


Why not defer closing resp.Body?

I was getting errors in unmarshaling JSON during testing, I think it was because I had it in the wrong scope. I've added it back.

GrantSheehan · 2018-04-10T01:28:16Z

I used the expontential backoff retrier package being used in request.go and mirrored more or less what it was doing.

ojongerius · 2018-04-10T01:36:11Z

Fair enough, that is indeed what we used before. Just stumbled upon go-retryablehttp and thought it looked neat.

GrantSheehan · 2018-04-10T01:52:46Z

Yeah, for sure, looks like it could make the code significantly easier to reason about. I can take a crack at migrating to it after this is merged, if you'd like.

ojongerius · 2018-04-10T01:52:54Z

Ok this is entertaining; I wrote the initial validate() method, and should have used doRequestWithRetries instead of doing the request manually. Thank you for wanting to fix the bug I created! 😊

Would you mind refactoring this to use doRequestWithRetries instead of copying the retry logic, or do you see a reason why that would not be desirable?

Re: using go-retryablehttp it could look cleaner, up to you if you want to take that on. Let's see if we can remove the deduplication first.

GrantSheehan · 2018-04-10T19:07:08Z

I updated my branch to use the doRequestWithRetries method instead.

GrantSheehan · 2018-04-12T17:05:34Z

@ojongerius Is there any additional testing you'd like me to do here? Am I missing anything?

ojongerius · 2018-04-17T08:25:40Z

Thanks @GrantSheehan was out due to illness. I'll have a gander tomorrow morning (NZST, UTC+12)!

GrantSheehan · 2018-04-17T16:16:59Z

Sorry to be a pain, this has been a thorn in our side for a while now. Having finally nailed down a root cause and a fix, I'll admit to being a bit eager to get a fix merged.

Glad you're feeling better!

ojongerius

Less code, I love it!

ojongerius · 2018-04-17T23:13:26Z

client.go


-	var resp *http.Response
-	resp, err = client.HttpClient.Do(req)
+	resp, err = client.doRequestWithRetries(req, maxTime)
 	if err != nil {
 		return false, err
 	}



Still needs a defer to close the response.

ojongerius · 2018-04-17T23:13:53Z

client.go

-		}
-		return false, fmt.Errorf("API error %s: %s", resp.Status, body)
-	}
-
 	body, err := ioutil.ReadAll(resp.Body)


Please check the value of err

ojongerius · 2018-04-17T23:20:30Z

client.go

-		}
-		return false, fmt.Errorf("API error %s: %s", resp.Status, body)
-	}
-
 	body, err := ioutil.ReadAll(resp.Body)
 	err = json.Unmarshal(body, &out)


If you want extra brownie points, improving my code, this could be something like:

if err = json.Unmarshal(body, &out); err != nil { return false, err }

ojongerius · 2018-04-17T23:21:21Z

Almost there! 🥇

ojongerius · 2018-04-18T03:08:41Z

client.go

-			return false, err
-		}
-		return false, fmt.Errorf("API error %s: %s", resp.Status, body)
+	if body, err := ioutil.ReadAll(resp.Body); err != nil {


I don't think this will work, as we'll need body later on.

Forgive my golang ignorance, how does doing it in the if change the logic in a way that would effect the content of body here?

To answer my own question, it seems to be the := inline declaration/assignment operator. TIL.

ojongerius · 2018-04-18T03:09:29Z

client.go

-	body, err := ioutil.ReadAll(resp.Body)
-	err = json.Unmarshal(body, &out)
-	if err != nil {
+	if err = json.Unmarshal(body, &out); err != nil {


ojongerius · 2018-04-18T03:10:51Z

client.go

 		return false, err
 	}

+	defer resp.Body.Close()


Please move the defer up to just after checking if doRequestWithRetries return an error, I think below line 82.

ojongerius

See my last comments, very close!

GrantSheehan · 2018-04-18T03:16:52Z

Yep, no problem. I'm still trying to grok some of golang's idioms/syntax, be as nit-picky as you want to be.

ojongerius

Just one error check and we should be good!

ojongerius · 2018-04-18T03:31:14Z

client.go

-	if bodyreader != nil {
-		req.Header.Add("Content-Type", "application/json")
-	}
+	req, err := http.NewRequest("GET", uri, nil)


Still needs checking if we got an error here.

@GrantSheehan btw, you were absolutely right about inline error checking. I stand corrected!

ojongerius

🎉

ojongerius · 2018-04-18T03:41:27Z

Nice work @GrantSheehan!

Can you confirm you've verified your changes work for you locally? Is there anything we need to check or can improve before we merge this?

GrantSheehan · 2018-04-18T18:08:42Z

I've been testing by vendoring and installing my forked repo/branch when compiling the Datadog Terraform provider and have had no issues. If there's anything you'd like me to do in addition to that, let me know.

ojongerius · 2018-04-18T21:24:21Z

Thanks @GrantSheehan! I'll cut a release for you so you can pull those changes into the Terraform provider.

ojongerius · 2018-04-18T22:24:54Z

I've cut release 2.8.4.

GrantSheehan · 2018-04-20T14:58:10Z

Thank you!

GrantSheehan force-pushed the auth_retry branch 2 times, most recently from 5132778 to 9fa6542 Compare April 7, 2018 20:10

Add retry logic to client's Validate method

cfd7c9c

In order to get around Datadog's API throwing 503 errors with some regularity, adds retry logic for retryable errors.

GrantSheehan force-pushed the auth_retry branch from 9fa6542 to cfd7c9c Compare April 7, 2018 20:15

ojongerius requested changes Apr 9, 2018

View reviewed changes

Add defer resp.Body.Close() back to Validate

3a3f08e

Use doRequestWithRetries for Validate method

bb258a6

Grant Sheehan added 2 commits April 10, 2018 19:08

Remove unneeded packages in client.go

fb62889

Still more unneeded deps

f4ac3fe

ojongerius requested changes Apr 17, 2018

View reviewed changes

Close body on Validate method, inline error checking

b004ee9

ojongerius reviewed Apr 18, 2018

View reviewed changes

ojongerius requested changes Apr 18, 2018

View reviewed changes

Syntax fixes

0211f83

Fix errant if's on inline declarations

ea2618f

ojongerius requested changes Apr 18, 2018

View reviewed changes

Check errors in Validate's NewRequest

963eee6

ojongerius approved these changes Apr 18, 2018

View reviewed changes

ojongerius merged commit ea1134d into zorkian:master Apr 18, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add retry logic to client's Validate method #150

Add retry logic to client's Validate method #150

GrantSheehan commented Apr 7, 2018 •

edited

Loading

ojongerius left a comment

ojongerius Apr 9, 2018

GrantSheehan Apr 10, 2018

GrantSheehan commented Apr 10, 2018

ojongerius commented Apr 10, 2018

GrantSheehan commented Apr 10, 2018

ojongerius commented Apr 10, 2018 •

edited

Loading

GrantSheehan commented Apr 10, 2018 •

edited

Loading

GrantSheehan commented Apr 12, 2018

ojongerius commented Apr 17, 2018

GrantSheehan commented Apr 17, 2018

ojongerius left a comment

ojongerius Apr 17, 2018

ojongerius Apr 17, 2018

ojongerius Apr 17, 2018

ojongerius commented Apr 17, 2018

ojongerius Apr 18, 2018

GrantSheehan Apr 18, 2018 •

edited

Loading

GrantSheehan Apr 18, 2018

ojongerius Apr 18, 2018

ojongerius Apr 18, 2018

ojongerius left a comment

GrantSheehan commented Apr 18, 2018

ojongerius left a comment

ojongerius Apr 18, 2018

ojongerius left a comment

ojongerius commented Apr 18, 2018 •

edited

Loading

GrantSheehan commented Apr 18, 2018 •

edited

Loading

ojongerius commented Apr 18, 2018 •

edited

Loading

ojongerius commented Apr 18, 2018

GrantSheehan commented Apr 20, 2018

Add retry logic to client's Validate method #150

Add retry logic to client's Validate method #150

Conversation

GrantSheehan commented Apr 7, 2018 • edited Loading

ojongerius left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GrantSheehan commented Apr 10, 2018

ojongerius commented Apr 10, 2018

GrantSheehan commented Apr 10, 2018

ojongerius commented Apr 10, 2018 • edited Loading

GrantSheehan commented Apr 10, 2018 • edited Loading

GrantSheehan commented Apr 12, 2018

ojongerius commented Apr 17, 2018

GrantSheehan commented Apr 17, 2018

ojongerius left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ojongerius commented Apr 17, 2018

Choose a reason for hiding this comment

GrantSheehan Apr 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ojongerius left a comment

Choose a reason for hiding this comment

GrantSheehan commented Apr 18, 2018

ojongerius left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ojongerius left a comment

Choose a reason for hiding this comment

ojongerius commented Apr 18, 2018 • edited Loading

GrantSheehan commented Apr 18, 2018 • edited Loading

ojongerius commented Apr 18, 2018 • edited Loading

ojongerius commented Apr 18, 2018

GrantSheehan commented Apr 20, 2018

GrantSheehan commented Apr 7, 2018 •

edited

Loading

ojongerius commented Apr 10, 2018 •

edited

Loading

GrantSheehan commented Apr 10, 2018 •

edited

Loading

GrantSheehan Apr 18, 2018 •

edited

Loading

ojongerius commented Apr 18, 2018 •

edited

Loading

GrantSheehan commented Apr 18, 2018 •

edited

Loading

ojongerius commented Apr 18, 2018 •

edited

Loading