apollo-engine-reporting: fix maxAttempts parameter, log 5xx body

CHANGELOG.md

@@ -7,6 +7,8 @@ The version headers in this history reflect the versions of Apollo Server itself
 > The changes noted within this `vNEXT` section have not been released yet.  New PRs and commits which introduce changes should include an entry in this `vNEXT` section as part of their development.  When a release is being prepared, a new header will be (manually) created below and the the appropriate changes within that release will be moved into the new section.
 
 - `apollo-server-core`: Make `formatError` available to subscriptions in the same spirit as the existing `formatResponse`. [PR #2942](https://github.com/apollographql/apollo-server/pull/2942)
+- `apollo-engine-reporting`: The behavior of the `engine.maxAttempts` parameter previously did not match its documentation. It is documented as being the max number of attempts *including* the initial attempt, but until this release it was actually the number of retries *excluding* the initial attempt. The behavior has been changed to match the documentation (and the literal reading of the option name). [PR #3218](https://github.com/apollographql/apollo-server/pull/3218)
+- `apollo-engine-reporting`: When sending the report fails with a server-side 5xx error, include the full error from the server in the logs. [PR #3218](https://github.com/apollographql/apollo-server/pull/3218)
 
 ### v2.9.0
 

packages/apollo-engine-reporting/src/agent.ts

@@ -403,27 +403,32 @@ export class EngineReportingAgent<TContext = any> {
         });
 
         if (curResponse.status >= 500 && curResponse.status < 600) {
-          throw new Error(`${curResponse.status}: ${curResponse.statusText}`);
+          throw new Error(
+            `HTTP status ${curResponse.status}, ${(await curResponse.text()) ||
+              '(no body)'}`,
+          );
         } else {
           return curResponse;
         }
       },
       {
-        retries: this.options.maxAttempts || 5,
+        retries: (this.options.maxAttempts || 5) - 1,
         minTimeout: this.options.minimumRetryDelayMs || 100,
         factor: 2,
       },
     ).catch((err: Error) => {
-      throw new Error(`Error sending report to Apollo Engine servers: ${err}`);
+      throw new Error(
+        `Error sending report to Apollo Engine servers: ${err.message}`,
+      );
     });
 
     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 Apollo Engine servers (HTTP status ${
+        `Error sending report to Apollo Engine servers: HTTP status ${
           response.status
-        }): ${await response.text()}`,
+        }, ${(await response.text()) || '(no body)'}`,
       );
     }
     if (this.options.debugPrintReports) {

packages/apollo-server-integration-testsuite/src/ApolloServer.ts

@@ -1945,70 +1945,121 @@ export function testApolloServer<AS extends ApolloServerBase>(
     });
 
     describe('apollo-engine-reporting', () => {
-      it('graphql server functions even when Apollo servers are down', async () => {
-        let return502Resolve: () => void;
-        const return502Promise = new Promise(
-          resolve => (return502Resolve = resolve),
-        );
-        const fakeEngineServer = http.createServer(async (_, res) => {
-          await return502Promise;
-          res.writeHead(502);
-          res.end();
-        });
-        await new Promise(resolve => {
-          fakeEngineServer.listen(0, '127.0.0.1', () => {
-            resolve();
+      describe('graphql server functions even when Apollo servers are down', () => {
+        async function testWithStatus(
+          status: number,
+          expectedRequestCount: number,
+        ) {
+          const networkError = status === 0;
+
+          let writeResponseResolve: () => void;
+          const writeResponsePromise = new Promise(
+            resolve => (writeResponseResolve = resolve),
+          );
+          const fakeEngineServer = http.createServer(async (_, res) => {
+            await writeResponsePromise;
+            res.writeHead(status);
+            res.end('Important text in the body');
           });
-        });
-        try {
-          const { family, address, port } = fakeEngineServer.address();
-          if (family !== 'IPv4') {
-            throw new Error(`The family was unexpectedly ${family}.`);
+          await new Promise(resolve => {
+            fakeEngineServer.listen(0, '127.0.0.1', () => {
+              resolve();
+            });
+          });
+          async function closeServer() {
+            await new Promise(resolve =>
+              fakeEngineServer.close(() => resolve()),
+            );
           }
-          const fakeEngineUrl = new URL(`http://${address}:${port}`).toString();
+          try {
+            const { family, address, port } = fakeEngineServer.address();
+            if (family !== 'IPv4') {
+              throw new Error(`The family was unexpectedly ${family}.`);
+            }
+            const fakeEngineUrl = `http://${address}:${port}`;
 
-          let reportErrorPromiseResolve: (error: Error) => void;
-          const reportErrorPromise = new Promise<Error>(
-            resolve => (reportErrorPromiseResolve = resolve),
-          );
-          const { url: uri } = await createApolloServer({
-            typeDefs: gql`
-              type Query {
-                something: String!
-              }
-            `,
-            resolvers: { Query: { something: () => 'hello' } },
-            engine: {
-              apiKey: 'service:my-app:secret',
-              endpointUrl: fakeEngineUrl,
-              reportIntervalMs: 1,
-              maxAttempts: 1,
-              reportErrorFunction(error: Error) {
-                reportErrorPromiseResolve(error);
+            // To simulate a network error, we create and close the server.
+            // This lets us still generate a port that is hopefully unused.
+            if (networkError) {
+              await closeServer();
+            }
+
+            let requestCount = 0;
+            const requestAgent = new http.Agent({ keepAlive: false });
+            const realCreateConnection = (requestAgent as any).createConnection;
+            (requestAgent as any).createConnection = function() {
+              requestCount++;
+              return realCreateConnection.apply(this, arguments);
+            };
+
+            let reportErrorPromiseResolve: (error: Error) => void;
+            const reportErrorPromise = new Promise<Error>(
+              resolve => (reportErrorPromiseResolve = resolve),
+            );
+            const { url: uri } = await createApolloServer({
+              typeDefs: gql`
+                type Query {
+                  something: String!
+                }
+              `,
+              resolvers: { Query: { something: () => 'hello' } },
+              engine: {
+                apiKey: 'service:my-app:secret',
+                endpointUrl: fakeEngineUrl,
+                reportIntervalMs: 1,
+                maxAttempts: 3,
+                requestAgent,
+                reportErrorFunction(error: Error) {
+                  reportErrorPromiseResolve(error);
+                },
               },
-            },
-          });
+            });
 
-          const apolloFetch = createApolloFetch({ uri });
+            const apolloFetch = createApolloFetch({ uri });
 
-          // Run a GraphQL query. Ensure that it returns successfully even
-          // though reporting is going to fail. (Note that reporting can't
-          // actually have failed yet because we haven't let return502Promise
-          // resolve.)
-          const result = await apolloFetch({
-            query: `{ something }`,
-          });
-          expect(result.data.something).toBe('hello');
+            // Run a GraphQL query. Ensure that it returns successfully even
+            // though reporting is going to fail. (Note that reporting can't
+            // actually have failed yet (except in the network-error case)
+            // because we haven't let writeResponsePromise resolve.)
+            const result = await apolloFetch({
+              query: `{ something }`,
+            });
+            expect(result.data.something).toBe('hello');
 
-          // Allow reporting to return 502.
-          return502Resolve();
+            if (!networkError) {
+              // Allow reporting to return its response (for every retry).
+              writeResponseResolve();
+            }
 
-          // Make sure we can get the 502 error from reporting.
-          const sendingError = await reportErrorPromise;
-          expect(sendingError.message).toContain('Error: 502: Bad Gateway');
-        } finally {
-          await new Promise(resolve => fakeEngineServer.close(() => resolve()));
+            // Make sure we can get the error from reporting.
+            const sendingError = await reportErrorPromise;
+            expect(sendingError).toBeTruthy();
+            if (networkError) {
+              expect(sendingError.message).toContain(
+                'Error sending report to Apollo Engine servers',
+              );
+              expect(sendingError.message).toContain('ECONNREFUSED');
+            } else {
+              expect(sendingError.message).toBe(
+                `Error sending report to Apollo Engine servers: HTTP status ${status}, Important text in the body`,
+              );
+            }
+            expect(requestCount).toBe(expectedRequestCount);
+          } finally {
+            if (!networkError) {
+              await closeServer();
+            }
+          }
         }
+        it('with retryable error', async () => {
+          await testWithStatus(500, 3);
+        });
+        it('with network error', async () => {
+          await testWithStatus(0, 3);
+        });
+        it('with non-retryable error', async () => {
+          await testWithStatus(400, 1);
+        });
       });
     });