feat: Add exponential and server-mediated backoff on retries #56

nolanmar511 · 2017-11-16T02:27:18Z

Fixes #43 and #44

codecov · 2017-11-16T02:33:38Z

Codecov Report

Merging #56 into master will increase coverage by 3.97%.
The diff coverage is 97.67%.

@@            Coverage Diff             @@
##           master      #56      +/-   ##
==========================================
+ Coverage   92.95%   96.92%   +3.97%     
==========================================
  Files           6        6              
  Lines         298      325      +27     
  Branches       54       59       +5     
==========================================
+ Hits          277      315      +38     
+ Misses         21       10      -11

Impacted Files	Coverage Δ
ts/src/config.ts	`100% <100%> (ø)`	⬆️
ts/src/profiler.ts	`96.07% <97.61%> (+9.46%)`	⬆️

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 8fe22e2...5c79415. Read the comment docs.

ts/src/config.ts

+  // by the server response, then profiler will wait this long before asking
+  // server to create a profile again. After a successful profile creation,
+  // the backoff will be reset to initialExpBackoffMillis.
+  initialExpBackoffMillis?: number;


ts/src/config.ts

@@ -108,5 +125,8 @@ export const defaultConfig = {
  timeIntervalMicros: 1000,
  heapIntervalBytes: 512 * 1024,
  heapMaxStackDepth: 64,
-  backoffMillis: 5 * 60 * 1000
+  initialExpBackoffMillis: 1000,
+  maxExpBackoffMillis: 60 * 60 * 1000,


ts/src/config.ts

+  initialExpBackoffMillis: 1000,
+  maxExpBackoffMillis: 60 * 60 * 1000,
+  expBackoffMultiplier: 1.3,
+  backoffLimitMillis: 7 * 24 * 60 * 60 * 1000,  // 7 days


ts/src/profiler.ts

+    super(message.toString());
+    this.statusCode = response.statusCode;
+    if (isServerBackoffResponse(response)) {
+      this.backoffMillis = parseDuration(response.body.details.retryDelay);


ts/src/profiler.ts

+  }
+  getBackoff(): number {
+    const curBackoff = this.nextBackoff;
+    this.nextBackoff = this.backoffMultiplier * this.nextBackoff;


ts/src/profiler.ts

@@ -201,29 +254,37 @@ export class Profiler extends common.ServiceObject {
    const delayMillis = await this.collectProfile();

    // Schedule the next profile.
-    setTimeout(this.runLoop.bind(this), delayMillis).unref();
+    if (typeof delayMillis === 'number') {


ts/src/profiler.ts

@@ -126,6 +137,48 @@ async function profileBytes(p: perftools.profiles.IProfile): Promise<string> {
 }

 /**
+ * Error which also indicates how long backoff should be in response to the


ts/src/profiler.ts

+  backoffMillis?: number;
+  statusCode: number;
+  constructor(response: http.ServerResponse) {
+    let message: number|string = response.statusCode;


ts/src/profiler.ts

   */
-  async collectProfile(): Promise<number> {
+  async collectProfile(retryer?: Retryer): Promise<number|Retryer> {


ts/src/profiler.ts

    let prof: RequestProfile;
    try {
      prof = await this.createProfile();
    } catch (err) {
      this.logger.error(
          `Error requesting profile type to be collected: ${err}.`);
-      return this.config.backoffMillis;
+      if (err.backoffMillis !== undefined) {


ts/src/profiler.ts

-      throw new Error(`Profile not valid: ${body}.`);
-    }
-    return body;
+    return await new Promise<RequestProfile>((resolve, reject) => {


ts/src/profiler.ts

+ */
+class BackoffResponseError extends Error {
+  // tslint:disable-next-line: variable-name
+  __proto__: Error;


ts/src/profiler.ts

+export class Retryer {
+  private nextMaxBackoffMillis: number;
+  constructor(
+      private initialBackoffMillis: number, private maxBackoffMillis: number,


ts/test/test-init-config.ts

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


ts/test/test-profiler.ts

+         const profiler = new Profiler(testConfig);
+         profiler.timeProfiler = instance(mockTimeProfiler);
+         const delayMillis = await profiler.collectProfile();
+         assert.equal(7 * 24 * 60 * 60 * 1000, delayMillis);


googlebot · 2017-11-20T17:21:18Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this State. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

ofrobots · 2017-11-20T17:27:13Z

@nolanmar511, I am not sure why the bot is unhappy. Did you use a different computer, or edit your git config perhaps?

nolanmar511 · 2017-11-20T17:34:06Z

I'm pretty sure the bot is unhappy after I merged.
Some of the commits are commits that were merged in.

nolanmar511 · 2017-11-20T18:38:32Z

With the changes from #60 merged in, tests here started to fail and I couldn't figure out why. I filed #68 with more details on this.
This PR reverts #60.

aalexand · 2017-11-20T20:25:59Z

Can you revert #60 separately from this PR?

…

On Mon, Nov 20, 2017 at 10:38 AM, Margaret Nolan ***@***.***> wrote: With the changes from #60 <#60> merged in, tests here started to fail and I couldn't figure out why. I filed #68 <#68> with more details on this. This PR reverts #60 <#60>. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#56 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABctwOvXFXuWDuAyboB9yIZk577s44b_ks5s4ccwgaJpZM4Qf3sX> .

-- Alex

aalexand · 2017-11-20T20:26:23Z

Also, with a PR in flight, never merge, rebase instead. On Mon, Nov 20, 2017 at 12:25 PM, Alexey Alexandrov <aalexand@google.com> wrote:

…

Can you revert #60 separately from this PR? On Mon, Nov 20, 2017 at 10:38 AM, Margaret Nolan ***@***.*** > wrote: > With the changes from #60 > <#60> > merged in, tests here started to fail and I couldn't figure out why. I > filed #68 > <#68> > with more details on this. > This PR reverts #60 > <#60>. > > — > You are receiving this because you commented. > Reply to this email directly, view it on GitHub > <#56 (comment)>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/ABctwOvXFXuWDuAyboB9yIZk577s44b_ks5s4ccwgaJpZM4Qf3sX> > . > -- Alex

-- Alex

nolanmar511 · 2017-11-21T17:23:12Z

PTAL

nolanmar511 · 2017-11-21T22:51:17Z

Closed, because I could not get the CLA approval again.

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Nov 16, 2017

aalexand reviewed Nov 16, 2017

View reviewed changes

ofrobots reviewed Nov 16, 2017

View reviewed changes

googlebot added cla: no This human has *not* signed the Contributor License Agreement. and removed cla: yes This human has signed the Contributor License Agreement. labels Nov 20, 2017

nolanmar511 force-pushed the retry-request branch from 1f722d3 to 68b0b08 Compare November 20, 2017 17:24

nolanmar511 mentioned this pull request Nov 20, 2017

Add back source-map-support for better stack traces #68

Closed

nolanmar511 force-pushed the retry-request branch from b432afd to ddbafc4 Compare November 21, 2017 17:16

nolanmar511 force-pushed the retry-request branch 2 times, most recently from 69ebe62 to 5ea0f63 Compare November 21, 2017 21:18

feat: Add exponential and server-mediated backoff on retries

5c79415

nolanmar511 force-pushed the retry-request branch from 5ea0f63 to 5c79415 Compare November 21, 2017 22:42

nolanmar511 closed this Nov 21, 2017

nolanmar511 reopened this Nov 21, 2017

jmdobry added the in progress label Nov 21, 2017

nolanmar511 closed this Nov 21, 2017

nolanmar511 deleted the retry-request branch November 21, 2017 22:48

nolanmar511 restored the retry-request branch November 21, 2017 22:48

nolanmar511 mentioned this pull request Nov 21, 2017

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

Merged

feat: Add exponential and server-mediated backoff on retries #56

feat: Add exponential and server-mediated backoff on retries #56

Conversation

nolanmar511 commented Nov 16, 2017 • edited Loading

codecov bot commented Nov 16, 2017 • edited Loading

Codecov Report

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

googlebot commented Nov 20, 2017

ofrobots commented Nov 20, 2017

nolanmar511 commented Nov 20, 2017

nolanmar511 commented Nov 20, 2017

aalexand commented Nov 20, 2017 via email

aalexand commented Nov 20, 2017 via email

nolanmar511 commented Nov 21, 2017

nolanmar511 commented Nov 21, 2017

nolanmar511 commented Nov 16, 2017 •

edited

Loading

codecov bot commented Nov 16, 2017 •

edited

Loading