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

Make rate-limiting configurable #907

Merged
merged 14 commits into from
Oct 5, 2021

Conversation

kfcampbell
Copy link
Member

@kfcampbell kfcampbell commented Sep 21, 2021

Fixes #845.

This PR provides a configurable delay to sleep between writes for our rate-limiting construct. Inspired by our very talented coworker Dave Cheney's "Functional options for friendly APIs" blog post. The default is the same as before, which is sleeping 1000ms between write operations.

Example terraform:

terraform {
  required_providers {
    github = {
      source = "integrations/github"
    }
  }
}

provider "github" {
  owner = "kfcampbell-terraform-provider"
  write_delay_ms = 50 # sleep for 50ms in between requests
}

resource "github_repository" "writedelay-test-repo-0" {
  name        = "writedelay-test-repo-0"
  visibility  = "private"
  description = "A test repository for looking at write delays."
}

Example log output showing new sleeping times:

2021-09-21T21:41:48.125-0700 [DEBUG] provider.terraform-provider-github: 2021/09/21 21:41:48 [DEBUG] Sleeping 50ms between write operations

Example error output when a negative value is given:
image

Example error output when a string is given:
image

Example error output when a cartoonishly large number (9999999999999999999999) is given:
image


@kfcampbell's TODO list:
Some things that need to be addressed before making ready for review:

  • Validate that the integer is positive
  • Is it desirable to give a default in both the schema and in the transport?
    • I think this is necessary.
  • Manual testing
  • Look into seeing if this is something we can test as part of an integration test
    • Existing transport_test.go tests don't test any of the behavior with write delays
    • I'm not quite sure how I'd go about adding this but I'm receptive to ideas if others have them.

@kfcampbell
Copy link
Member Author

Questions for reviewers:

  • Is write_delay an appropriate name? Should I use write_delay_ms or something else entirely?
  • Are there ways of integration testing this behavior? There's no prior art.
    • Maybe we could do something where we invoke a timer and artificially make ourselves rate-limited? I'm not quite seeing how we'd do this.
    • Maybe it doesn't need it?
  • Are there additional docs updates that are missing from this PR?

@kfcampbell kfcampbell marked this pull request as ready for review September 22, 2021 04:55
Copy link
Contributor

@jcudit jcudit left a comment

Choose a reason for hiding this comment

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

This is on the right track. Added some cosmetic suggestions but likely to ship in the upcoming release.

github/provider.go Outdated Show resolved Hide resolved
github/transport.go Show resolved Hide resolved
github/transport.go Outdated Show resolved Hide resolved
website/docs/index.html.markdown Outdated Show resolved Hide resolved
kfcampbell and others added 4 commits September 28, 2021 22:52
Co-authored-by: Jeremy Udit <jcudit@github.com>
Co-authored-by: Jeremy Udit <jcudit@github.com>
Co-authored-by: Jeremy Udit <jcudit@github.com>
@jcudit jcudit added this to the v4.16.0 milestone Oct 5, 2021
Copy link
Contributor

@jcudit jcudit left a comment

Choose a reason for hiding this comment

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

Excited to see this one land, thanks!

@jcudit jcudit merged commit 696168b into integrations:main Oct 5, 2021
kfcampbell added a commit to kfcampbell/terraform-provider-github that referenced this pull request Jul 26, 2022
* Make struct type public

* Scaffold functional pattern for providing writeDelay

* Add WriteDelay to config and plumb it down

* Add configuration for write delay

* Validate writeDelay is positive

* Add website description of write_delay argument

* Use convenience func to pass writeDelay into rate limiter

* Move convenience func closer to implementation; add convenience type

* Rename write_delay --> write_delay_ms

* Update github/provider.go

Co-authored-by: Jeremy Udit <jcudit@github.com>

* Describe default for write_delay_ms in help text

* Update github/transport.go

Co-authored-by: Jeremy Udit <jcudit@github.com>

* Update website/docs/index.html.markdown

Co-authored-by: Jeremy Udit <jcudit@github.com>

Co-authored-by: Jeremy Udit <jcudit@github.com>
@kfcampbell kfcampbell deleted the optional-ratelimit-config branch December 1, 2022 19:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Disable RateLimitTransport
2 participants