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

feat(robot): handle ratelimiting with constant backoff #572

Merged
merged 1 commit into from Nov 30, 2023

Conversation

apricote
Copy link
Member

Add a constant backoff in case we are being rate limited by the Robot API.

This is done through an opaque wrapping of the robot.Client interface, so callers do not need to do this manually.

@apricote apricote added the enhancement New feature or request label Nov 30, 2023
@apricote apricote self-assigned this Nov 30, 2023
@apricote apricote requested a review from a team as a code owner November 30, 2023 12:07
Copy link
Member

@jooola jooola left a comment

Choose a reason for hiding this comment

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

What about using https://pkg.go.dev/github.com/syself/hrobot-go#NewBasicAuthClientWithCustomHttpClient and implement the rate limiter and cache around the http.Client ?

@@ -21,7 +20,7 @@ type cacheRobotClient struct {
serversByID map[int]*hrobotmodels.Server
}

func NewClient(robotClient hrobot.RobotClient, cacheTimeout time.Duration) Client {
func WithCache(cacheTimeout time.Duration, robotClient Client) Client {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
func WithCache(cacheTimeout time.Duration, robotClient Client) Client {
func NewCachedClient(robotClient Client, cacheTimeout time.Duration) Client {

Copy link
Member Author

Choose a reason for hiding this comment

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

I put the client last on purpose. I found it pretty confusing to have the client first when nesting, as the argument RateLimitWaitTime is not next to WithRateLimit anymore. For comparison:

Client last (current version):

robotClient = robot.WithRateLimit(
  robot.WithCache(
    cfg.Robot.CacheTimeout,
    c
  ),
  cfg.Robot.RateLimitWaitTime,
)

Client first (your suggestion):

robotClient = robot.WithRateLimit(
  cfg.Robot.RateLimitWaitTime,
  robot.WithCache(
    cfg.Robot.CacheTimeout,
    c
  ),
)

Copy link
Member

Choose a reason for hiding this comment

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

No strong opinion on my side, the naming confuses me only because I would not expect to have a new client when I call WithCache

Copy link
Member Author

Choose a reason for hiding this comment

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

Yea, I updated the naming :) Just did not agree with the changed order of arguments in your suggested replacement. (Sorry, should have been more explicit.)

internal/robot/ratelimit.go Outdated Show resolved Hide resolved
Add a constant backoff in case we are being rate limited by the Robot API.

This is done through an opaque wrapping of the robot.Client interface,
so callers do not need to do this manually.
@apricote
Copy link
Member Author

What about using https://pkg.go.dev/github.com/syself/hrobot-go#NewBasicAuthClientWithCustomHttpClient and implement the rate limiter and cache around the http.Client ?

IMO both additional features are based on the business logic of the API, rather than the transport mechanism.

For caching, we actually only use the list endpoint, and serve GET /servers/:id from the list cache. We do not cache the GET /reset/:id call.

For Rate Limiting we would need to parse the response in our middleware once more to evaluate the error, and similarly fake out the http response if we decide not to make a proper request.

Implement this on top of the Client interface is also simpler than doing it on the more generalized http.RoundTripper.

@apricote apricote merged commit 2ddc201 into main Nov 30, 2023
8 checks passed
@apricote apricote deleted the robot-ratelimit branch November 30, 2023 13:36
apricote pushed a commit that referenced this pull request Dec 1, 2023
🤖 I have created a release *beep* *boop*
---


##
[1.19.0-rc.0](v1.18.0...v1.19.0-rc.0)
(2023-12-01)


### Features

* **chart:** add daemonset and node selector
([#537](#537))
([a94384f](a94384f))
* **config:** stricter validation for settings
`HCLOUD_LOAD_BALANCERS_ENABLED`, `HCLOUD_METRICS_ENABLED` &
`HCLOUD_NETWORK_ROUTES_ENABLED`
([#546](#546))
([335a2c9](335a2c9))
* **helm:** remove "v" prefix from chart version
([#565](#565))
([f11aa0d](f11aa0d)),
closes
[#529](#529)
* **load-balancer:** handle planned targets exceedings max targets
([#570](#570))
([8bb131f](8bb131f))
* remove unused variable NODE_NAME
([#545](#545))
([a659408](a659408))
* **robot:** handle ratelimiting with constant backoff
([#572](#572))
([2ddc201](2ddc201))
* support for Robot servers
([#561](#561))
([65dea11](65dea11))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
apricote added a commit that referenced this pull request Dec 7, 2023
## [1.19.0](v1.18.0...v1.19.0)
(2023-12-07)


### Features

* **chart:** add daemonset and node selector
([#537](#537))
([a94384f](a94384f))
* **config:** stricter validation for settings
`HCLOUD_LOAD_BALANCERS_ENABLED`, `HCLOUD_METRICS_ENABLED` &
`HCLOUD_NETWORK_ROUTES_ENABLED`
([#546](#546))
([335a2c9](335a2c9))
* **helm:** remove "v" prefix from chart version
([#565](#565))
([f11aa0d](f11aa0d)),
closes
[#529](#529)
* **load-balancer:** handle planned targets exceedings max targets
([#570](#570))
([8bb131f](8bb131f))
* remove unused variable NODE_NAME
([#545](#545))
([a659408](a659408))
* **robot:** handle ratelimiting with constant backoff
([#572](#572))
([2ddc201](2ddc201))
* support for Robot servers
([#561](#561))
([65dea11](65dea11))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants