Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

feat: add exponential and server-mediated backoff on retries #75

Merged
merged 29 commits into from
Dec 12, 2017

Conversation

nolanmar511
Copy link
Contributor

Fixes #43 and #44.
See #56 for previous discussion on this topic.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Nov 21, 2017
@codecov
Copy link

codecov bot commented Nov 21, 2017

Codecov Report

Merging #75 into master will increase coverage by 1.42%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #75      +/-   ##
==========================================
+ Coverage    95.4%   96.83%   +1.42%     
==========================================
  Files           6        6              
  Lines         305      348      +43     
  Branches       51       56       +5     
==========================================
+ Hits          291      337      +46     
+ Misses         14       11       -3
Impacted Files Coverage Δ
ts/src/config.ts 100% <100%> (ø) ⬆️
ts/src/profiler.ts 98.79% <100%> (+2.82%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 289c105...1392993. Read the comment docs.

ts/src/config.ts Outdated
// fails.
backoffMillis?: number;
// On first error during profile creation, if the backoff is not specified
// by the server response, then profiler will between 0 and

This comment was marked as spam.

This comment was marked as spam.

ts/src/config.ts Outdated
maxBackoffMillis?: number;

// On each consecutive error in profile creation, the maximum backoff will
// increase by this factor. The backoff will be random value selected

This comment was marked as spam.

This comment was marked as spam.

ts/src/config.ts Outdated
// again.
maxBackoffMillis?: number;

// On each consecutive error in profile creation, the maximum backoff will

This comment was marked as spam.

This comment was marked as spam.

ts/src/config.ts Outdated

// On each consecutive error in profile creation, the maximum backoff will
// increase by this factor. The backoff will be random value selected
// from a uniform distribution between 0 and the maximum backoff.

This comment was marked as spam.

This comment was marked as spam.

ts/src/config.ts Outdated
backoffMultiplier?: number;

// Server-specified backoffs will be capped at backoffLimitMillis.
backoffLimitMillis?: number;

This comment was marked as spam.

This comment was marked as spam.

* createProfile() should be retried when response indicates this request
* should be retried or with exponential backoff (up to one hour) if the
* response does not indicate when to retry this request.
* collecting another profile, or retryer specifying exponential backoff.
*/
async collectProfile(): Promise<number> {
let prof: RequestProfile;
try {
prof = await this.createProfile();
} catch (err) {
this.logger.error(`Failed to create profile: ${err}`);

This comment was marked as spam.

This comment was marked as spam.

throw new Error(response.statusMessage);
}
if (err) {
reject(err);

This comment was marked as spam.

This comment was marked as spam.

return new Promise<RequestProfile>((resolve, reject) => {
this.request(
options,
(err: Error, prof: object, response: http.ServerResponse) => {

This comment was marked as spam.

This comment was marked as spam.

resolve(prof);
return;
}
throw new Error(`Profile not valid: ${JSON.stringify(prof)}.`);

This comment was marked as spam.

This comment was marked as spam.

initialBackoffMillis: 1000,
maxBackoffMillis: 60 * 60 * 1000,
backoffMultiplier: 1.3,
backoffLimitMillis: 7 * 24 * 60 * 60 * 1000

This comment was marked as spam.

This comment was marked as spam.

@nolanmar511
Copy link
Contributor Author

PTAL

@nolanmar511 nolanmar511 changed the title feat: Add exponential and server-mediated backoff on retries feat: add exponential and server-mediated backoff on retries Nov 28, 2017
Copy link
Contributor

@aalexand aalexand left a comment

Choose a reason for hiding this comment

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

Some nits, mostly looks good. @ofrobots please take a look as well.

ts/src/config.ts Outdated
// by the server response, then profiler will wait between 0 and
// initialBackoffMillis before asking the server to create a profile again.
// After a successful profile creation, the backoff envelope will be reset to
// initialExpBackoffMillis.

This comment was marked as spam.

This comment was marked as spam.

ts/src/config.ts Outdated
// If the backoff is not specified by the server response, then profiler will
// wait at most expBackoffMillisCap before asking server to create a profile
// again.
expBackoffMillisCap?: number;

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

ts/src/config.ts Outdated
// The backoff is capped here because setTimeout (which is used to control
// when next profile is collected) will run immediately if the backoff is
// to large.
serverBackoffMillisCap?: number;

This comment was marked as spam.

This comment was marked as spam.

ts/src/config.ts Outdated
initialBackoffMillis: 1000,
expBackoffMillisCap: parseDuration('1h'),
backoffMultiplier: 1.3,
serverBackoffMillisCap: parseDuration('7d'), // 7 days

This comment was marked as spam.

This comment was marked as spam.

@@ -120,6 +142,69 @@ async function profileBytes(p: perftools.profiles.IProfile): Promise<string> {
return gzBuf.toString('base64');
}

/**
* Error constructed from http server response which indicates backoff.

This comment was marked as spam.

This comment was marked as spam.

export class Retryer {
private nextBackoffMillis: number;
constructor(
readonly initialBackoffMillis: number, readonly maxBackoffMillis: number,

This comment was marked as spam.

This comment was marked as spam.

ts/src/config.ts Outdated
// Server-specified backoffs will be capped at serverBackoffCapMillis.
// The backoff is capped here because setTimeout (which is used to control
// when next profile is collected) will run immediately if the backoff is
// to large.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

import {ProfilerConfig} from './config';
import {HeapProfiler} from './profilers/heap-profiler';
import {TimeProfiler} from './profilers/time-profiler';

export const common: Common = require('@google-cloud/common');
const parseDuration: (str: string) => number = require('parse-duration');
const msToStr: (ms: number) => string = require('pretty-ms');

This comment was marked as spam.

This comment was marked as spam.

const item = response.body.error.details[i];
if (typeof item === 'object' && item.retryDelay &&
typeof item.retryDelay === 'string') {
return parseDuration(item.retryDelay);

This comment was marked as spam.

This comment was marked as spam.

*/
function isBackoffResponseError(err: Error): err is BackoffResponseError {
// tslint:disable-next-line: no-any
return typeof (err as any).backoffMillis === 'number';

This comment was marked as spam.

This comment was marked as spam.

@@ -97,7 +119,7 @@ function isRequestProfile(prof: any): prof is RequestProfile {
}

/**
* @return true if response has statusCode.
* @return true iff response has statusCode.
*/
// tslint:disable-next-line: no-any
function hasHttpStatusCode(response: any):

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

try {
await this.profileAndUpload(prof);
} catch (err) {
this.logger.error(`Failed to collect and upload profile: ${err}`);
this.logger.warn(`Failed to collect and upload profile: ${err}`);

This comment was marked as spam.

This comment was marked as spam.

@nolanmar511
Copy link
Contributor Author

PTAL

ts/src/config.ts Outdated
initialBackoffMillis: 1000,
backoffCapMillis: parseDuration('1h'),
backoffMultiplier: 1.3,
serverBackoffCapMillis: 2147483647

This comment was marked as spam.

@nolanmar511
Copy link
Contributor Author

@ofrobots -- any additional comments?

@aalexand
Copy link
Contributor

aalexand commented Dec 3, 2017

@ofrobots ping

package.json Outdated
@@ -26,13 +26,15 @@
"license": "Apache-2.0",
"dependencies": {
"@google-cloud/common": "^0.15.0",
"@types/pretty-ms": "^3.0.0",

This comment was marked as spam.

This comment was marked as spam.

* that backoff is greater than 0. Otherwise returns undefined.
*/
// tslint:disable-next-line: no-any
function getServerResponseBackoff(response: any): number|undefined {

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

*/
// tslint:disable-next-line: no-any
function getServerResponseBackoff(response: any): number|undefined {
if (response && response.body && response.body.error &&

This comment was marked as spam.

This comment was marked as spam.

function getServerResponseBackoff(response: any): number|undefined {
if (response && response.body && response.body.error &&
response.body.error.details &&
response.body.error.details instanceof Array) {

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

if (response && response.body && response.body.error &&
response.body.error.details &&
response.body.error.details instanceof Array) {
for (let i = 0; i < response.body.error.details.length; i++) {

This comment was marked as spam.

This comment was marked as spam.

for (let i = 0; i < response.body.error.details.length; i++) {
const item = response.body.error.details[i];
if (typeof item === 'object' && item.retryDelay &&
typeof item.retryDelay === 'string') {

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@@ -97,7 +119,7 @@ function isRequestProfile(prof: any): prof is RequestProfile {
}

/**
* @return true if response has statusCode.
* @return true iff response has statusCode.
*/
// tslint:disable-next-line: no-any
function hasHttpStatusCode(response: any):

This comment was marked as spam.

@nolanmar511
Copy link
Contributor Author

PTAL

if (!hasHttpStatusCode(response)) {
this.logger.debug(
'Server response missing status information when attempting to upload profile.');
if (!(response instanceof http.ServerResponse)) {

This comment was marked as spam.

This comment was marked as spam.

function getServerResponseBackoff(response: any): number|undefined {
if (response && response.body && response.body.error &&
response.body.error.details &&
response.body.error.details instanceof Array) {

This comment was marked as spam.

for (let i = 0; i < response.body.error.details.length; i++) {
const item = response.body.error.details[i];
if (typeof item === 'object' && item.retryDelay &&
typeof item.retryDelay === 'string') {

This comment was marked as spam.

@nolanmar511
Copy link
Contributor Author

nolanmar511 commented Dec 8, 2017

I have stopped using instanceof.
I have manually tested this PR. And I will be working on an e2e test (this is my next priority). I will also look at using the types from #87 going forward.

(Also, apologies for adding a comment instead of replying to comments; there were some comments I don't seem to be able to reply directly to).

@nolanmar511
Copy link
Contributor Author

PTAL

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
cla: yes This human has signed the Contributor License Agreement. in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants