Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add toggle for including error messages in reports #1615

Merged
merged 3 commits into from
Sep 5, 2018
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ All of the packages in the `apollo-server` repo are released with the same versi
- Update `graphql-playground-html` to 1.7.4 [#1586](https://github.com/apollographql/apollo-server/pull/1586)
- Add support for `graphql-js` v14 by augmenting typeDefs with the `@cacheControl` directive so SDL validation doesn't fail [#1595](https://github.com/apollographql/apollo-server/pull/1595)
- Add `node-fetch` extensions typing to `RequestInit` [#1602](https://github.com/apollographql/apollo-server/pull/1602)
- Add toggle for including error messages in reports [#1615](https://github.com/apollographql/apollo-server/pull/1615)

### v2.0.5

Expand Down
4 changes: 4 additions & 0 deletions docs/source/api/apollo-server.md
Original file line number Diff line number Diff line change
Expand Up @@ -344,3 +344,7 @@ addMockFunctionsToSchema({
itself. Set this to false to disable. You can manually invoke 'stop()' and
'sendReport()' on other signals if you'd like. Note that 'sendReport()'
does not run synchronously so it cannot work usefully in an 'exit' handler.

* `sendErrorTraces`: boolean
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this setting isn't really about whether or not to send a trace, maybe it should be something like maskErrors or maskErrorDetails?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Totally! The name is definitely an artifact of evolving from not sending them completely to masking


To remove errors from traces, set this to false. Defaults to true
8 changes: 5 additions & 3 deletions packages/apollo-engine-reporting/src/agent.ts
Original file line number Diff line number Diff line change
Expand Up @@ -60,9 +60,9 @@ export interface EngineReportingOptions {
maxAttempts?: number;
// Minimum backoff for retries. Defaults to 100ms.
minimumRetryDelayMs?: number;
// By default, errors sending reports to Engine servers will be logged
// to standard error. Specify this function to process errors in a different
// way.
// By default, errors that occur when sending trace reports to Engine servers
// will be logged to standard error. Specify this function to process errors
// in a different way.
reportErrorFunction?: (err: Error) => void;
// A case-sensitive list of names of variables whose values should not be sent
// to Apollo servers, or 'true' to leave out all variables. In the former
Expand All @@ -81,6 +81,8 @@ export interface EngineReportingOptions {
handleSignals?: boolean;
// Sends the trace report immediately. This options is useful for stateless environments
sendReportsImmediately?: boolean;
// To remove errors from traces, set this to false. Defaults to true
sendErrorTraces?: boolean;

// XXX Provide a way to set client_name, client_version, client_address,
// service, and service_version fields. They are currently not revealed in the
Expand Down
30 changes: 20 additions & 10 deletions packages/apollo-engine-reporting/src/extension.ts
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,10 @@ export class EngineReportingExtension<TContext = any>
options: EngineReportingOptions,
addTrace: (signature: string, operationName: string, trace: Trace) => void,
) {
this.options = options;
this.options = {
sendErrorTraces: true,
...options,
};
this.addTrace = addTrace;
const root = new Trace.Node();
this.trace.root = root;
Expand Down Expand Up @@ -225,15 +228,22 @@ export class EngineReportingExtension<TContext = any>
node = specificNode;
}
}
node!.error!.push(
new Trace.Error({
message: error.message,
location: (error.locations || []).map(
({ line, column }) => new Trace.Location({ line, column }),
),
json: JSON.stringify(error),
}),
);

// With noErrorTraces, the Engine proxy strips all error information
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't explain the behavior by comparing it to the noErrorTraces option in the Engine proxy, we should just describe what it does.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay that makes sense. I'll include the comment in the commit message. I think it's important to understand the context around how AS2 differs from the proxy.

// from the traces and sends error counts inside of the aggregated stats
// reports. To get similar behavior inside of the apollo server metrics
// reporting, we always send a trace and mask out the PII
const errorInfo = this.options.sendErrorTraces
? {
message: error.message,
location: (error.locations || []).map(
({ line, column }) => new Trace.Location({ line, column }),
),
json: JSON.stringify(error),
}
: { message: 'Masked by Apollo Engine Reporting' };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe just <masked>?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it


node!.error!.push(new Trace.Error(errorInfo));
});
}
}
Expand Down