Simplify apollo-engine-reporting operationName capturing#2899
Merged
Conversation
1819edb to
ab8a250
Compare
abernix
approved these changes
Jun 24, 2019
abernix
left a comment
Member
There was a problem hiding this comment.
LGTM! Thank you so much for taking the time to unwind this!
The full query caching change (#2437) intended to introduce didResolveOperation to the old graphql-extensions API used by apollo-engine-reporting ("backporting" it from the newer plugin API). However, that change accidentally forgot to invoke didResolveOperation from the request pipeline! This meant that the operation name never got reported. The change was backed out in #2557. But this unfortunately re-introduced the exact bug that the change in #2437 was intended to fix: operationName was no longer set when a result is served from the cache! Additionally, it was not set if a *plugin* didResolveOperation call threw, which is what happens when the operation registry plugin forbids an operation. While we could have fixed this by reintroducing the didResolveOperation extension API, there would be a subtle requirement that the apollo-engine-reporting extension didResolveOperation be run before the possibly-throwing operation registry didResolveOperation. So instead, @abernix implemented #2711. This used `requestContext.operationName` as a fallback if neither executionDidStart nor willResolveField gets called. This will be set if the operation properly parsed, validates, and either has a specified operationName that is found in the document, or there is no specified operationName and there is exactly one operation in the document and it has a name. (Note that no version of this code ever sent the user-provided operationName in case of parse or validation errors.) The existing code is correct, but this PR cleans up a few things: - #2557 reverted the one *implementation* of the didResolveOperation extension API, and #2437 accidentally didn't contain any *callers* of the API, but it was still declared on GraphQLExtension and GraphQLExtensionStack. This PR removes those declarations (which have never been useful). - We currently look for the operation name in willResolveField. But in any case where fields are successfully being resolved, the pipeline must have managed to successfully resolve the operation and set requestContext.operationName. So we don't actually need the willResolveField code, because the "fallback" in the requestDidStart end-callback will have the same value. So take this code away. (This change is the motivation for this PR; for federation metrics I'm trying to disengage the "calculate times for fields" part of trace generation from the rest of it.) - Fix the comment in "requestDidEnd" that implied incorrectly that requestContext.operationName was the user-provided name rather than the pipeline-calculated name. Be explicit both there and in requestPipeline.ts that we are relying on the fact that the RequestContext passed to requestDidStart is mutated to add operationName before its end handler is called. This change is intended to be a no-op change (other than the removal of the never-used APIs).
ab8a250 to
bc0ae82
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The full query caching change (#2437) intended to introduce didResolveOperation
to the old graphql-extensions API used by apollo-engine-reporting ("backporting"
it from the newer plugin API). However, that change accidentally forgot to
invoke didResolveOperation from the request pipeline! This meant that the
operation name never got reported.
The change was backed out in #2557. But this unfortunately re-introduced the
exact bug that the change in #2437 was intended to fix: operationName was no
longer set when a result is served from the cache! Additionally, it was not set
if a plugin didResolveOperation call threw, which is what happens when the
operation registry plugin forbids an operation.
While we could have fixed this by reintroducing the didResolveOperation
extension API, there would be a subtle requirement that the
apollo-engine-reporting extension didResolveOperation be run before the
possibly-throwing operation registry didResolveOperation.
So instead, @abernix implemented #2711. This used
requestContext.operationNameas a fallback if neither executionDidStart nor willResolveField gets
called. This will be set if the operation properly parsed, validates, and either
has a specified operationName that is found in the document, or there is no
specified operationName and there is exactly one operation in the document and
it has a name.
(Note that no version of this code ever sent the user-provided operationName in
case of parse or validation errors.)
The existing code is correct, but this PR cleans up a few things:
[release-2.5.0] Fix missing
operationNameinapollo-engine-reporting. #2557 reverted the one implementation of the didResolveOperation extensionAPI, and Full query response cache plugin #2437 accidentally didn't contain any callers of the API, but it
was still declared on GraphQLExtension and GraphQLExtensionStack. This PR
removes those declarations (which have never been useful).
We currently look for the operation name in willResolveField. But in any case
where fields are successfully being resolved, the pipeline must have managed
to successfully resolve the operation and set requestContext.operationName. So
we don't actually need the willResolveField code, because the "fallback" in
the requestDidStart end-callback will have the same value. So take this code
away. (This change is the motivation for this PR; for federation metrics I'm
trying to disengage the "calculate times for fields" part of trace generation
from the rest of it.)
Fix the comment in "requestDidEnd" that implied incorrectly that
requestContext.operationName was the user-provided name rather than the
pipeline-calculated name. Be explicit both there and in requestPipeline.ts
that we are relying on the fact that the RequestContext passed to
requestDidStart is mutated to add operationName before its end handler is
called.
This change is intended to be a no-op change (other than the removal of the
never-used APIs).