bitbucket server api rate limiting #24860

kjjuno · 2024-05-21T20:56:48Z

Hey, I just made a Pull Request!

Adds the ability to rate limit calls to the bitbucket server api. If no rateLimit is provided in the configuration no rate limiting will be applied.

✔️ Checklist

A changeset describing the change and affected packages. (more info)
Added or updated documentation
Tests for new functionality and regression tests for bug fixes
Screenshots attached (for UI changes)
All your commits have a Signed-off-by line in the message. (more info)

backstage-goalie · 2024-05-21T21:13:38Z

Unexpected Changesets

The following changeset(s) reference packages that have not been changed in this PR:

/home/runner/work/backstage/backstage/.changeset/tidy-ducks-scream.md: @backstage/plugin-catalog-backend

Note that only changes that affect the published package require changesets, for example changes to tests and storybook stories do not require changesets.

Changed Packages

Package Name	Package Path	Changeset Bump	Current Version
@backstage/backend-defaults	packages/backend-defaults	patch	`v0.5.0-next.0`
@backstage/integration	packages/integration	minor	`v1.14.0`
@backstage/plugin-bitbucket-cloud-common	plugins/bitbucket-cloud-common	patch	`v0.2.22`
@backstage/plugin-catalog-backend-module-bitbucket-server	plugins/catalog-backend-module-bitbucket-server	patch	`v0.2.2-next.0`

kjjuno · 2024-05-21T21:18:16Z

Missing Changesets

The following package(s) are changed by this PR but do not have a changeset:

@backstage/integration

See CONTRIBUTING.md for more information about how to add changesets.

Changed Packages

Package Name Package Path Changeset Bump Current Version
@backstage/integration packages/integration none v1.11.0
@backstage/plugin-catalog-backend-module-bitbucket-server plugins/catalog-backend-module-bitbucket-server minor v0.1.33-next.0

I'm not sure how the @backstage/integration package is showing as changed. I didn't think I had changed it. Is this a mistake because of the merge conflicts I dealt with, or am I legitimately missing something here?

vinzscam

hey, question: are you getting the rate limiting when using the BitbucketServerEntityProvider? 🤔

Looking at the code it doesn't seem you should get any rate limiting

vinzscam · 2024-05-22T09:20:28Z

Missing Changesets

The following package(s) are changed by this PR but do not have a changeset:

@backstage/integration

See CONTRIBUTING.md for more information about how to add changesets.

Changed Packages

Package Name Package Path Changeset Bump Current Version
@backstage/integration packages/integration none v1.11.0
@backstage/plugin-catalog-backend-module-bitbucket-server plugins/catalog-backend-module-bitbucket-server minor v0.1.33-next.0

I'm not sure how the @backstage/integration package is showing as changed. I didn't think I had changed it. Is this a mistake because of the merge conflicts I dealt with, or am I legitimately missing something here?

you have changed packages/integration/src/bitbucketServer/config.ts, that's why 😄

backstage-goalie · 2024-05-22T14:58:05Z

Thanks for the contribution!
All commits need to be DCO signed before they are reviewed. Please refer to the the DCO section in CONTRIBUTING.md or the DCO status for more info.

kjjuno · 2024-05-22T15:36:29Z

hey, question: are you getting the rate limiting when using the BitbucketServerEntityProvider? 🤔

Looking at the code it doesn't seem you should get any rate limiting

We added this plugin to scrape our bitbucket server instance, and almost immediately we started getting performance issues on our server. The team that owns that has asked us to make sure we only hit the api around 1 time per second.

This PR adds the ability rate limit those calls by making use of the Bottleneck library.

The rate limiting is implemented in the BitbucketServerClient itself.

Does this address your concern, or do you still have questions or concerns?

vinzscam · 2024-05-22T18:17:01Z

BitbucketServerClient

my question was more about: are you using the BitbucketServerEntityProvider or are you using the BitbucketServerClient directly?

kjjuno · 2024-05-22T18:38:04Z

we are using the BitbucketServerEntityProvider

Rugvip

Thank you! 👍

plugins/catalog-backend-module-bitbucket-server/src/lib/BitbucketServerClient.ts

packages/integration/src/bitbucketServer/config.ts

plugins/catalog-backend-module-bitbucket-server/src/lib/BitbucketServerClient.ts

.changeset/silent-carpets-explode.md

plugins/catalog-backend-module-bitbucket-server/src/providers/BitbucketServerEntityProvider.ts

github-actions · 2024-06-19T18:31:08Z

This PR has been automatically marked as stale because it has not had recent activity from the author. It will be closed if no further activity occurs. If the PR was closed and you want it re-opened, let us know and we'll re-open the PR so that you can continue the contribution!

kjjuno · 2024-06-21T19:10:50Z

@freben how would you feel about going back to the approach of putting the throttling into the client? We could make it so that you need to opt into that behavior. If there is no throttling configured no throttling would be applied.

The client itself seems to be the only place I can guarantee that throttling is properly applied

kjjuno · 2024-07-30T16:17:48Z

Still waiting for the backstage team to get back from vacation. This should not be marked stale.

brunooliveiramac · 2024-07-31T18:34:44Z

thumbs up for this

kjjuno · 2024-08-12T15:20:59Z

Any chance I could get a new review? This is a critical enhancement for us. If there are still any outstanding concerns please let me know.

plugins/bitbucket-cloud-common/package.json

benjdlambert

Hey 👋

Apologies for the long turnaround on reviews for this, we've been pretty slammed after the holiday season, so we're just getting back to full capacity again!

I'm thinking that whilst we might get to having some form of FetchService in the backend, similar to the fetchApiRef in the frontend, we're not really there yet and we don't know what that's going to look like either. I don't want to be even more of a blocker for this work, so what I would suggest is that we narrow down the changes for what's required for this PR just to the @backstage/plugin-bitbucket* packages instead, and leave out the other changes for now.

It might need some restructuring of course, to maybe have this in some other config variable or to be set in some other way in the interim and out of the integrations.config for now, but I think that it's probably easier to go down that path, and then eventually deprecate that way once we have some more thoughts about what we're going to do with a global FetchService.

Does this make sense for now?

packages/integration/api-report.md

kjjuno · 2024-08-19T20:59:45Z

Hey 👋

Apologies for the long turnaround on reviews for this, we've been pretty slammed after the holiday season, so we're just getting back to full capacity again!

I'm thinking that whilst we might get to having some form of FetchService in the backend, similar to the fetchApiRef in the frontend, we're not really there yet and we don't know what that's going to look like either. I don't want to be even more of a blocker for this work, so what I would suggest is that we narrow down the changes for what's required for this PR just to the @backstage/plugin-bitbucket* packages instead, and leave out the other changes for now.

It might need some restructuring of course, to maybe have this in some other config variable or to be set in some other way in the interim and out of the integrations.config for now, but I think that it's probably easier to go down that path, and then eventually deprecate that way once we have some more thoughts about what we're going to do with a global FetchService.

Does this make sense for now?

Totally makes sense. I'll restructure things to make that service only for the bitbucket plugins. I can also change the config as @joshjung suggested. I should have something up hopefully as early as tomorrow

joshjung · 2024-08-20T16:42:53Z

Totally makes sense. I'll restructure things to make that service only for the bitbucket plugins. I can also change the config as @joshjung suggested. I should have something up hopefully as early as tomorrow

Great! Once you have this done, what I can do is restructure the PR I have for the Gitlab plugin to match the same config format you use.

Signed-off-by: Kevin Johnson <kljohnson@verisk.com>

kjjuno · 2024-08-21T16:11:36Z

This is ready for another look now I think

freben

Sorry for the delay and the large amount of comments. This turns out to be a very tricky thing, in part because of previous decisions that are none of your fault, but also because of just the tricky nature of the integrations package and polymorphism. That package is being revamped in some nearish future, making things a lot simpler. But here we are.

Let me summarize the comments:

The -common packages cannot use node-fetch at all
The integrations package needs to be kept almost perfectly pristine and must not contain any implementation code

The effect of this is:

Need to make a node typed package, preferably @backstage/integration-bitbucket-node and have all implementation there
Dial back the integrations package even more to exclusively the throttler type change
Change things to accept a typeof fetch making it entirely transparent to their implementations what the fetcher actually is doing.

freben · 2024-08-29T07:10:00Z

docs/integrations/bitbucketCloud/locations.md

@@ -20,6 +20,11 @@ integrations:
  bitbucketCloud:
    - username: ${BITBUCKET_CLOUD_USERNAME}
      appPassword: ${BITBUCKET_CLOUD_PASSWORD}
+      fetch: # optional


Might as well remove this nesting level (in the docs below it's not here)

This was something I specifically requested because we were introducing a new object that was going to be shared across multiple configurations for customizing fetch. Since I have to add it in another spot, would it not make sense to have the nesting so that there is a consistent indicator across configuration locations, since most likely this throttling will end up in a lot of spots in the future? What if we also want to add more configurations for fetching?

freben · 2024-08-29T07:10:07Z

docs/integrations/bitbucketServer/locations.md

@@ -32,6 +37,11 @@ integrations:
      apiBaseUrl: https://bitbucket.mycompany.com/rest/api/1.0
      username: ${BITBUCKET_SERVER_USERNAME}
      password: ${BITBUCKET_SERVER_PASSWORD}
+      fetch: # optional


remove nesting

freben · 2024-08-29T07:12:06Z

packages/backend-defaults/src/entrypoints/urlReader/lib/BitbucketCloudUrlReader.ts

+  BitbucketFetchFunction,
+  BitbucketFetchService,


I really would want to get rid of these types. The core of it is, the integrations package should be left as pristine as is humanly possible. See comments below.

freben · 2024-08-29T07:12:34Z

packages/backend-defaults/src/entrypoints/urlReader/lib/BitbucketCloudUrlReader.ts

  getBitbucketCloudDefaultBranch,
  getBitbucketCloudDownloadUrl,
  getBitbucketCloudFileFetchUrl,
  getBitbucketCloudRequestOptions,
  ScmIntegrations,
 } from '@backstage/integration';
-import fetch, { Response } from 'node-fetch';


keep this import

freben · 2024-08-29T07:12:46Z

packages/backend-defaults/src/entrypoints/urlReader/lib/BitbucketCloudUrlReader.ts

@@ -58,6 +60,8 @@ export class BitbucketCloudUrlReader implements UrlReaderService {
    });
  };

+  private readonly fetch: BitbucketFetchFunction;


Suggested change

private readonly fetch: BitbucketFetchFunction;

private readonly fetch: typeof fetch;

freben · 2024-08-29T07:37:10Z

packages/integration/package.json

+    "node-fetch": "^2.6.7",
+    "p-throttle": "^4.1.1"


freben · 2024-08-29T07:41:34Z

packages/integration/src/bitbucket/BitbucketFetchService.ts

+ * This service can be configured to throttle the number of requests that can be made.
+ * @internal
+ */
+export class BitbucketFetchService {


If you make a @backstage/integration-bitbucket-node package, it could have a

function createThrottledFetch(options: { inner: typeof fetch, throttleByHost: { [host in string]: ThrottleConfig; } }): typeof fetch

or something like that, and make it like a "middleware-like" thing around the inner fetch, like we do when we create the fetchApi for the frontend.

No need for caching fetch implementations in a map. Its internal state will be a Map<string, typeof pThrottle> instead.

freben · 2024-08-29T07:43:42Z

plugins/bitbucket-cloud-common/package.json

@@ -42,11 +42,13 @@
  },
  "dependencies": {
    "@backstage/integration": "workspace:^",
-    "cross-fetch": "^4.0.0"
+    "cross-fetch": "^4.0.0",
+    "node-fetch": "^2.6.7"


freben · 2024-08-29T07:43:46Z

plugins/bitbucket-cloud-common/package.json

  },
  "devDependencies": {
    "@backstage/cli": "workspace:^",
    "@openapitools/openapi-generator-cli": "^2.4.26",
+    "@types/node-fetch": "^2.5.12",


freben · 2024-08-29T07:44:53Z

plugins/bitbucket-cloud-common/src/BitbucketCloudClient.ts

@@ -32,9 +36,13 @@ export class BitbucketCloudClient {
    return new BitbucketCloudClient(config);
  }

+  private readonly fetch: BitbucketFetchFunction;


This one should also accept a fetch?: typeof fetch in its constructor. But we'll see if typescript gets cranky with the node-fetch vs cross-fetch types.

github-actions · 2024-09-20T10:53:48Z

This PR has been automatically marked as stale because it has not had recent activity from the author. It will be closed if no further activity occurs. If the PR was closed and you want it re-opened, let us know and we'll re-open the PR so that you can continue the contribution!

kjjuno · 2024-09-20T20:07:30Z

I am still working through this. I have been distracted by other priorities at work, but this is still very much on my mind and I intend to have an update posted next week sometime.

kjjuno requested review from a team and backstage-service as code owners May 21, 2024 20:56

kjjuno requested review from Rugvip and vinzscam May 21, 2024 20:56

github-actions bot added the area:catalog Related to the Catalog Project Area label May 21, 2024

kjjuno force-pushed the feature/bitbucket-server-rate-limiting branch from 384b54c to 9c2ffd8 Compare May 21, 2024 21:13

kjjuno changed the title ~~Adds bitbucket server rate limiting~~ bitbucket server api rate limiting May 21, 2024

vinzscam requested changes May 22, 2024

View reviewed changes

kjjuno force-pushed the feature/bitbucket-server-rate-limiting branch 2 times, most recently from f9bc959 to 7112134 Compare May 22, 2024 16:39

kjjuno requested a review from vinzscam May 23, 2024 00:39

Rugvip requested changes May 28, 2024

View reviewed changes

kjjuno force-pushed the feature/bitbucket-server-rate-limiting branch from 7112134 to b569090 Compare June 4, 2024 14:54

github-actions bot added the documentation Improvements or additions to documentation label Jun 4, 2024

kjjuno requested a review from Rugvip June 4, 2024 21:34

freben reviewed Jun 5, 2024

View reviewed changes

kjjuno requested a review from freben June 5, 2024 18:18

github-actions bot added the stale label Jun 19, 2024

freben removed the stale label Jun 19, 2024

github-actions bot added the stale label Jul 30, 2024

freben removed the stale label Aug 6, 2024

kjjuno requested a review from cal5barton August 13, 2024 15:01

cal5barton reviewed Aug 13, 2024

View reviewed changes

plugins/bitbucket-cloud-common/package.json Show resolved Hide resolved

benjdlambert requested changes Aug 19, 2024

View reviewed changes

benjdlambert mentioned this pull request Aug 19, 2024

Adding ability to throttle Gitlab discovery via sleepBetweenMs #26063

Open

5 tasks

joshjung reviewed Aug 19, 2024

View reviewed changes

packages/integration/api-report.md Outdated Show resolved Hide resolved

Kevin Johnson added 5 commits August 20, 2024 13:05

Merge branch 'master' into feature/bitbucket-server-rate-limiting

2a0ca98

chore: Make FetchService more specific to bitbucket

7fb56f3

Signed-off-by: Kevin Johnson <kljohnson@verisk.com>

Update api report

5836f41

Signed-off-by: Kevin Johnson <kljohnson@verisk.com>

chore: Fix api-report warnings

cf5c7b5

Signed-off-by: Kevin Johnson <kljohnson@verisk.com>

chore: Fix prettier formatting

d8a6bf0

Signed-off-by: Kevin Johnson <kljohnson@verisk.com>

github-actions bot added the BEP Backstage Enhancement Proposals label Aug 20, 2024

Update docs

dc28db7

Signed-off-by: Kevin Johnson <kljohnson@verisk.com>

kjjuno force-pushed the feature/bitbucket-server-rate-limiting branch from 746a96c to dc28db7 Compare August 20, 2024 22:52

Merge branch 'master' into feature/bitbucket-server-rate-limiting

ec3d749

kjjuno requested review from joshjung and benjdlambert August 21, 2024 16:11

freben reviewed Aug 29, 2024

View reviewed changes

Merge branch 'master' into feature/bitbucket-server-rate-limiting

66ddf49

github-actions bot added the stale label Sep 20, 2024

github-actions bot removed the stale label Sep 20, 2024

	private readonly fetch: BitbucketFetchFunction;
	private readonly fetch: typeof fetch;

bitbucket server api rate limiting #24860

Are you sure you want to change the base?

bitbucket server api rate limiting #24860

Conversation

kjjuno commented May 21, 2024 • edited Loading

Hey, I just made a Pull Request!

✔️ Checklist

backstage-goalie bot commented May 21, 2024 • edited Loading

Unexpected Changesets

Changed Packages

kjjuno commented May 21, 2024

Missing Changesets

Changed Packages

vinzscam left a comment

Choose a reason for hiding this comment

vinzscam commented May 22, 2024

Missing Changesets

Changed Packages

backstage-goalie bot commented May 22, 2024

kjjuno commented May 22, 2024

vinzscam commented May 22, 2024

kjjuno commented May 22, 2024

Rugvip left a comment

Choose a reason for hiding this comment

github-actions bot commented Jun 19, 2024

kjjuno commented Jun 21, 2024

kjjuno commented Jul 30, 2024

brunooliveiramac commented Jul 31, 2024

kjjuno commented Aug 12, 2024

benjdlambert left a comment

Choose a reason for hiding this comment

kjjuno commented Aug 19, 2024

joshjung commented Aug 20, 2024

kjjuno commented Aug 21, 2024

freben 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

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

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

github-actions bot commented Sep 20, 2024

kjjuno commented Sep 20, 2024

kjjuno commented May 21, 2024 •

edited

Loading

backstage-goalie bot commented May 21, 2024 •

edited

Loading