engine-reporting: Capture operationName and document within errors.
#2711
Conversation
This file contains 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
This is another take on #2659 which has been otherwise excluded from the 2.6.0 release. The work in #2659 was meant to ensure that the `document` and `operationName` were set at the appropriate locations within the lifecycle events for the `graphql-extensions` module, specifically when an error had been thrown within `didResolveOperation` of the new request pipeline. While the solution in #2659 certainly works, it introduced a new life-cycle method for details which we could also calculate without adding to the API surface area by using information which was already exposed. Even though `graphql-extensions` is no longer really being supported, avoiding growth on the API surface area of `graphql-extensions` is still important for any API! Ref: #2659
abernix
added a commit
that referenced
this pull request
May 22, 2019
By testing to make sure we don't default to the query-based signature, rather than the calculated signature when an error condition occurs within `didResolveOperation`.
abernix
added a commit
that referenced
this pull request
May 22, 2019
abernix
added a commit
that referenced
this pull request
May 23, 2019
glasser
reviewed
Jun 21, 2019
| 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 |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Thanks for sorting this out, @glasser!
glasser
added a commit
that referenced
this pull request
Jun 21, 2019
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).
glasser
added a commit
that referenced
this pull request
Jun 21, 2019
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).
glasser
added a commit
that referenced
this pull request
Jun 24, 2019
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).
glasser
added a commit
that referenced
this pull request
Jun 24, 2019
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).
abernix
deleted the
abernix/capture-operationNameAndDocument-via-other-means
branch
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.
This is another take on #2659 which has been otherwise excluded from the
2.6.0 release.
The work in #2659 was meant to ensure that the
documentandoperationNamewere set at the appropriate locations within the lifecycleevents for the
graphql-extensionsmodule, specifically when an error hadbeen thrown within
didResolveOperationof the new request pipeline.While the solution in #2659 certainly works, it introduced a new life-cycle
method for details which we could also calculate without adding to the
API surface area by using information which was already exposed.
Even though
graphql-extensionsis no longer really being supported,avoiding growth on the API surface area of
graphql-extensionsis stillimportant for any API!
Ref: #2659