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

engine-reporting: Capture operationName and document within errors. #2711

Merged
merged 1 commit into from
May 22, 2019
Merged

engine-reporting: Capture operationName and document within errors. #2711

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
10 changes: 8 additions & 2 deletions packages/apollo-engine-reporting/src/extension.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,12 +199,18 @@ export class EngineReportingExtension<TContext = any>
this.trace.fullQueryCacheHit = !!o.requestContext.metrics
.responseCacheHit;

const operationName = this.operationName || '';
// If the `operationName` was not already set elsewhere, for example,
// through the `executionDidStart` or the `willResolveField` hooks, then
// we'll resort to using the `operationName` which was requested to be
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure this change does what the comment says. requestContext.operationName is not "the name the client wrote in the operation" (that's request.operationName). It's "the name of the operation that will be run, assuming the request passes the parsing and validation stages, and the 'operation naming' rules pass".

By "operation naming rules" I mean "either the document contains an operation with the given name, or no name was given and there is exactly one operation". Notably, this is null if the client wrote a name but the name isn't present.

this.operationName currently gets set to:

  • if we make it to executionDidStart, "the name the client wrote in the operation" (because that's how ExecutionArgs is set up in requestPipeline: request.operationName, not requestContext.operationName)
  • if the client didn't write a name in the operation and we make it to resolve some field, it gets set to the actual operation name we're running (which, in the case we made it this far, must equal requestContext.operationName)

Notably, though, parsing or validation fails, then not only will we fail to make it to executionDidStart/willResolveField, we also won't get to the code that assigns requestContext.operationName. And so the code here won't actually add the client-supplied operation name, which I think is the goal? Or is it only that we care about the didResolveOperation throwing case, and not the parse failure / validation failure case? (THANK YOU for explaining in the PR desc the motivation behind this PR!!!)

Anyway, I'm trying right now to disengage willResolveField (timing calculations) from the rest of the extension (a refactoring that'll be helpful for federated metrics). I think I have a simple answer and will send you a PR including a tweak of this comment. See also #2557 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for sorting this out, @glasser!

// executed by the client.
const operationName =
this.operationName || o.requestContext.operationName || '';
const documentAST = this.documentAST || o.requestContext.document;

this.addTrace({
operationName,
queryHash,
documentAST: this.documentAST,
documentAST,
queryString: this.queryString || '',
trace: this.trace,
});
Expand Down