Skip to content

Commit

Permalink
bitbucketserver: Document rate limit change and rationale (#9048)
Browse files Browse the repository at this point in the history
* Revert "bitbucketserver: Increase client rate limit to 8 req/s (#9038)"

This reverts commit 128069c.

@tsenart in this commit you increased the Bitbucket Server rate limit, which is OK, but:

1. There isn't a CHANGELOG entry. Telling folks we plan to make up to 4x the number of requests to their Bitbucket instance (and why we think this isn't an issue) is _important_, especially because many customers are aware of us in the past having issues with taking down / causing harm to code hosts due to excessive API requests.
2. Even if we don't believe this will cause issues (which seems reasonable to me), we should have some documented form of recourse. This can be as simple as "If you encounter issues, please downgrade and contact support." in the CHANGELOG entry.
3. You removed the comment explaining how we chose these limits. Anyone reading this code will feel it is arbitrary, and anyone looking at the history will think we went from something logical ("same as bitbucket cloud") to something arbitrary (explanation removed).
4. You removed the `// 120/min or 7200/hr` comment, which is helpful when linking customers/users to this code as an explanation of the max we will do.

Note: I know I could've just fixed these for you more easily, my hope is this clarifies why I think this is important to communicate to our consumers.

* what I would do

* Update CHANGELOG.md

Co-authored-by: Tomás Senart <tomas@sourcegraph.com>
  • Loading branch information
slimsag and Tomás Senart committed Mar 17, 2020
1 parent 34817df commit 07ee5aa
Show file tree
Hide file tree
Showing 2 changed files with 13 additions and 1 deletion.
1 change: 1 addition & 0 deletions CHANGELOG.md
Expand Up @@ -34,6 +34,7 @@ All notable changes to Sourcegraph are documented in this file.
- Archived repositories are excluded from search by default. Adding `archived:yes` includes archived repositories.
- Forked repositories are excluded from search by default. Adding `fork:yes` includes forked repositories.
- CSRF and session cookies now set `SameSite=None` when Sourcegraph is running behind HTTPS and `SameSite=Lax` when Sourcegraph is running behind HTTP in order to comply with a [recent IETF proposal](https://web.dev/samesite-cookies-explained/#samesitenone-must-be-secure). As a side effect, the Sourcegraph browser extension and GitLab/Bitbucket native integrations can only connect to private instances that have HTTPS configured. If your private instance is only running behind HTTP, please configure your instance to use HTTPS in order to continue using these.
- The Bitbucket Server rate limit that Sourcegraph self-imposes has been raised from 120 req/min to 480 req/min to account for Sourcegraph instances that make use of Sourcegraphs' Bitbucket Server repository permissions and campaigns at the same time (which require a larger number of API requests aginst Bitbucket). The new number is based on us consuming roughly 8% the average API request rate against at large customers' Bitbucket Server instances. [#9048](https://github.com/sourcegraph/sourcegraph/pull/9048/files)
- If a single, unambiguous commit SHA is used in a search query (e.g., `repo@c98f56`) and a search index exists at this commit (i.e., it is the `HEAD` commit), then the query is searched using the index. Prior to this change, unindexed search was performed for any query containing an `@commit` specifier.

### Fixed
Expand Down
13 changes: 12 additions & 1 deletion internal/extsvc/bitbucketserver/client.go
Expand Up @@ -36,8 +36,19 @@ var requestCounter = metrics.NewRequestMeter("bitbucket", "Total number of reque
//
// See https://godoc.org/golang.org/x/time/rate#Limiter for an explanation of these fields.
//
// We chose the limits here based on the fact that Sourcegraph is a heavy consumer of the Bitbucket
// Server API and that a large customer had reported to us their Bitbucket instance receives
// ~100 req/s so it seems reasonable for us to (at max) consume ~8 req/s.
//
// Note that, for comparison, Bitbucket Cloud restricts "List all repositories" requests (which are
// a good portion of our requests) to 1,000/hr, and they restrict "List a user or team's repositories"
// requests (which are roughly equal to our repository lookup requests) to 1,000/hr. We perform a list
// repositories request for every 1000 repositories on Bitbucket every 1m by default, so for someone
// with 20,000 Bitbucket repositories we need 20,000/1000 requests per minute (1200/hr) + overhead for
// repository lookup requests by users, and requests for identifying which repositories a user has
// access to (if authorization is in use) and requests for campaign synchronization if it is in use.
const (
rateLimitRequestsPerSecond = 8
rateLimitRequestsPerSecond = 8 // 480/min or 28,800/hr
RateLimitMaxBurstRequests = 500
)

Expand Down

0 comments on commit 07ee5aa

Please sign in to comment.