[BE-615] AER: use a static identifier for GraphQL validation failures #3465

zionts · 2019-11-02T02:35:35Z

This PR changes the apollo usage reporting library to use static identifiers for operation documents that are not able to be executed.

When users of this module receive many un-executable operation documents, such as a non parse-able operation documents, invalid operation documents, or invalid operation names, every operation document is sent to Apollo Studio. This results in a cardinality explosion within Graph Manager. After a few thousand of these invalid operation names / documents are reported, the UI for the customer is borderline unusable due to the cardinality explosion & schema validation reaches a capacity of operations as well.

In general, we want to avoid storing & exposing personal information in Studio, and in current reporting agents, this is also problematic for operations that fail to execute. Because we currently report these operations with a signature matching the entire operation body, this is an easy trap for users to accidentally send user information through our system, when argument literals exist in the document.

The static identifiers are:

## GraphQLParseFailure for documents that don't parse as valid graphql
## GraphQLValidationFailure for documents that aren't valid given the schema running on the server
## GraphQLUnknownOperationName for operation documents which don't have an operation name in the document

Additionally, it allows users to optionally include the body of the operations that fail validation with sendUnexecutableOperationDocuments. This will send the operation body as part of the trace so they can be viewed alongside the trace.

zionts · 2019-11-02T02:44:53Z

This needs an upstream PR on monorepo, too, but ready for review otherwise :)

packages/apollo-engine-reporting-protobuf/src/reports.proto

packages/apollo-engine-reporting/src/__tests__/extension.test.ts

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

packages/apollo-engine-reporting-protobuf/package-lock.json

package-lock.json

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

Addressed — please re-review when possible :)

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

glasser

(can't respond to one thread, putting it here)

No more prettier in this project, but I fixed it manually.

prettier is still installed at a consistent version in apollo-server; it just isn't automatically enforced via a linter. However, apollo-engine-reporting still is perfectly formatted with prettier, and I'd like to keep it this way. @abernix and I talked this week. I understand why he thinks Prettier specifically isn't a good fit for our open source projects but he agrees that that doesn't need to mean that we should regard our files as having no style rules at all, or that we shouldn't add in other style enforcement via eslint (just not the line length rules from prettier specifically). Let's keep this package prettier-clean, manually for now and maybe automatically later.

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

glasser · 2019-11-20T07:15:13Z

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

+  // and the operation name and signature will always be reported with a static
+  // identifier. Whether the operation was a parse failure or a validation
+  // failure will be embedded within the stats report key itself
+  sendOperationDocumentsOnValidationFailure?: boolean;


Why not separate flags for validation and parsing? Or a name that is explicit? I really think it's misleading to use the word "validation" to mean "parse or validation". Words have meanings.

glasser · 2019-11-20T07:16:49Z

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

+    } else {
+      const signature = await this.getTraceSignature({
+        queryHash,
+        documentAST,


I don't get how this works: this variable is nullable but the parameter it's being passed to isn't?

The null case is handled above in an else if branch, so here we know that documentAST is defined.

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

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

glasser · 2019-11-20T07:24:21Z

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

@@ -177,6 +193,7 @@ export class EngineReportingExtension<TContext = any>
    // isn't actually in the document. We want to know the name in that case
    // too, which is why it's important that we save the name now, and not just
    // rely on requestContext.operationName (which will be null in this case).
+    this.gqlValidationSucceeded = true;


Similarly, at least have a comment that this would better belong in validationDidEnd. Though frankly you could also just change graphql-extensions to pass validationErrors to validationDidEnd. I made plenty of graphql-extensions changes as needed by this file.

It seemed like making more extensions-specific changes would make this logic harder to port to the new plugin API in a way that was pretty certainly in alignment.

That would actually align the stack API with the new plugin API, which already passes validation errors to its version of validationDidEnd.

Also graphql-extensions is a bit of a beast I just prefer not to touch 😅 Along with the fact that we already know there is overhead for each function call we're putting onto the stack, which is currently causing aer to spew tons of garbage, adding another function for validationDidEnd is not something I really really want to do, but if you want to push for it, I shall comply :P

The function already exists, it just doesn't receive the list of errors...

glasser · 2019-11-20T07:26:19Z

packages/apollo-engine-reporting-protobuf/src/reports.proto

+	// Optional: when GraphQL parsing or validation against the GraphQL schema fails, these fields
+	// can include reference to the operation being sent for users to dig into the set of operations
+	// that are failing validation.
+	string operationBodyOnValidationFailure = 27;


similarly, these shouldn't use "validation" to mean "parsing or validation". Split the fields or make a better name.

Maybe unexecutedOperationBody / unexecutedOperationName?

glasser · 2019-12-14T00:46:45Z

sorry for the lag. will try to get to this early next week. looks like it needs a rebase.

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

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

glasser · 2019-12-17T00:39:25Z

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

@@ -177,6 +193,7 @@ export class EngineReportingExtension<TContext = any>
    // isn't actually in the document. We want to know the name in that case
    // too, which is why it's important that we save the name now, and not just
    // rely on requestContext.operationName (which will be null in this case).
+    this.gqlValidationSucceeded = true;


That would actually align the stack API with the new plugin API, which already passes validation errors to its version of validationDidEnd.

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

le stale again

glasser · 2019-12-18T21:51:37Z

packages/apollo-engine-reporting/src/__tests__/extension.test.ts

@@ -65,11 +65,17 @@ test('trace construction', async () => {
    addTrace,


How about end-to-end tests! Search for eg sets the trace key to operationName when it is defined

… failure. Addresses feedback in below referenced [[Comment]]. If operation resolution (parsing and validating the document followed by selecting the correct operation) resulted in the population of the `operationName`, we'll use that. (For anonymous operations, `requestContext.operationName` is null, which we represent here as the empty string.) If the user explicitly specified an `operationName` in their request but operation resolution failed (due to parse or validation errors or because there is no operation with that name in the document), we still put _that_ user-supplied `operationName` in the trace. This allows the error to be better understood in Graph Manager. (We are considering changing the behavior of `operationName` in these three error cases; see [[#3465]] below for details.) [Comment]: #3998 (comment) [#3465]: #3465

glasser · 2020-08-17T16:15:08Z

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

@@ -163,6 +163,18 @@ export const plugin = <TContext>(
        }
      }

+      /**
+       *  This is set to true at the beginning of the pipeline. If we resole


minor spelling/capitalization/punctuation notes: "resolve", "GraphQL", "can start as true", "operation, the operation must"

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

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

packages/apollo-engine-reporting/src/__tests__/plugin.test.ts

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

packages/apollo-engine-reporting/src/__tests__/plugin.test.ts

Josh should update this commit message to be accurate :)

glasser

overall looking great

packages/apollo-server-core/src/utils/pluginTestHarness.ts

packages/apollo-server-core/src/plugin/usageReporting/__tests__/plugin.test.ts

packages/apollo-server-core/src/plugin/usageReporting/options.ts

glasser · 2020-09-29T18:51:55Z

docs/source/api/plugin/usage-reporting.md

+</td>
+<td>
+
+Whether to include the entire document in the trace if the operation was a GraphQL parse or validation error (i.e. failed the GraphQL parse or validation phases). This will be included as a separate field on the trace and the operation name and signature will always be reported with a cosntant identifier. Whether the operation was a parse failure or a validation failure will be embedded within the stats report key itself.


cosntant -> constant

But also I think we can make this be a little more user-focused. Maybe something more along the lines of

Statistics about operations that your server cannot execute are not reported under each document separately to Apollo's servers, but are grouped together as "parse failure", "validation failure", or "unknown operation name". By default, the usage reporting plugin does not include the full operation document in reported traces, because it is challenging to strip potential private information (like string constants) from invalid operations. If you'd like the usage reporting plugin to include the full operation document so you can view it in Apollo Studio's trace view, set this to true.

The wording isn't ideal but I think this helps explain what's going on and why you might want to set it better?

glasser

i have some changelog suggestions, and i do hope you remember to make the commit message up to date, but otherwise looks great!

…pii-in-invalid-graphql

glasser · 2020-10-02T21:32:46Z

for the PR description (final commit message when squashing) make sure you have double #s in the operation names

abernix · 2020-10-05T12:23:21Z

I've retargeted this to release-2.19.0, which is where I expect this will land.

We've already done #3465, so we no longer need a single const to represent "either the actual resolved operation name that definitely points to a real parsed and validated operation, or else what the user wrote in the request". We can use the latter in the one case where we report an unexecuted operation name, and the former otherwise.

zionts requested review from abernix, glasser and pcarrier November 2, 2019 02:35

glasser previously requested changes Nov 7, 2019

View reviewed changes

glasser reviewed Nov 7, 2019

View reviewed changes

packages/apollo-engine-reporting/src/agent.ts Outdated Show resolved Hide resolved

zionts force-pushed the adam/19/10/avoid-cardinality-explosions-and-pii-in-invalid-graphql branch from 8b8814b to aa8d823 Compare November 14, 2019 02:19

zionts commented Nov 14, 2019

View reviewed changes

packages/apollo-engine-reporting/src/extension.ts Outdated Show resolved Hide resolved

glasser requested changes Nov 20, 2019

View reviewed changes

zionts force-pushed the adam/19/10/avoid-cardinality-explosions-and-pii-in-invalid-graphql branch 2 times, most recently from 2d9e00f to b0f2429 Compare December 9, 2019 23:10

glasser previously requested changes Dec 17, 2019

View reviewed changes

zionts force-pushed the adam/19/10/avoid-cardinality-explosions-and-pii-in-invalid-graphql branch from 539d2da to 815b562 Compare December 17, 2019 02:20

zionts removed the request for review from pcarrier December 17, 2019 02:45

glasser reviewed Dec 18, 2019

View reviewed changes

zionts changed the title ~~AER: use a static identifier for GraphQL validation failures~~ [BE-615] AER: use a static identifier for GraphQL validation failures Jan 24, 2020

glasser mentioned this pull request Apr 24, 2020

refactor: Graph Manager (Engine) reporting "extensions" become "plugins". #3998

Merged

glasser mentioned this pull request May 20, 2020

feat(reporting): Add reportTiming option to EngineReportingOptions #3918

Merged

Base automatically changed from master to main June 24, 2020 18:16

jsegaran force-pushed the adam/19/10/avoid-cardinality-explosions-and-pii-in-invalid-graphql branch from 8c3488a to d04a273 Compare August 3, 2020 00:05

jsegaran requested a review from glasser August 3, 2020 00:25

jsegaran force-pushed the adam/19/10/avoid-cardinality-explosions-and-pii-in-invalid-graphql branch from 0afac33 to 786bdcb Compare August 4, 2020 04:30

glasser requested changes Aug 17, 2020

View reviewed changes

glasser added a commit that referenced this pull request Sep 23, 2020

[GM-615] Rebase of #3465 onto v2.18

483ce9a

Josh should update this commit message to be accurate :)

glasser force-pushed the adam/19/10/avoid-cardinality-explosions-and-pii-in-invalid-graphql branch from d66ef1e to 483ce9a Compare September 23, 2020 23:18

[GM-615] Rebase of #3465 onto v2.18

34390d9

Josh should update this commit message to be accurate :)

jsegaran force-pushed the adam/19/10/avoid-cardinality-explosions-and-pii-in-invalid-graphql branch from 7fc3644 to 4f70449 Compare September 28, 2020 18:03

Joshua Segaran added 3 commits September 28, 2020 11:05

Fix special signatures

e476ed3

Fix comments

0f7a57f

Dry up if blocks

e5e8dbb

jsegaran force-pushed the adam/19/10/avoid-cardinality-explosions-and-pii-in-invalid-graphql branch from 4f70449 to e5e8dbb Compare September 28, 2020 18:05

Joshua Segaran added 2 commits September 28, 2020 11:16

Add an anonymous op test

e7d6e0c

More tests

fd9c1d5

jsegaran force-pushed the adam/19/10/avoid-cardinality-explosions-and-pii-in-invalid-graphql branch from 14e4218 to fd9c1d5 Compare September 28, 2020 22:05

jsegaran requested a review from glasser September 28, 2020 22:06

glasser requested changes Sep 29, 2020

View reviewed changes

Joshua Segaran added 2 commits September 30, 2020 11:33

Fix tests names and comments

03d0054

better message

3f4f561

jsegaran requested a review from glasser September 30, 2020 18:37

FIx test

d71abe9

jsegaran force-pushed the adam/19/10/avoid-cardinality-explosions-and-pii-in-invalid-graphql branch from 7d322cf to d71abe9 Compare September 30, 2020 18:51

Joshua Segaran added 2 commits September 30, 2020 14:11

Change name

65b7392

Add changelog and rename option

943fe65

glasser approved these changes Oct 2, 2020

View reviewed changes

Merge branch 'main' into adam/19/10/avoid-cardinality-explosions-and-…

7b316b7

…pii-in-invalid-graphql

abernix added this to the Release 2.19.0 milestone Oct 5, 2020

abernix changed the base branch from main to release-2.19.0 October 5, 2020 12:22

Update changelog

49204ba

jsegaran merged commit b427e78 into release-2.19.0 Oct 16, 2020

jsegaran deleted the adam/19/10/avoid-cardinality-explosions-and-pii-in-invalid-graphql branch October 16, 2020 21:49

glasser mentioned this pull request Jun 9, 2021

Make error handling around APQs more consistent #5287

Merged

github-actions bot locked as resolved and limited conversation to collaborators Mar 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BE-615] AER: use a static identifier for GraphQL validation failures #3465

[BE-615] AER: use a static identifier for GraphQL validation failures #3465

zionts commented Nov 2, 2019 •

edited by jsegaran

Loading

zionts commented Nov 2, 2019

glasser left a comment

glasser Nov 20, 2019

glasser Nov 20, 2019

zionts Dec 5, 2019

glasser Nov 20, 2019

zionts Dec 5, 2019

glasser Dec 17, 2019

zionts Dec 17, 2019

glasser Dec 18, 2019

glasser Nov 20, 2019

zionts Dec 5, 2019

glasser commented Dec 14, 2019

glasser Dec 17, 2019

glasser Dec 18, 2019

glasser Aug 17, 2020

glasser left a comment

glasser Sep 29, 2020 •

edited

Loading

glasser left a comment

glasser commented Oct 2, 2020

abernix commented Oct 5, 2020

		@@ -65,11 +65,17 @@ test('trace construction', async () => {
		addTrace,

[BE-615] AER: use a static identifier for GraphQL validation failures #3465

[BE-615] AER: use a static identifier for GraphQL validation failures #3465

Conversation

zionts commented Nov 2, 2019 • edited by jsegaran Loading

zionts commented Nov 2, 2019

glasser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glasser commented Dec 14, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glasser left a comment

Choose a reason for hiding this comment

glasser Sep 29, 2020 • edited Loading

Choose a reason for hiding this comment

glasser left a comment

Choose a reason for hiding this comment

glasser commented Oct 2, 2020

abernix commented Oct 5, 2020

zionts commented Nov 2, 2019 •

edited by jsegaran

Loading

glasser Sep 29, 2020 •

edited

Loading