-
Notifications
You must be signed in to change notification settings - Fork 2k
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.
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
Gateway(experimental): compress downstream requests via generated fragments #3791
Gateway(experimental): compress downstream requests via generated fragments #3791
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great! Love how concise it is.
I left a couple of comments to make it less concise ;) Specifically, maybe it's a worthwhile experiment to refcount generated fragments and only include them if they definitely reduce query size?
) { | ||
const expandedSelectionSet = selectionSetFromFieldSet(fields, returnType); | ||
|
||
const key = print(expandedSelectionSet); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't think we should block on this, but at some point it would be nice to try a key trie rather than printing string keys here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you feel like explaining a bit, I'd be curious to hear what you imagine this looks like.
selectionSet: fragmentSelection, | ||
}; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could consider retaining the expanded selection set and updating a refcount here and then only emitting the fragment & definition if count > some n
?
(Or, rather than an arbitrary n, we could actually check if print(definition).length + count - count * print(expandedSelectionSet).length < 0
and only do the fragment if that's the case. Does that get messy with fragments including fragments? It seems like at worst it'll overestimate the savings.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think since we're emitting these fragment spreads on the fly, we're pretty limited currently on when we get to emit.
I believe the approach you're hinting at requires a second pass one way or another - one which performs the analysis and another which handles some transformation. Certainly not opposed to something like this in a future refinement, but I'm not sure it fits into this approach.
Please let me know if I've misunderstood anything!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few comments, but I think my primary concerns are considering the testing of aliases. They may just work since they would be considered within the key
generated by print
.
returnType: GraphQLCompositeType, | ||
context: QueryPlanningContext | ||
) { | ||
const expandedSelectionSet = selectionSetFromFieldSet(fields, returnType); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally speaking, as we increase its use, it would be nice if we had tests for selectionSetFromFieldSet
, which I'm not seeing in the current test coverage. I realize this isn't as a result of your work on this PR though.
@@ -784,17 +872,25 @@ export class QueryPlanningContext { | |||
return isAbstractType(type) ? this.schema.getPossibleTypes(type) : [type]; | |||
} | |||
|
|||
getVariableUsages(selectionSet: SelectionSetNode) { | |||
getVariableUsages(selectionSet: SelectionSetNode, fragments: Set<FragmentDefinitionNode>) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is merely a matter of remembering the difference, but this is a divergence from the getVariableUsages
that I'm used to!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We discussed this one via call, and I don't see another way to approach this, really. graphql-js
has a similar function that recurses and handles full documents, however in our case we're only ever visiting a single selectionSet
at a time. For now I'm content with the approach here, but please let me know if you have a major concern with this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've spoken outside the context of this PR. There are some changes to consider but I bless with my approval.
A |
For large queries (esp. ones that leverage many nested fragments), queries to downstream services can blow up astronomically because the gateway expands everything during a downstream request. i.e. we've seen a ~700 line query become over 200k lines due to this expansion. This commit has the gateway build fragments on the fly for the selection sets that it encounters and reuses them whenever possible. This is an initial and simple implementation, but with tangible wins. This is currently being landed as an experimental feature that is disabled by default. To enable, simply add to your gateway config: experimental_compressDownstreamRequests: true
78ae298
to
98bdb30
Compare
…llographql/apollo-server#3791) For large queries (esp. ones that leverage many nested fragments), queries to downstream services can blow up astronomically because the gateway expands everything during a downstream request. i.e. we've seen a ~700 line query become over 200k lines due to this expansion. This commit has the gateway build fragments on the fly for the selection sets that it encounters and reuses them whenever possible. This is an initial and simple implementation, but with tangible wins. This is currently being landed as an experimental feature that is disabled by default. To enable, simply add to your gateway config: experimental_autoFragmentization: true Apollo-Orig-Commit-AS: apollographql/apollo-server@c48c0a3
For large queries (esp. ones that leverage many nested fragments), queries to downstream services can blow up astronomically because the gateway expands everything during a downstream request. For example, we've seen a ~700 line query making heavy use of fragments become over 200k lines (formatted) due to this expansion.
This PR has the gateway build fragments on the fly for the selection sets that it encounters and reuses them whenever possible. This is an heavy-handed and simple implementation, but with tangible wins for large queries. More complex and correct solutions exist and are currently being discussed. If anyone feels so inclined to solve this interesting problem, please reach out to us - we'd be happy to discuss approach and entertain a contribution here.
This is currently being landed as an experimental feature that is disabled by default. To enable, simply add to your gateway config:
experimental_compressDownstreamRequests: true