fix: Improve remote evaluation fetch retry logic (#40)

amplitude · Jan 31, 2024 · 525cf0f · 525cf0f
1 parent ed1403e
commit 525cf0f
Show file tree

Hide file tree

Showing 3 changed files with 62 additions and 6 deletions.
diff --git a/packages/node/package.json b/packages/node/package.json
@@ -32,6 +32,6 @@
   "dependencies": {
     "@amplitude/analytics-node": "^1.3.4",
     "@amplitude/analytics-types": "^1.3.1",
-    "@amplitude/experiment-core": "^0.7.1"
+    "@amplitude/experiment-core": "^0.7.2"
   }
 }
diff --git a/packages/node/src/remote/client.ts b/packages/node/src/remote/client.ts
@@ -1,4 +1,8 @@
-import { EvaluationApi, SdkEvaluationApi } from '@amplitude/experiment-core';
+import {
+  EvaluationApi,
+  FetchError,
+  SdkEvaluationApi,
+} from '@amplitude/experiment-core';
 
 import { version as PACKAGE_VERSION } from '../../gen/version';
 import { FetchHttpClient, WrapperClient } from '../transport/http';
@@ -65,10 +69,12 @@ export class RemoteEvaluationClient {
       return await this.doFetch(user, this.config.fetchTimeoutMillis, options);
     } catch (e) {
       console.error('[Experiment] Fetch failed: ', e);
-      try {
-        return await this.retryFetch(user, options);
-      } catch (e) {
-        console.error(e);
+      if (this.shouldRetryFetch(e)) {
+        try {
+          return await this.retryFetch(user, options);
+        } catch (e) {
+          console.error(e);
+        }
       }
       throw e;
     }
@@ -155,6 +161,13 @@ export class RemoteEvaluationClient {
       console.debug(message, ...optionalParams);
     }
   }
+
+  private shouldRetryFetch(e: Error): boolean {
+    if (e instanceof FetchError) {
+      return e.statusCode < 400 || e.statusCode >= 500 || e.statusCode === 429;
+    }
+    return true;
+  }
 }
 
 /**

diff --git a/packages/node/test/remote/client.test.ts b/packages/node/test/remote/client.test.ts
@@ -1,3 +1,4 @@
+import { FetchError } from '@amplitude/experiment-core';
 import { RemoteEvaluationClient } from 'src/remote/client';
 import { ExperimentUser } from 'src/types/user';
 
@@ -56,3 +57,45 @@ test('ExperimentClient.fetch, v2 off returns default variant', async () => {
   expect(variant.value).toBeUndefined();
   expect(variant.metadata.default).toEqual(true);
 });
+
+describe('ExperimentClient.fetch, retry with different response codes', () => {
+  beforeEach(() => {
+    jest.clearAllMocks();
+  });
+  test.each([
+    [300, 'Fetch Exception 300', 1],
+    [400, 'Fetch Exception 400', 0],
+    [429, 'Fetch Exception 429', 1],
+    [500, 'Fetch Exception 500', 1],
+    [0, 'Other Exception', 1],
+  ])(
+    'responseCode=%p, errorMessage=%p, retryCalled=%p',
+    async (responseCode, errorMessage, retryCalled) => {
+      const client = new RemoteEvaluationClient(API_KEY, { fetchRetries: 1 });
+
+      jest
+        // eslint-disable-next-line @typescript-eslint/no-explicit-any
+        .spyOn(RemoteEvaluationClient.prototype as any, 'doFetch')
+        .mockImplementation(async () => {
+          return new Promise<RemoteEvaluationClient>((_resolve, reject) => {
+            if (responseCode === 0) {
+              reject(new Error(errorMessage));
+            } else {
+              reject(new FetchError(responseCode, errorMessage));
+            }
+          });
+        });
+      const retryMock = jest.spyOn(
+        // eslint-disable-next-line @typescript-eslint/no-explicit-any
+        RemoteEvaluationClient.prototype as any,
+        'retryFetch',
+      );
+      try {
+        await client.fetch({ user_id: 'test_user' });
+      } catch (e) {
+        // catch error
+      }
+      expect(retryMock).toHaveBeenCalledTimes(retryCalled);
+    },
+  );
+});