Implement & use RateLimitTransport

[radeksimko]

As described in attached code comments this PR implements best practices GitHub describes in their official documentation.

This is second of two PRs addressing #5
Closes #5

The upside is that with this patch user should never see rate-limit related errors anymore as we'll always retry accordingly and more importantly avoid hitting the limit.

Downsides include slowdown of CUD operations and generally slower UX when managing large number of resources. Admittedly if the previous/current experience was "rate limit errors" and now is slow, but successful apply/destroy I'd still call that a win. 🤷‍♂️ See some figures below.

We could in theory expose a flag to turn off the sleep logic and/or mutex, but I'm not too keen on doing that pre-emptively without further context, esp. because we're just implementing GitHub's own best practices.

100 repositories test

Current implementation

• creation: 16 seconds
• refresh: 4 seconds
• destroy: 9 seconds

Serializing requests via mutex

• creation: 147 seconds (9x slower)
• refresh: 25 seconds (6x slower)
• destroy: 48 seconds (5x slower)

1sec delay between write operations

• creation: 352 seconds (22x and 2.4x slower)
• refresh: 25 seconds (6x slower and equal)
• destroy: 150 seconds (16x and 3x slower)

[appilon]

I think there might have been 1 bug I found and then a few gentle suggestions, however you might inform me every line is intentional at which point I will approve!

[appilon]

do you forsee this method needing rlt in scope? Not a huge preference but I would just define this as a helper drainBody as opposed to a method of *rateLimitTransport

[appilon]

this method I feel more strongly to having just as a little helper and not a method

[appilon]

nitpick suggestion:

rlt.delayNextRequest = rlt.isWriteMethod(req.Method)

[appilon]

need to check ghErr or reuse defined err

[appilon]

looking further down maybe this error check is not wanted at all? seems like ghErr is handled below

[radeksimko]

Good spot! 🙈

[appilon]

I usually always need a refresher when it comes to golangs io package (my mental model always thinks things are backwards). I have a feeling what you've written is exactly what you needed, but would io.TeeReader be of any use? passing the first reader to checkResponse which will fill the second reader with the content? My apologies if there is some gotcha you already explored.

[radeksimko]

I don't see how TeeReader would be handy without more re-buffering/hacking. 🤔
It seems to do the same thing, but it requires io.Writer as source and io.ReadCloser coming from the response body doesn't comply with that interface.

Maybe I'm missing something obvious?

[appilon]

I'm seeing now that github.CheckResponse accepts the response object, making the use of TeeReader impossible. The first time I read it I thought it accepted just a Reader, in that case TeeReader would have worked nicely

var r2 bytes.Buffer
r1 := io.TeeReader(resp.Body, &r2)
ghErr := github.CheckResponse(r1)
resp.Body = &r2

Also the interfaces might not have aligned, if resp.Body was ReadCloser you would have to wrap it just like you did in drainBody anyways nevermind me... always looking for an excuse to use io builtins 😛