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

Migrate request to got (part 1 of many) #6160

Merged
merged 10 commits into from
Mar 9, 2021
Merged
Show file tree
Hide file tree
Changes from 9 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
5 changes: 4 additions & 1 deletion core/base-service/base.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ const {
Deprecated,
} = require('./errors')
const { validateExample, transformExample } = require('./examples')
const { fetchFactory } = require('./got')
const {
makeFullUrl,
assertValidRoute,
Expand Down Expand Up @@ -430,6 +431,8 @@ class BaseService {
ServiceClass: this,
})

const fetcher = fetchFactory(fetchLimitBytes)

camp.route(
regex,
handleRequest(cacheHeaderConfig, {
Expand All @@ -440,7 +443,7 @@ class BaseService {
const namedParams = namedParamsForMatch(captureNames, match, this)
const serviceData = await this.invoke(
{
sendAndCacheRequest: request.asPromise,
sendAndCacheRequest: fetcher,
sendAndCacheRequestWithCallbacks: request,
githubApiProvider,
metricHelper,
Expand Down
97 changes: 97 additions & 0 deletions core/base-service/got.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
'use strict'

const got = require('got')
const { Inaccessible, InvalidResponse } = require('./errors')

function requestOptions2GotOptions(options) {
Copy link
Member Author

Choose a reason for hiding this comment

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

This allows us to pass a request-like options object to got without modifying service code (mostly).
I reckon once we've completely removed all our usage of request, we remove this translation layer, update the service code and update https:/badges/shields/blob/master/doc/TUTORIAL.md to point to got's docs.

const requestOptions = Object.assign({}, options)
const gotOptions = {}
const interchangableOptions = ['body', 'form', 'headers', 'method', 'url']

interchangableOptions.forEach(function (opt) {
if (opt in requestOptions) {
gotOptions[opt] = requestOptions[opt]
delete requestOptions[opt]
}
})

if ('qs' in requestOptions) {
gotOptions.searchParams = requestOptions.qs
delete requestOptions.qs
}

if ('gzip' in requestOptions) {
gotOptions.decompress = requestOptions.gzip
delete requestOptions.gzip
}

if ('strictSSL' in requestOptions) {
gotOptions.https = {
rejectUnauthorized: requestOptions.strictSSL,
}
delete requestOptions.strictSSL
}

if ('auth' in requestOptions) {
gotOptions.username = requestOptions.auth.user
gotOptions.password = requestOptions.auth.pass
delete requestOptions.auth
}

if (Object.keys(requestOptions).length > 0) {
throw new Error(`Found unrecognised options ${Object.keys(requestOptions)}`)
Copy link
Member Author

Choose a reason for hiding this comment

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

I've decided to fail loudly if we don't know what to do with it. Obviously this massively constrains what selection of options we can use without modifying this, but I'm pretty certain I've covered the full subset of options we are actively using. If we try to use another one, we need to work out the translation not make an assumption.

Copy link
Member

Choose a reason for hiding this comment

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

For some reason I started with this file from sendRequest below, and then as I was reading through requestOptions2GotOptions found myself wondering why the keys were being deleted on requestOptions since it seemed like that was tossed/unused for the actual request but now see why as we're using it for validation.

I don't expect any issues with this, but since it will add processing steps on every request do we want to do some kind of perf run?

Copy link
Member

Choose a reason for hiding this comment

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

actually have thinking on this some more, such a hypothetical test would really need to be executed under load over an extended duration which doesn't really seem feasible/viable. i think this will be a case better handled by simply keeping an eye on prod for a while after deployed to be prepared to scale out or even roll back should something really go wildly unexpected

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think performance should be a worry here. The options object is always going to be a small object with only a few keys.

}

return gotOptions
}

async function sendRequest(gotWrapper, url, options) {
const gotOptions = requestOptions2GotOptions(options)
gotOptions.throwHttpErrors = false
Copy link
Member Author

Choose a reason for hiding this comment

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

Something to consider here:

By default got auto-retries failed requests. See
https:/sindresorhus/got#api
https:/nock/nock#common-issues
This behaviour is somewhat idiosyncratic I'm not 100% certain if this is desirable behaviour for us. I haven't done it, but we could turn this off by setting

gotOptions.retry = 0

here. Thoughts?

Copy link
Member Author

Choose a reason for hiding this comment

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

Having played around with it a bit more locally, I think this behaviour is going to be undesirable for us so I've turned it off in ece2f57

Copy link
Member

Choose a reason for hiding this comment

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

I wonder if there are cases with a flaky upstream service where auto-retry could potentially be helpful, though I think the constraints of our max response time are a bigger factor (i.e. better to return a failed/error badge that renders than take 5-6 seconds and have the badge fail to render).

There are some service tests that I'd love to have some auto-backoff and retry logic for, but that's entirely separate

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I think the magic 4 second limit is important here, at least in production

One of the nice things about got is as well as passing a number we can also pass retry an object including a calculateDelay function (which allows you to implement backoff) see https:/sindresorhus/got#retry so we could possibly find a way to make this a setting. We set retry to 0 in production/dev, but under test we set retry to an object which allows N retries with a backoff. I think we should leave this for now and come back to it once we've migrated everything to use got and dropped request.

Copy link
Member

Choose a reason for hiding this comment

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

One of the nice things about got is as well as passing a number we can also pass retry an object including a calculateDelay function (which allows you to implement backoff) see https:/sindresorhus/got#retry so we could possibly find a way to make this a setting. We set retry to 0 in production/dev, but under test we set retry to an object which allows N retries with a backoff. I think we should leave this for now and come back to it once we've migrated everything to use got and dropped request.

that sounds potentially glorious

gotOptions.retry = 0
try {
const resp = await gotWrapper(url, gotOptions)
return { res: resp, buffer: resp.body }
} catch (err) {
if (err instanceof got.CancelError) {
throw new InvalidResponse({
underlyingError: new Error('Maximum response size exceeded'),
})
}
throw new Inaccessible({ underlyingError: err })
}
}

function fetchFactory(fetchLimitBytes) {
const gotWithLimit = got.extend({
handlers: [
(options, next) => {
const promiseOrStream = next(options)
promiseOrStream.on('downloadProgress', progress => {
if (
progress.transferred > fetchLimitBytes &&
// just accept the file if we've already finished downloading
// the entire file before we went over the limit
progress.percent !== 1
) {
/*
TODO: we should be able to pass cancel() a message
https:/sindresorhus/got/blob/main/documentation/advanced-creation.md#examples
but by the time we catch it, err.message is just "Promise was canceled"
*/
promiseOrStream.cancel('Maximum response size exceeded')
}
})

return promiseOrStream
},
],
})

return sendRequest.bind(sendRequest, gotWithLimit)
}

module.exports = {
requestOptions2GotOptions,
fetchFactory,
}
89 changes: 89 additions & 0 deletions core/base-service/got.spec.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
'use strict'

const { expect } = require('chai')
const nock = require('nock')
const { requestOptions2GotOptions, fetchFactory } = require('./got')
const { Inaccessible, InvalidResponse } = require('./errors')

describe('requestOptions2GotOptions function', function () {
it('translates valid options', function () {
expect(
requestOptions2GotOptions({
body: 'body',
form: 'form',
headers: 'headers',
method: 'method',
url: 'url',
qs: 'qs',
gzip: 'gzip',
strictSSL: 'strictSSL',
auth: { user: 'user', pass: 'pass' },
})
).to.deep.equal({
body: 'body',
form: 'form',
headers: 'headers',
method: 'method',
url: 'url',
searchParams: 'qs',
decompress: 'gzip',
https: { rejectUnauthorized: 'strictSSL' },
username: 'user',
password: 'pass',
})
})

it('throws if unrecognised options are found', function () {
expect(() =>
requestOptions2GotOptions({ body: 'body', foobar: 'foobar' })
).to.throw(Error, 'Found unrecognised options foobar')
})
})

describe('got wrapper', function () {
it('should not throw an error if the response <= fetchLimitBytes', async function () {
nock('https://www.google.com')
.get('/foo/bar')
.once()
.reply(200, 'x'.repeat(100))
const sendRequest = fetchFactory(100)
const { res } = await sendRequest('https://www.google.com/foo/bar')
expect(res.statusCode).to.equal(200)
})

it('should throw an InvalidResponse error if the response is > fetchLimitBytes', async function () {
nock('https://www.google.com')
.get('/foo/bar')
.once()
.reply(200, 'x'.repeat(101))
const sendRequest = fetchFactory(100)
return expect(
sendRequest('https://www.google.com/foo/bar')
).to.be.rejectedWith(InvalidResponse, 'Maximum response size exceeded')
})

it('should throw an Inaccessible error if the request throws a (non-HTTP) error', async function () {
nock('https://www.google.com').get('/foo/bar').replyWithError('oh no')
const sendRequest = fetchFactory(1024)
return expect(
sendRequest('https://www.google.com/foo/bar')
).to.be.rejectedWith(Inaccessible, 'oh no')
})

it('should throw an Inaccessible error if the host can not be accessed', async function () {
this.timeout(5000)
nock.disableNetConnect()
const sendRequest = fetchFactory(1024)
return expect(
sendRequest('https://www.google.com/foo/bar')
).to.be.rejectedWith(
Inaccessible,
'Nock: Disallowed net connect for "www.google.com:443/foo/bar"'
)
})

afterEach(function () {
nock.cleanAll()
nock.enableNetConnect()
})
})
22 changes: 22 additions & 0 deletions core/server/server.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

const path = require('path')
const url = require('url')
const { bootstrap } = require('global-agent')
const { URL } = url
const cloudflareMiddleware = require('cloudflare-middleware')
const bytes = require('bytes')
Expand Down Expand Up @@ -421,6 +422,25 @@ class Server {
)
}

bootstrapAgent() {
/*
Bootstrap global agent.
This allows self-hosting users to configure a proxy with
HTTP_PROXY, HTTPS_PROXY, NO_PROXY variables
*/
if (!('GLOBAL_AGENT_ENVIRONMENT_VARIABLE_NAMESPACE' in process.env)) {
process.env.GLOBAL_AGENT_ENVIRONMENT_VARIABLE_NAMESPACE = ''
}

const proxyPrefix = process.env.GLOBAL_AGENT_ENVIRONMENT_VARIABLE_NAMESPACE
const HTTP_PROXY = process.env[`${proxyPrefix}HTTP_PROXY`] || null
const HTTPS_PROXY = process.env[`${proxyPrefix}HTTPS_PROXY`] || null

if (HTTP_PROXY || HTTPS_PROXY) {
bootstrap()
}
}

/**
* Start the HTTP server:
* Bootstrap Scoutcamp,
Expand All @@ -436,6 +456,8 @@ class Server {
requireCloudflare,
} = this.config.public

this.bootstrapAgent()

log(`Server is starting up: ${this.baseUrl}`)

const camp = (this.camp = Camp.create({
Expand Down
Loading