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.

Sign up for GitHub

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

Jump to bottom

Add toggle for including error messages in reports #1615

Merged
merged 3 commits into from
Sep 5, 2018
Merged

Add toggle for including error messages in reports #1615

merged 3 commits into from
Sep 5, 2018

Conversation

evans
Copy link
Contributor

@evans evans commented Sep 4, 2018 •
edited
Loading

We always send traces that includes an error node if the trace has an
error. In the case that sending errors is disabled, we replace the
message and remove the location.

closes #1613

The ui looks like:

screen shot 2018-09-04 at 11 01 03 am

TODO:

  • Update CHANGELOG.md with your change (include reference to issue & this PR)
  • Make sure all of the significant new logic is covered by tests
  • Rebase your changes on master so that they can be merged easily
  • Make sure all tests and linter rules pass

@ghost ghost added the ⛲️ feature New addition or enhancement to existing solutions label Sep 4, 2018
@evans
add toggle for including error messages in reports
8e30c45 
We always send traces that includes an error node if the trace has an
error. In the case that sending errors is disabled, we replace the
message and remove the location.
@evans evans force-pushed the evans/toggle-masking-errors branch from 11bab34 to 8e30c45 Compare September 4, 2018 18:03
@ghost ghost added the ⛲️ feature New addition or enhancement to existing solutions label Sep 4, 2018
@evans
Copy link
Contributor Author

evans commented Sep 4, 2018

This PR does not currently test the end to end integration of the error pipeline from apollo server to Engine ingestion to Engine frontend.

At best right now, we can use a similar strategy to the full AS2 lifecycle test

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

Lines 566 to 579 in 8e30c45

let listener = await startEngineServer({
port,
check: (req, res) => {
const trace = JSON.stringify(Trace.decode(req.body));
try {
expect(trace).toMatch(/nope/);
expect(trace).not.toMatch(/masked/);
} catch (e) {
reject(e);
}
res.end();
listener.close(resolve);
},
});

@martijnwalraven
Copy link
Contributor

I actually just fixed that test today, because the trace report checking wasn't actually being called, among other things. But that should work now.

@martijnwalraven
Copy link
Contributor

Agree we need more comprehensive Engine reporting integration tests, but we can tackle that later.

martijnwalraven
martijnwalraven suggested changes Sep 4, 2018
View reviewed changes
packages/apollo-engine-reporting/src/extension.ts Outdated
}),
);

// 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.

docs/source/api/apollo-server.md Outdated
@@ -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

packages/apollo-engine-reporting/src/extension.ts Outdated
),
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

@evans evans force-pushed the evans/toggle-masking-errors branch from 4f45d9f to 6981267 Compare September 4, 2018 20:50
@evans
[Squash] Address feedback
6152862 
Note that the Engine proxy strips all error information from the traces
with noErrorTraces set. To get errors to show up in the ui, the proxy
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.
@evans evans force-pushed the evans/toggle-masking-errors branch from 6981267 to 6152862 Compare September 4, 2018 21:06
@martijnwalraven martijnwalraven merged commit 4a40976 into master Sep 5, 2018
@martijnwalraven martijnwalraven deleted the evans/toggle-masking-errors branch September 5, 2018 14:01
@dependencies dependencies bot mentioned this pull request Sep 15, 2018
Open
@dependencies dependencies bot mentioned this pull request Oct 1, 2018
Open
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@martijnwalraven martijnwalraven martijnwalraven approved these changes

Assignees
No one assigned
Labels
⛲️ feature New addition or enhancement to existing solutions
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Toggle to remove errors from traces sent to Engine Servers
2 participants
@evans @martijnwalraven