Skip to content

Commit

Permalink
apollo-engine-reporting: fix maxAttempts parameter, log 5xx body (#3218)
Browse files Browse the repository at this point in the history 
- The behavior of maxAttempts previously failed to match its documentation or
  the literal reading of its name. Previously, setting maxAttempts=1 would
  actually result in one retry after the initial attempt. This change fixes
  the behavior to match the docs and the name.

- We intend the bodies of Engine report endpoint errors to be useful in error
  logs, even 5xx errors. This change returns to including them in the reported
  error.

PR #1274 (merged before the initial release of apollo-engine-reporting)
regressed both of these issues.
  • Loading branch information
@glasser @trevor-scheer
glasser authored and trevor-scheer committed Aug 27, 2019
1 parent c798068 commit 4675c67
Show file tree
Hide file tree
Showing 3 changed files with 118 additions and 60 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
15 changes: 10 additions & 5 deletions packages/apollo-engine-reporting/src/agent.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
161 changes: 106 additions & 55 deletions packages/apollo-server-integration-testsuite/src/ApolloServer.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});
});
});

Expand Down

0 comments on commit 4675c67

Please sign in to comment.