fix: parse the response body message to determine retry backoff (#249)

* fix: parse the response body message to determine retry backoff * address comments * address comments
googleapis · Jul 16, 2018 · 52003c8 · 52003c8
1 parent da64079
commit 52003c8
Show file tree

Hide file tree

Showing 4 changed files with 84 additions and 37 deletions.
diff --git a/package-lock.json b/package-lock.json
diff --git a/package.json b/package.json
@@ -26,7 +26,7 @@
   },
   "license": "Apache-2.0",
   "dependencies": {
-    "@google-cloud/common": "^0.20.0",
+    "@google-cloud/common": "^0.20.3",
     "bindings": "^1.2.1",
     "delay": "^3.0.0",
     "extend": "^3.0.1",

diff --git a/ts/src/profiler.ts b/ts/src/profiler.ts
@@ -81,16 +81,33 @@ function getServerResponseBackoff(response: http.IncomingMessage): number|
     undefined {
   // tslint:disable-next-line: no-any
   const body = (response as any).body;
-  if (body && body.error && body.error.details &&
-      Array.isArray(body.error.details)) {
-    for (const item of body.error.details) {
-      if (typeof item === 'object' && item.retryDelay &&
-          typeof item.retryDelay === 'string') {
-        const backoffMillis = parseDuration(item.retryDelay);
-        if (backoffMillis > 0) {
-          return backoffMillis;
-        }
-      }
+
+  // The response currently does not have field containing the server-specified
+  // backoff. As a workaround, response body's message is parsed to get the
+  // backoff.
+  // TODO (issue #250): Remove this workaround and get the retry delay from
+  // body.error.details.
+  if (body && body.message && typeof body.message === 'string') {
+    return parseBackoffDuration(body.message);
+  }
+  return undefined;
+}
+
+/**
+ * @return if the backoff duration can be parsed, then the backoff duration in
+ * ms, otherwise undefined.
+ *
+ * Public for testing.
+ */
+export function parseBackoffDuration(backoffMessage: string): number|undefined {
+  const backoffMessageRegex =
+      /action throttled, backoff for ((?:([0-9]+)h)?(?:([0-9]+)m)?([0-9.]+)s)$/;
+  const [, duration] =
+      backoffMessageRegex.exec(backoffMessage) || [undefined, undefined];
+  if (duration) {
+    const backoffMillis = parseDuration(duration);
+    if (backoffMillis > 0) {
+      return backoffMillis;
     }
   }
   return undefined;

diff --git a/ts/test/test-profiler.ts b/ts/test/test-profiler.ts
@@ -25,7 +25,7 @@ import * as zlib from 'zlib';
 
 import {perftools} from '../../proto/profile';
 import {ProfilerConfig} from '../src/config';
-import {Profiler, Retryer} from '../src/profiler';
+import {parseBackoffDuration, Profiler, Retryer} from '../src/profiler';
 import * as heapProfiler from '../src/profilers/heap-profiler';
 import {TimeProfiler} from '../src/profilers/time-profiler';
 
@@ -637,7 +637,9 @@ describe('Profiler', () => {
                            .onCall(0)
                            .callsArgWith(1, undefined, undefined, {
                              statusCode: 409,
-                             body: {error: {details: [{retryDelay: '50s'}]}}
+                             body: {
+                               message: 'action throttled, backoff for 50s',
+                             }
                            });
 
          const profiler = new Profiler(testConfig);
@@ -792,12 +794,13 @@ describe('Profiler', () => {
            duration: '10s',
            labels: {instance: config.instance}
          };
-         requestStub = sinon.stub(common.ServiceObject.prototype, 'request')
-                           .onCall(0)
-                           .callsArgWith(1, undefined, undefined, {
-                             statusCode: 409,
-                             body: {error: {details: [{retryDelay: '50s'}]}}
-                           });
+         requestStub =
+             sinon.stub(common.ServiceObject.prototype, 'request')
+                 .onCall(0)
+                 .callsArgWith(1, undefined, undefined, {
+                   statusCode: 409,
+                   body: {message: 'action throttled, backoff for 50s'}
+                 });
          const profiler = new Profiler(testConfig);
          profiler.timeProfiler = instance(mockTimeProfiler);
          const delayMillis = await profiler.collectProfile();
@@ -817,15 +820,15 @@ describe('Profiler', () => {
                            .onCall(0)
                            .callsArgWith(1, undefined, undefined, {
                              statusCode: 409,
-                             body: {error: {details: [{retryDelay: ''}]}}
+                             body: {message: 'some message'},
                            });
          const profiler = new Profiler(testConfig);
          profiler.timeProfiler = instance(mockTimeProfiler);
          const delayMillis = await profiler.collectProfile();
          assert.equal(500, delayMillis);
        });
     it('should return backoff limit, when server specified backoff is greater' +
-           'then backoff limit',
+           ' then backoff limit',
        async () => {
          const config = extend(true, {}, testConfig);
          const requestProfileResponseBody = {
@@ -839,7 +842,7 @@ describe('Profiler', () => {
                  .onCall(0)
                  .callsArgWith(1, undefined, undefined, {
                    statusCode: 409,
-                   body: {error: {details: [{retryDelay: '1000h'}]}}
+                   body: {message: 'action throttled, backoff for 1000h0s'},
                  });
          const profiler = new Profiler(testConfig);
          profiler.timeProfiler = instance(mockTimeProfiler);
@@ -871,4 +874,42 @@ describe('Profiler', () => {
          assert.equal(0, delayMillis);
        });
   });
+  describe('parseBackoffDuration', () => {
+    it('should return undefined when no duration specified', () => {
+      assert.equal(undefined, parseBackoffDuration(''));
+    });
+    it('should parse backoff with minutes and seconds specified', () => {
+      assert.equal(
+          62000, parseBackoffDuration('action throttled, backoff for 1m2s'));
+    });
+    it('should parse backoff with fraction of second', () => {
+      assert.equal(
+          2500, parseBackoffDuration('action throttled, backoff for 2.5s'));
+    });
+    it('should parse backoff with minutes and seconds, including fraction of second',
+       () => {
+         assert.equal(
+             62500,
+             parseBackoffDuration('action throttled, backoff for 1m2.5s'));
+       });
+    it('should parse backoff with hours and seconds', () => {
+      assert.equal(
+          3602500,
+          parseBackoffDuration('action throttled, backoff for 1h2.5s'));
+    });
+    it('should parse backoff with hours, minutes, and seconds', () => {
+      assert.equal(
+          3662500,
+          parseBackoffDuration('action throttled, backoff for 1h1m2.5s'));
+    });
+    it('should parse return undefined for unexpected backoff time string format',
+       () => {
+         assert.equal(
+             undefined,
+             parseBackoffDuration('action throttled, backoff for  1m2+s'));
+       });
+    it('should parse return undefined for unexpected string format', () => {
+      assert.equal(undefined, parseBackoffDuration('time 1m2s'));
+    });
+  });
 });