Skip to content

Commit

Permalink
Disable exposeDocumentNodeInFetchNode by default (#2230)
Browse files Browse the repository at this point in the history
Since #1878 (added in 2.1.0), the full AST of subgraph fetch operations
has been store in query plans. However, this is not used by the gateway
by default, while it noticeably increases the size of query plans,
leading to either a less efficient query plan cache (if its size is
limited) or increased memory consumption/pressue (if the cache is given
plenty of size).

This patch disable this option by default. Users that explicitely
need to access the subgraph fetch ASTs can still make the trade-off
of additional memory pressure by manually using the
`GatewayConfig.queryPlannerConfig.exposeDocumentNodeInFetchNode` config
option.

Fixes #2227
  • Loading branch information
Sylvain Lebresne authored Nov 2, 2022
1 parent cf3bfcc commit 13d3c86
Show file tree
Hide file tree
Showing 6 changed files with 34 additions and 8 deletions.
4 changes: 4 additions & 0 deletions gateway-js/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,10 @@ This CHANGELOG pertains only to Apollo Federation packages in the 2.x range. The

## vNext

- __BREAKING__: Disable exposing full document to sub-query by default (introduced 2.1.0):
- This change decreases memory consumption in general (which is the reason for disabling this by
default), but users that have custom code making use of `GraphQLDataSourceProcessOptions.document`
will now need to explicitly set `GatewayConfig.queryPlannerConfig.exposeDocumentNodeInFetchNode`.
- Allows `@shareable` to be repeatable so it can be allowed on both a type definition and its extensions [PR #2175](https://github.com/apollographql/federation/pull/2175).
- Note that this require the use of the new 2.2 version of the federation spec introduced in this change.
- Preserve default values of input object fields [PR #2218](https://github.com/apollographql/federation/pull/2218).
Expand Down
2 changes: 1 addition & 1 deletion gateway-js/src/__tests__/execution-utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ export function getFederatedTestingSchema(services: ServiceDefinitionModule[] =
throw new GraphQLSchemaValidationError(compositionResult.errors);
}

const queryPlanner = new QueryPlanner(compositionResult.schema, { exposeDocumentNodeInFetchNode: false} );
const queryPlanner = new QueryPlanner(compositionResult.schema);
const schema = buildSchema(compositionResult.supergraphSdl);

const serviceMap = Object.fromEntries(
Expand Down
4 changes: 4 additions & 0 deletions gateway-js/src/datasources/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,10 @@ export type GraphQLDataSourceProcessOptions<
context: GatewayGraphQLRequestContext<TContext>['context'];
/**
* The document representation of the request's query being sent to the subgraph, if available.
*
* Note that this field is not populated by default. You can enable it by setting the
* `GatewayConfig.queryPlannerConfig.exposeDocumentNodeInFetchNode` configuration but note that
* this will increase the memory used by the gateway query plan cache.
*/
document?: GatewayGraphQLRequestContext<TContext>['document'];
}
Expand Down
14 changes: 8 additions & 6 deletions gateway-js/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -192,13 +192,15 @@ export class ApolloGateway implements GatewayInterface {
}

private initQueryPlanStore(approximateQueryPlanStoreMiB?: number) {
// Create ~about~ a 30MiB InMemoryLRUCache (or 50MiB if the full operation ASTs are
// enabled in query plans as this requires plans to use more memory). This is
// less than precise since the technique to calculate the size of a DocumentNode is
// only using JSON.stringify on the DocumentNode (and thus doesn't account
// for unicode characters, etc.), but it should do a reasonable job at
// providing a caching document store for most operations.
const defaultSize = this.config.queryPlannerConfig?.exposeDocumentNodeInFetchNode ? 50 : 30;
return new LRUCache<string, QueryPlan>({
// Create ~about~ a 30MiB InMemoryLRUCache. This is less than precise
// since the technique to calculate the size of a DocumentNode is
// only using JSON.stringify on the DocumentNode (and thus doesn't account
// for unicode characters, etc.), but it should do a reasonable job at
// providing a caching document store for most operations.
maxSize: Math.pow(2, 20) * (approximateQueryPlanStoreMiB || 30),
maxSize: Math.pow(2, 20) * (approximateQueryPlanStoreMiB || defaultSize),
sizeCalculation: approximateObjectSize,
});
}
Expand Down
4 changes: 4 additions & 0 deletions query-planner-js/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,10 @@ This CHANGELOG pertains only to Apollo Federation packages in the 2.x range. The

## vNext

- __BREAKING__: Disable exposing full document to sub-query by default (introduced in 2.1.0):
- This change decreases memory consumption in general (which is the reason for disabling this by
default), but users that have custom code making use of `GraphQLDataSourceProcessOptions.document`
will now need to explicitly set `GatewayConfig.queryPlannerConfig.exposeDocumentNodeInFetchNode`.
- Drop support for node12 [PR #2202](https://github.com/apollographql/federation/pull/2202)

## 2.1.4
Expand Down
14 changes: 13 additions & 1 deletion query-planner-js/src/config.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,18 @@
import { Concrete } from "@apollo/federation-internals";

export type QueryPlannerConfig = {
/**
* If enabled, the `FetchNode.operationDocumentNode` field in query plan will be populated with the AST
* of the underlying operation (_on top_ of the "serialized" string `FetchNode.operation` which is always
* present). This can used by specific gateway user code that needs read-only access to such AST in
* order to save having to parse `FetchNode.operation`. Without this option, `FetchNode.operationDocumentNode`
* will always be `undefined`.
*
* Enabling this option will make query plans use more memory and you should consider increasing the
* query plan cache size (though `GatewayConfig.experimental_approximateQueryPlanStoreMiB`) if you enable it.
*
* Defaults to false (at least since 2.2; it temporarily defaulted to true before 2.2).
*/
exposeDocumentNodeInFetchNode?: boolean;

/**
Expand Down Expand Up @@ -39,7 +51,7 @@ export function enforceQueryPlannerConfigDefaults(
config?: QueryPlannerConfig
): Concrete<QueryPlannerConfig> {
return {
exposeDocumentNodeInFetchNode: true,
exposeDocumentNodeInFetchNode: false,
reuseQueryFragments: true,
incrementalDelivery: {
enableDefer: false,
Expand Down

0 comments on commit 13d3c86

Please sign in to comment.