From 2aced303c4b03ba384c388366c9a970b325da54b Mon Sep 17 00:00:00 2001 From: Alex Ross Date: Fri, 10 Mar 2023 16:42:36 +0100 Subject: [PATCH 1/2] Add an internal rate limit Fixes #4546 --- package.json | 1 + ....proposed.contribEditorLineNumberMenu.d.ts | 8 ++ .../vscode.proposed.contribShareMenu.d.ts | 1 + .../vscode.proposed.quickDiffProvider.d.ts | 4 + src/github/credentials.ts | 2 +- src/github/loggingOctokit.ts | 89 +++++++++---------- src/test/mocks/mockGitHubRepository.ts | 3 +- src/test/view/prsTree.test.ts | 2 +- yarn.lock | 12 +++ 9 files changed, 70 insertions(+), 52 deletions(-) create mode 100644 src/@types/vscode.proposed.contribEditorLineNumberMenu.d.ts diff --git a/package.json b/package.json index f049a59f26..8ce5250f5f 100644 --- a/package.json +++ b/package.json @@ -2362,6 +2362,7 @@ "dayjs": "1.10.4", "events": "3.2.0", "fast-deep-equal": "^3.1.3", + "limiter": "^2.1.0", "lru-cache": "6.0.0", "marked": "^4.0.10", "react": "^16.12.0", diff --git a/src/@types/vscode.proposed.contribEditorLineNumberMenu.d.ts b/src/@types/vscode.proposed.contribEditorLineNumberMenu.d.ts new file mode 100644 index 0000000000..7ee400c56b --- /dev/null +++ b/src/@types/vscode.proposed.contribEditorLineNumberMenu.d.ts @@ -0,0 +1,8 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See License.txt in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +// empty placeholder declaration for the `editor/lineNumber/context` menu contribution point + +// https://github.com/microsoft/vscode/issues/175945 @joyceerhl diff --git a/src/@types/vscode.proposed.contribShareMenu.d.ts b/src/@types/vscode.proposed.contribShareMenu.d.ts index a38d03f4fd..e308029d4e 100644 --- a/src/@types/vscode.proposed.contribShareMenu.d.ts +++ b/src/@types/vscode.proposed.contribShareMenu.d.ts @@ -4,3 +4,4 @@ *--------------------------------------------------------------------------------------------*/ // empty placeholder declaration for the `file/share`-submenu contribution point +// https://github.com/microsoft/vscode/issues/176316 diff --git a/src/@types/vscode.proposed.quickDiffProvider.d.ts b/src/@types/vscode.proposed.quickDiffProvider.d.ts index c0c0078f56..3778e7092e 100644 --- a/src/@types/vscode.proposed.quickDiffProvider.d.ts +++ b/src/@types/vscode.proposed.quickDiffProvider.d.ts @@ -10,4 +10,8 @@ declare module 'vscode' { export namespace window { export function registerQuickDiffProvider(selector: DocumentSelector, quickDiffProvider: QuickDiffProvider, label: string, rootUri?: Uri): Disposable; } + + interface QuickDiffProvider { + label?: string; + } } diff --git a/src/github/credentials.ts b/src/github/credentials.ts index a738bb8c04..43b7cc933d 100644 --- a/src/github/credentials.ts +++ b/src/github/credentials.ts @@ -347,7 +347,7 @@ export class CredentialStore implements vscode.Disposable { }, }); - const rateLogger = new RateLogger(this._context, this._telemetry); + const rateLogger = new RateLogger(this._telemetry); const github: GitHub = { octokit: new LoggingOctokit(octokit, rateLogger), graphql: new LoggingApolloClient(graphql, rateLogger), diff --git a/src/github/loggingOctokit.ts b/src/github/loggingOctokit.ts index 759f7c3f24..5b0cb4314e 100644 --- a/src/github/loggingOctokit.ts +++ b/src/github/loggingOctokit.ts @@ -5,14 +5,12 @@ import { Octokit } from '@octokit/rest'; import { ApolloClient, ApolloQueryResult, FetchResult, MutationOptions, NormalizedCacheObject, OperationVariables, QueryOptions } from 'apollo-boost'; +import { RateLimiter } from 'limiter'; import * as vscode from 'vscode'; import Logger from '../common/logger'; import { ITelemetry } from '../common/telemetry'; import { RateLimit } from './graphql'; -const RATE_COUNTER_LAST_WINDOW = 'rateCounterLastWindow'; -const RATE_COUNTER_COUNT = 'rateCounterCount'; - interface RestResponse { headers: { 'x-ratelimit-limit': string; @@ -21,44 +19,30 @@ interface RestResponse { } export class RateLogger { - private lastWindow: number; - private count: number = 0; + private limiter: RateLimiter; private static ID = 'RateLimit'; private hasLoggedLowRateLimit: boolean = false; - constructor(private readonly context: vscode.ExtensionContext, private readonly telemetry: ITelemetry) { - // We assume the common case for this logging: only one user. - // We also make up our own window. This will not line up exactly with GitHub's rate limit reset time, - // but it will give us a nice idea of how many API calls we're making. We use an hour, just like GitHub. - this.lastWindow = this.context.globalState.get(RATE_COUNTER_LAST_WINDOW, 0); - // It looks like there might be separate rate limits for the REST and GraphQL api. - // We'll just count total API calls as a lower bound. - this.count = this.context.globalState.get(RATE_COUNTER_COUNT, 0); - this.tryUpdateWindow(); - } - - private tryUpdateWindow() { - const now = new Date().getTime(); - if ((now - this.lastWindow) > (60 * 60 * 1000) /* 1 hour */) { - this.lastWindow = now; - this.context.globalState.update(RATE_COUNTER_LAST_WINDOW, this.lastWindow); - this.count = 0; - } + constructor(private readonly telemetry: ITelemetry) { + this.limiter = new RateLimiter({ tokensPerInterval: 100, interval: 'second' }); } - public log(info: string | undefined) { - this.tryUpdateWindow(); - this.count++; - this.context.globalState.update(RATE_COUNTER_COUNT, this.count); - const countMessage = `API call count: ${this.count}${info ? ` (${info})` : ''}`; - if (this.count > 4000) { - Logger.appendLine(countMessage, RateLogger.ID); - } else { - Logger.debug(countMessage, RateLogger.ID); + public logAndLimit(): boolean { + if (!this.limiter.tryRemoveTokens(1)) { + Logger.appendLine('API call count has exceeded 100 calls in 1 second.', RateLogger.ID); + // We have hit 100 requests in 1 second. This likely indicates a bug in the extension. + /* __GDPR__ + "pr.highApiCallRate" : {} + */ + this.telemetry.sendTelemetryErrorEvent('pr.highApiCallRate'); + vscode.window.showErrorMessage(vscode.l10n.t('The GitHub Pull Requests extension is making too many requests to GitHub. This indicates a bug in the extension. Please file an issue on GitHub and include the output from "GitHub Pull Request".')); + return false; } + Logger.debug(`Extension rate limit remaining: ${this.limiter.getTokensRemaining()}`, RateLogger.ID); + return true; } - public async logRateLimit(result: Promise<{ data: { rateLimit: RateLimit | undefined } | undefined } | undefined>, isRest: boolean = false) { + public async logRateLimit(info: string | undefined, result: Promise<{ data: { rateLimit: RateLimit | undefined } | undefined } | undefined>, isRest: boolean = false) { let rateLimitInfo; try { rateLimitInfo = (await result)?.data?.rateLimit; @@ -69,7 +53,7 @@ export class RateLogger { if ((rateLimitInfo?.limit ?? 5000) < 5000) { Logger.appendLine(`Unexpectedly low rate limit: ${rateLimitInfo?.limit}`, RateLogger.ID); } - const remaining = `${isRest ? 'REST' : 'GraphQL'} Rate limit remaining: ${rateLimitInfo?.remaining}`; + const remaining = `${isRest ? 'REST' : 'GraphQL'} Rate limit remaining: ${rateLimitInfo?.remaining}, ${info}`; if ((rateLimitInfo?.remaining ?? 1000) < 1000) { Logger.appendLine(remaining, RateLogger.ID); } else { @@ -83,7 +67,7 @@ export class RateLogger { } } - public async logRestRateLimit(restResponse: Promise) { + public async logRestRateLimit(info: string | undefined, restResponse: Promise) { let result; try { result = await restResponse; @@ -97,7 +81,7 @@ export class RateLogger { remaining: Number(result.headers['x-ratelimit-remaining']), resetAt: '' }; - this.logRateLimit(Promise.resolve({ data: { rateLimit } }), true); + this.logRateLimit(info, Promise.resolve({ data: { rateLimit } }), true); } } @@ -105,17 +89,23 @@ export class LoggingApolloClient { constructor(private readonly _graphql: ApolloClient, private _rateLogger: RateLogger) { }; query(options: QueryOptions): Promise> { - this._rateLogger.log((options.query.definitions[0] as { name: { value: string } | undefined }).name?.value); - const result = this._graphql.query(options); - this._rateLogger.logRateLimit(result as any); - return result; + if (this._rateLogger.logAndLimit()) { + const result = this._graphql.query(options); + this._rateLogger.logRateLimit((options.query.definitions[0] as { name: { value: string } | undefined }).name?.value, result as any); + return result; + } else { + throw new Error('API call count has exceeded a rate limit.'); + } } mutate(options: MutationOptions): Promise> { - this._rateLogger.log(options.context); - const result = this._graphql.mutate(options); - this._rateLogger.logRateLimit(result as any); - return result; + if (this._rateLogger.logAndLimit()) { + const result = this._graphql.mutate(options); + this._rateLogger.logRateLimit(options.context, result as any); + return result; + } else { + throw new Error('API call count has exceeded a rate limit.'); + } } } @@ -123,9 +113,12 @@ export class LoggingOctokit { constructor(public readonly api: Octokit, private _rateLogger: RateLogger) { }; async call(api: (T) => Promise, args: T): Promise { - this._rateLogger.log((api as unknown as { endpoint: { DEFAULTS: { url: string } | undefined } | undefined }).endpoint?.DEFAULTS?.url); - const result = api(args); - this._rateLogger.logRestRateLimit(result as Promise as Promise); - return result; + if (this._rateLogger.logAndLimit()) { + const result = api(args); + this._rateLogger.logRestRateLimit((api as unknown as { endpoint: { DEFAULTS: { url: string } | undefined } | undefined }).endpoint?.DEFAULTS?.url, result as Promise as Promise); + return result; + } else { + throw new Error('API call count has exceeded a rate limit.'); + } } } diff --git a/src/test/mocks/mockGitHubRepository.ts b/src/test/mocks/mockGitHubRepository.ts index 144e1a162e..63f5805936 100644 --- a/src/test/mocks/mockGitHubRepository.ts +++ b/src/test/mocks/mockGitHubRepository.ts @@ -20,7 +20,6 @@ import { import { MockTelemetry } from './mockTelemetry'; import { Uri } from 'vscode'; import { LoggingOctokit, RateLogger } from '../../github/loggingOctokit'; -import { MockExtensionContext } from './mockExtensionContext'; const queries = require('../../github/queries.gql'); export class MockGitHubRepository extends GitHubRepository { @@ -32,7 +31,7 @@ export class MockGitHubRepository extends GitHubRepository { this.queryProvider = new QueryProvider(sinon); this._hub = { - octokit: new LoggingOctokit(this.queryProvider.octokit, new RateLogger(new MockExtensionContext(), new MockTelemetry())), + octokit: new LoggingOctokit(this.queryProvider.octokit, new RateLogger(new MockTelemetry())), graphql: null, }; diff --git a/src/test/view/prsTree.test.ts b/src/test/view/prsTree.test.ts index 311801b2a0..8f1f124db3 100644 --- a/src/test/view/prsTree.test.ts +++ b/src/test/view/prsTree.test.ts @@ -54,7 +54,7 @@ describe('GitHub Pull Requests view', function () { baseUrl: 'https://github.com', userAgent: 'GitHub VSCode Pull Requests', previews: ['shadow-cat-preview'], - }), new RateLogger(context, telemetry)), + }), new RateLogger(telemetry)), graphql: null, }; diff --git a/yarn.lock b/yarn.lock index 454c37ca3d..eaf2bc0c48 100644 --- a/yarn.lock +++ b/yarn.lock @@ -3625,6 +3625,11 @@ just-extend@^4.0.2: resolved "https://registry.yarnpkg.com/just-extend/-/just-extend-4.1.1.tgz#158f1fdb01f128c411dc8b286a7b4837b3545282" integrity sha512-aWgeGFW67BP3e5181Ep1Fv2v8z//iBJfrvyTnq8wG86vEESwmonn1zPBJ0VfmT9CJq2FIT0VsETtrNFm2a+SHA== +just-performance@4.3.0: + version "4.3.0" + resolved "https://registry.yarnpkg.com/just-performance/-/just-performance-4.3.0.tgz#cc2bc8c9227f09e97b6b1df4cd0de2df7ae16db1" + integrity sha512-L7RjvtJsL0QO8xFs5wEoDDzzJwoiowRw6Rn/GnvldlchS2JQr9wFYPiwZcDfrbbujEKqKN0tvENdbjXdYhDp5Q== + keygrip@~1.1.0: version "1.1.0" resolved "https://registry.yarnpkg.com/keygrip/-/keygrip-1.1.0.tgz#871b1681d5e159c62a445b0c74b615e0917e7226" @@ -3741,6 +3746,13 @@ levn@~0.3.0: prelude-ls "~1.1.2" type-check "~0.3.2" +limiter@^2.1.0: + version "2.1.0" + resolved "https://registry.yarnpkg.com/limiter/-/limiter-2.1.0.tgz#d38d7c5b63729bb84fb0c4d8594b7e955a5182a2" + integrity sha512-361TYz6iay6n+9KvUUImqdLuFigK+K79qrUtBsXhJTLdH4rIt/r1y8r1iozwh8KbZNpujbFTSh74mJ7bwbAMOw== + dependencies: + just-performance "4.3.0" + lines-and-columns@^1.1.6: version "1.1.6" resolved "https://registry.yarnpkg.com/lines-and-columns/-/lines-and-columns-1.1.6.tgz#1c00c743b433cd0a4e80758f7b64a57440d9ff00" From ab91e26d0f326c6075aa9113d3a72f60b2d37114 Mon Sep 17 00:00:00 2001 From: Alex Ross Date: Fri, 10 Mar 2023 16:45:22 +0100 Subject: [PATCH 2/2] Fix some error/info messages --- src/github/loggingOctokit.ts | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/github/loggingOctokit.ts b/src/github/loggingOctokit.ts index 5b0cb4314e..11ad197eab 100644 --- a/src/github/loggingOctokit.ts +++ b/src/github/loggingOctokit.ts @@ -29,7 +29,7 @@ export class RateLogger { public logAndLimit(): boolean { if (!this.limiter.tryRemoveTokens(1)) { - Logger.appendLine('API call count has exceeded 100 calls in 1 second.', RateLogger.ID); + Logger.error('API call count has exceeded 100 calls in 1 second.', RateLogger.ID); // We have hit 100 requests in 1 second. This likely indicates a bug in the extension. /* __GDPR__ "pr.highApiCallRate" : {} @@ -55,14 +55,15 @@ export class RateLogger { } const remaining = `${isRest ? 'REST' : 'GraphQL'} Rate limit remaining: ${rateLimitInfo?.remaining}, ${info}`; if ((rateLimitInfo?.remaining ?? 1000) < 1000) { - Logger.appendLine(remaining, RateLogger.ID); - } else { if (!this.hasLoggedLowRateLimit) { /* __GDPR__ "pr.lowRateLimitRemaining" : {} */ this.telemetry.sendTelemetryErrorEvent('pr.lowRateLimitRemaining'); + this.hasLoggedLowRateLimit = true; } + Logger.warn(remaining, RateLogger.ID); + } else { Logger.debug(remaining, RateLogger.ID); } }