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: improve request throttling [ZEND-4197] #1255

Merged
merged 1 commit into from
Oct 19, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions index.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ export type RunMigrationConfig = {
yes?: boolean
retryLimit?: number
requestBatchSize?: number
requestLimit?: number
host?: string
} & ({ filePath: string } | { migrationFunction: MigrationFunction })

Expand Down
71 changes: 71 additions & 0 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@
"kind-of": "^6.0.3",
"listr2": "^3.13.5",
"lodash": "^4.17.15",
"p-throttle": "^4.1.1",
"uuid": "^9.0.0",
"yargs": "^15.3.1"
},
Expand All @@ -75,11 +76,13 @@
"@types/chai": "^4.2.11",
"@types/chai-as-promised": "^7.1.2",
"@types/hoek": "^4.1.3",
"@types/inquirer": "^9.0.4",
"@types/joi": "^17.2.3",
"@types/lodash": "^4.14.149",
"@types/mocha": "^10.0.1",
"@types/node": "^17.0.14",
"@types/sinon-chai": "^3.2.4",
"@types/yargs": "^17.0.28",
"chai": "^4.2.0",
"chai-as-promised": "^7.1.1",
"commitizen": "^4.2.2",
Expand Down
36 changes: 27 additions & 9 deletions src/bin/cli.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,23 +2,27 @@ import * as path from 'path'
import type { AxiosRequestConfig } from 'axios'

import chalk from 'chalk'
import * as inquirer from 'inquirer'
import inquirer from 'inquirer'
import { Listr } from 'listr2'
import { createManagementClient } from './lib/contentful-client'
const { version } = require('../../package.json')
const { SpaceAccessError } = require('../lib/errors')
import createMigrationParser, { ParseResult } from '../lib/migration-parser'
import { renderPlan, renderValidationErrors, renderRuntimeErrors } from './lib/render-migration'
import { renderPlan, renderRuntimeErrors, renderValidationErrors } from './lib/render-migration'
import renderStepsErrors from './lib/steps-errors'
import writeErrorsToLog from './lib/write-errors-to-log'
import { RequestBatch } from '../lib/offline-api/index'
import { RequestBatch } from '../lib/offline-api'
import { getConfig } from './lib/config'
import ValidationError from '../lib/interfaces/errors'
import { PlainClientAPI } from 'contentful-management'
import trim from 'lodash/trim'

import { SpaceAccessError } from '../lib/errors'
import pThrottle from 'p-throttle'

const { version } = require('../../package.json')

class ManyError extends Error {
public errors: (Error | ValidationError)[]

constructor(message: string, errors: (Error | ValidationError)[]) {
super(message)
this.errors = errors
Expand All @@ -28,6 +32,7 @@ class ManyError extends Error {
class BatchError extends Error {
public batch: RequestBatch
public errors: Error[]

constructor(message: string, batch: RequestBatch, errors: Error[]) {
super(message)
this.batch = batch
Expand All @@ -45,7 +50,16 @@ const makeTerminatingFunction =
}
}

export const createMakeRequest = (client: PlainClientAPI, { spaceId, environmentId }) => {
export const createMakeRequest = (
client: PlainClientAPI,
{ spaceId, environmentId, limit = 10 }
) => {
const throttle = pThrottle({
limit,
interval: 1000,
strict: false
})

const makeBaseUrl = (url: string) => {
const parts = ['spaces', spaceId, 'environments', environmentId, trim(url, '/')]

Expand All @@ -56,11 +70,14 @@ export const createMakeRequest = (client: PlainClientAPI, { spaceId, environment
const { url, ...config } = requestConfig
const fullUrl = makeBaseUrl(url)

return client.raw.http(fullUrl, config)
return throttle(() => client.raw.http(fullUrl, config))()
Copy link
Contributor

Choose a reason for hiding this comment

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

Do I get this correctly that createMakeRequest is the only constructor in this project that creates functions which trigger API requests (makeRequest instances), and by wrapping the throttle here we effectively throttle all requests, both for the fetching in preparation of the migration as well as for the applying of the changes in the migration, because wherever api interactions happen they do with makeRequest?

Copy link
Member Author

Choose a reason for hiding this comment

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

Exactly!

}
}

const getMigrationFunctionFromFile = (filePath, terminate) => {
const getMigrationFunctionFromFile = (
filePath: string,
terminate: ReturnType<typeof makeTerminatingFunction>
) => {
try {
return require(filePath)
} catch (e) {
Expand Down Expand Up @@ -90,7 +107,8 @@ const createRun = ({ shouldThrow }) =>
const client = createManagementClient(clientConfig)
const makeRequest = createMakeRequest(client, {
spaceId: clientConfig.spaceId,
environmentId: clientConfig.environmentId
environmentId: clientConfig.environmentId,
limit: argv.requestLimit
})

const migrationParser = createMigrationParser(makeRequest, clientConfig)
Expand Down
7 changes: 7 additions & 0 deletions src/bin/usage-params.ts
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,13 @@ export default yargs
type: 'number',
default: 100
})
.option('request-limit', {
alias: 'r',
boolean: false,
describe: 'Max requests per second',
type: 'number',
default: 10
})
.option('header', {
alias: 'H',
type: 'string',
Expand Down