[2.0] Switch apollo-engine-reporting to use fetch instead of the Node…

… request module (#1274) * Remove Node dependencies from package.json * Replace Node request with fetch * Wrap fetch in @zeit/fetch-retry, convert to async/await, and fix types * Use async-retry directly because @zeit/fetch-retry doesn't support Node 6 * bump server-env version in graphql-extensions * explicitly include factor in async-retry * change apollo-server-env to rc.5
apollographql · Jun 29, 2018 · cbb5bf3 · cbb5bf3
1 parent 59ca96c
commit cbb5bf3
Show file tree

Hide file tree

Showing 6 changed files with 78 additions and 85 deletions.
diff --git a/packages/apollo-engine-reporting/package.json b/packages/apollo-engine-reporting/package.json
@@ -21,23 +21,21 @@
     "node": ">=6.0"
   },
   "dependencies": {
-    "apollo-server-env": "^2.0.0-rc.3",
     "apollo-engine-reporting-protobuf": "0.0.0-beta.7",
+    "apollo-server-env": "^2.0.0-rc.5",
+    "async-retry": "^1.2.1",
     "graphql-extensions": "^0.1.0-beta.15",
-    "lodash": "^4.17.10",
-    "requestretry": "^1.13.0"
+    "lodash": "^4.17.10"
   },
   "devDependencies": {
+    "@types/async-retry": "^1.2.1",
     "@types/graphql": "^0.13.1",
     "@types/jest": "^23.1.2",
     "@types/lodash": "^4.14.110",
-    "@types/node-fetch": "^2.1.1",
-    "@types/requestretry": "^1.12.3",
     "graphql": "^0.13.2",
     "graphql-tag": "^2.9.2",
     "graphql-tools": "^3.0.4",
     "jest": "^23.2.0",
-    "node-fetch": "^2.1.2",
     "ts-jest": "^22.4.6",
     "tslint": "^5.10.0"
   },

diff --git a/packages/apollo-engine-reporting/src/agent.ts b/packages/apollo-engine-reporting/src/agent.ts
@@ -1,6 +1,5 @@
 import * as os from 'os';
 import { gzip } from 'zlib';
-import * as request from 'requestretry';
 import { DocumentNode } from 'graphql';
 import {
   FullTracesReport,
@@ -9,6 +8,9 @@ import {
   Trace,
 } from 'apollo-engine-reporting-protobuf';
 
+import { fetch, Response } from 'apollo-server-env';
+import * as retry from 'async-retry';
+
 import { EngineReportingExtension } from './extension';
 
 // Override the generated protobuf Traces.encode function so that it will look
@@ -170,101 +172,94 @@ export class EngineReportingAgent<TContext = any> {
     }
   }
 
-  public sendReport(): Promise<void> {
+  public async sendReport(): Promise<void> {
     const report = this.report;
     this.resetReport();
 
     if (Object.keys(report.tracesPerQuery).length === 0) {
-      return Promise.resolve();
+      return;
     }
 
     // Send traces asynchronously, so that (eg) addTrace inside a resolver
     // doesn't block on it.
-    return Promise.resolve()
-      .then(() => {
-        if (this.options.debugPrintReports) {
-          // tslint:disable-next-line no-console
-          console.log(
-            `Engine sending report: ${JSON.stringify(report.toJSON())}`,
-          );
-        }
+    await Promise.resolve();
+
+    if (this.options.debugPrintReports) {
+      // tslint:disable-next-line no-console
+      console.log(`Engine sending report: ${JSON.stringify(report.toJSON())}`);
+    }
+
+    const protobufError = FullTracesReport.verify(report);
+    if (protobufError) {
+      throw new Error(`Error encoding report: ${protobufError}`);
+    }
+    const message = FullTracesReport.encode(report).finish();
 
-        const protobufError = FullTracesReport.verify(report);
-        if (protobufError) {
-          throw new Error(`Error encoding report: ${protobufError}`);
+    const compressed = await new Promise<Buffer>((resolve, reject) => {
+      // The protobuf library gives us a Uint8Array. Node 8's zlib lets us
+      // pass it directly; convert for the sake of Node 6. (No support right
+      // now for Node 4, which lacks Buffer.from.)
+      const messageBuffer = Buffer.from(
+        message.buffer as ArrayBuffer,
+        message.byteOffset,
+        message.byteLength,
+      );
+      gzip(messageBuffer, (err, compressed) => {
+        if (err) {
+          reject(err);
+        } else {
+          resolve(compressed);
         }
-        const message = FullTracesReport.encode(report).finish();
+      });
+    });
 
-        return new Promise((resolve, reject) => {
-          // The protobuf library gives us a Uint8Array. Node 8's zlib lets us
-          // pass it directly; convert for the sake of Node 6. (No support right
-          // now for Node 4, which lacks Buffer.from.)
-          const messageBuffer = Buffer.from(
-            message.buffer as ArrayBuffer,
-            message.byteOffset,
-            message.byteLength,
-          );
-          gzip(messageBuffer, (err, compressed) => {
-            if (err) {
-              reject(err);
-            } else {
-              resolve(compressed);
-            }
-          });
-        });
-      })
-      .then(compressed => {
-        // Grab this here because the delayStrategy function has a different 'this'.
-        const minimumRetryDelayMs = this.options.minimumRetryDelayMs || 100;
+    const endpointUrl =
+      (this.options.endpointUrl || 'https://engine-report.apollodata.com') +
+      '/api/ingress/traces';
 
-        // note: retryrequest has built-in Promise support, unlike the base 'request'.
-        return (request({
-          url:
-            (this.options.endpointUrl ||
-              'https://engine-report.apollodata.com') + '/api/ingress/traces',
+    // Wrap fetch with async-retry for automatic retrying
+    const response: Response = await retry(
+      // Retry on network errors and 5xx HTTP
+      // responses.
+      async () => {
+        const response = await fetch(endpointUrl, {
           method: 'POST',
           headers: {
             'user-agent': 'apollo-engine-reporting',
             'x-api-key': this.apiKey,
             'content-encoding': 'gzip',
           },
           body: compressed,
-          // By default, retryrequest will retry on network errors and 5xx HTTP
-          // responses.
-          maxAttempts: this.options.maxAttempts || 5,
-          // Note: use a non-arrow function as this API gives us useful information
-          // on 'this', and use an 'as any' because the type definitions don't know
-          // about the function version of this parameter.
-          delayStrategy: function() {
-            return Math.pow(minimumRetryDelayMs * 2, this.attempts);
-          },
-          // XXX Back in Optics, we had an explicit proxyUrl option for corporate
-          //     proxies. I was never clear on why `request`'s handling of the
-          //     standard env vars wasn't good enough (see
-          //     https://github.com/apollographql/optics-agent-js/pull/70#discussion_r89374066).
-          //     We may have to add it here.
-
-          // Include 'as any's because @types/requestretry doesn't understand the
-          // promise API or delayStrategy.
-        } as any) as any).catch((err: Error) => {
-          throw new Error(`Error sending report to Engine servers: ${err}`);
         });
-      })
-      .then(response => {
-        if (response.statusCode < 200 || response.statusCode >= 300) {
-          // Note that we don't expect to see a 3xx here because request follows
-          // redirects.
-          throw new Error(
-            `Error sending report to Engine servers (HTTP status ${
-              response.statusCode
-            }): ${response.body}`,
-          );
-        }
-        if (this.options.debugPrintReports) {
-          // tslint:disable-next-line no-console
-          console.log(`Engine report: status ${response.statusCode}`);
+
+        if (response.status >= 500 && response.status < 600) {
+          throw new Error(`${response.status}: ${response.statusText}`);
+        } else {
+          return response;
         }
-      });
+      },
+      {
+        retries: this.options.maxAttempts || 5,
+        minTimeout: this.options.minimumRetryDelayMs || 100,
+        factor: 2,
+      },
+    ).catch((err: Error) => {
+      throw new Error(`Error sending report to Engine servers: ${err}`);
+    });
+
+    if (response.status < 200 || response.status >= 300) {
+      // Note that we don't expect to see a 3xx here because request follows
+      // redirects.
+      throw new Error(
+        `Error sending report to Engine servers (HTTP status ${
+          response.status
+        }): ${await response.text()}`,
+      );
+    }
+    if (this.options.debugPrintReports) {
+      // tslint:disable-next-line no-console
+      console.log(`Engine report: status ${response.status}`);
+    }
   }
 
   // Stop prevents reports from being sent automatically due to time or buffer

diff --git a/packages/apollo-server-env/src/fetch.d.ts b/packages/apollo-server-env/src/fetch.d.ts
@@ -98,4 +98,4 @@ export interface ResponseInit {
   statusText?: string;
 }
 
-export type BodyInit = ArrayBuffer | string;
+export type BodyInit = ArrayBuffer | ArrayBufferView | string;
diff --git a/packages/apollo-server-env/tsconfig.json b/packages/apollo-server-env/tsconfig.json
@@ -15,5 +15,5 @@
   },
   "include": ["src/**/*"],
   "exclude": ["node_modules", "**/__tests__/*", "**/__mocks__/*"],
-  "types": ["node"]
+  "types": []
 }
diff --git a/packages/apollo-server-integration-testsuite/package.json b/packages/apollo-server-integration-testsuite/package.json
@@ -32,7 +32,7 @@
     "apollo-link": "^1.2.2",
     "apollo-link-http": "^1.5.4",
     "apollo-link-persisted-queries": "^0.2.1",
-    "apollo-server-env": "^2.0.0-rc.3",
+    "apollo-server-env": "^2.0.0-rc.5",
     "body-parser": "^1.18.3",
     "graphql-extensions": "^0.1.0-beta.15",
     "graphql-subscriptions": "^0.5.8",

diff --git a/packages/graphql-extensions/package.json b/packages/graphql-extensions/package.json
@@ -22,7 +22,7 @@
     "node": ">=6.0"
   },
   "dependencies": {
-    "apollo-server-env": "^2.0.0-rc.3",
+    "apollo-server-env": "^2.0.0-rc.5",
     "core-js": "^2.5.7",
     "source-map-support": "^0.5.6"
   },