chore(gateway): Simplify startup code paths #440

trevor-scheer · 2021-01-27T16:34:37Z

In my attempts to implement CSDL fetching for managed gateway configurations, I found the complexity of our startup code to really inhibit my progress (and in turn, I'll need to add a bit more complexity for what I'm trying to accomplish).

After thinking through what is actually happening during startup (and wanting to simplify the code paths specifically loadServiceDefinitions), I was able to tease out the idea of static vs. dynamic configs and isolate those code paths a bit. Static === (local || csdl) configs where the gateway never updates.

Previously, we had loadServiceDefinitions overloaded to handle every type of config, but really it doesn't need to concern itself with the static cases. Those cases, I found, could actually be handled synchronously within the constructor!

The effect of these changes can be seen in the test updates that I had to make - the gateway was actually going through a full async update cycle even in the cases where that was totally unnecessary.

glasser

Wow, this is a lot more clear than the old version.

gateway-js/src/index.ts

glasser · 2021-01-28T00:21:51Z

gateway-js/src/index.ts

@@ -727,13 +793,40 @@ export class ApolloGateway implements GraphQLService {
      });
    }

+    const canUseManagedConfig =
+      this.apolloConfig?.graphId && this.apolloConfig?.keyHash;
    if (!canUseManagedConfig) {
      throw new Error(
        'When `serviceList` is not set, an Apollo configuration must be provided. See https://www.apollographql.com/docs/apollo-server/federation/managed-federation/ for more information.',


Error isn't quite right, since there are other approaches like csdl

Also added a test for this: 238fbec

Open to suggestions on rewording this, this one's kinda weird and, as demonstrated here, not the most obvious to keep up to date.

glasser · 2021-01-28T00:25:13Z

gateway-js/src/index.ts

+      this.maybeWarnOnConflictingConfig();
+    }
+
+    await this.updateComposition();


I wonder if it would be helpful to rename updateComposition and updateServiceDefinitions to contain "dynamic", to be more clear they are only part of the dynamic case? On the other hand maybe this is implicit in the concept of updating.

I'm open to this but the names will get to be on the "mouthful" side. I'm open to suggestions if you can think of something nice, otherwise will leave as is for now.

glasser · 2021-01-28T00:28:53Z

I do think there is something to be said for "any given field is either always or never assigned synchronously by the constructor" as kind of an abstract principle that this is going against. But I think practically speaking it's nice to be able to have things synchronously exist when possible.

Static init now happens under `load()` in a similar way to dynamic init. Additional cleanup / DRYing around these fns.

trevor-scheer · 2021-01-28T19:58:45Z

@glasser I think this was a great comment:

I do think there is something to be said for "any given field is either always or never assigned synchronously by the constructor" as kind of an abstract principle that this is going against. But I think practically speaking it's nice to be able to have things synchronously exist when possible.

Addressed via 525df37. Let me know if you meant something else, but this was my takeaway and I'm pretty happy with the result.

glasser

basically good, a few tiny suggestions?

glasser · 2021-01-28T20:13:13Z

gateway-js/src/index.ts

+      ({ schema, composedSdl } = this.createSchema(schemaConstructionOpts));
+    } catch (e) {
+      throw Error(
+        "A valid schema couldn't be composed. The following errors were found:\n" +


Is this error appropriate in the CSDL case? It wasn't there before this commit and the wording is a bit off for it.

(Though I do wonder why we need this try/catch anyway rather than have createSchema throw a useful error in the first place? What particular error is this trying to annotate?

You're correct, this.createSchema flat out won't throw in the CSDL case so I can go back to splitting the cases via two ifs.

This message precedes a list of graphql errors. We could just let that bubble up and not annotate it at all, or add this annotation over there instead?

Yeah I guess I'm wondering if adding it more tightly would help. It's an error about "composing", it could be thrown in the compose function or right when that is called?

This is a nice change and additionally gives us better control over how the list of composition errors is displayed (GraphQLSchemaValidationError forced multiple new lines between each error in the printing).
d66d683

Cool. I don't think I understand the implications of switching from GraphQLSchemaValidationError to just a plain Error (eg nothing checks error types when catching, right?) but trust that you do.

Yeah, this is a safe change (esp. since we were already just catching it and rethrowing as a plain Error).

There may have been some things to be careful of if it was public surface area / in use in other places, but not the case here.

glasser · 2021-01-28T20:16:54Z

gateway-js/src/index.ts

-    const mode = isManagedConfig(this.config) ? 'managed' : 'unmanaged';
+    // Handles initial assignment of `this.schema`, `this.queryPlannerPointer`
+    isStaticConfig(this.config)
+      ? this.loadStatic(this.config)


Is there a particular reason that loadStatic takes config as an argument and loadDynamic reads it from this? I mean it's not bad for them to have different interfaces, but if it's actually important that they differ I don't see why.

loadDynamic no longer reads from config at all, which is why the arg went away.

I prefer loadStatic to accept it as an argument since we can refine the type of config to StaticConfig on its way in, rather than do another isStaticConfig(this.config) once inside the function.

Ah, I see — loadDynamic calls functions which read from this.config but doesn't do so directly.

Actually, thinking about this makes me wonder if Experimental_UpdateServiceDefinitions should be changed to take a DynamicConfig instead of a GatewayConfig?

gateway-js/src/index.ts

glasser · 2021-01-28T22:45:06Z

gateway-js/src/index.ts

@@ -781,8 +781,7 @@ export class ApolloGateway implements GraphQLService {
      graphId: this.apolloConfig!.graphId!,
      apiKeyHash: this.apolloConfig!.keyHash!,
      graphVariant: this.apolloConfig!.graphVariant,
-      federationVersion:


That's cool!

glasser · 2021-01-28T22:48:02Z

gateway-js/src/index.ts

+      ({ schema, composedSdl } = this.createSchema(schemaConstructionOpts));
+    } catch (e) {
+      throw Error(
+        "A valid schema couldn't be composed. The following errors were found:\n" +


Cool. I don't think I understand the implications of switching from GraphQLSchemaValidationError to just a plain Error (eg nothing checks error types when catching, right?) but trust that you do.

The point of apollo-server-caching is to provide an abstraction over multiple cache backends. Gateway is not using that abstraction; it's just using one particular implementation, which is a wrapper around an old version of lru-cache. As part of apollographql/apollo-server#6057 and apollographql/apollo-server#6719 we want to remove dependencies on Apollo Server from Apollo Gateway. Technically we don't really need to remove this dependency (since apollo-server-caching doesn't depend on anything else in AS) but apollo-server-caching won't be updated any more (in fact, even in AS3 it has already been replaced by `@apollo/utils.keyvaluecache`), so let's do it. While we're at it, we make a few other improvements: - Ever since #440, the queryPlanStore field is always set, so we can remove some conditionals around it. - Instead of using the old lru-cache@6 wrapped by the apollo-server-caching package, we use the newer lru-cache@7 (which improves the algorithm internally and changes the names of methods a bit). - The get and set methods on InMemoryLRUCache were only async because they implement the abstract KeyValueCache interface: the implementations didn't actually do anything async. So we no longer need to await them or include a giant comment about how we're not awaiting them.

* gateway: remove dependency on apollo-server-caching (#2029) The point of apollo-server-caching is to provide an abstraction over multiple cache backends. Gateway is not using that abstraction; it's just using one particular implementation, which is a wrapper around an old version of lru-cache. As part of apollographql/apollo-server#6057 and apollographql/apollo-server#6719 we want to remove dependencies on Apollo Server from Apollo Gateway. Technically we don't really need to remove this dependency (since apollo-server-caching doesn't depend on anything else in AS) but apollo-server-caching won't be updated any more (in fact, even in AS3 it has already been replaced by `@apollo/utils.keyvaluecache`), so let's do it. While we're at it, we make a few other improvements: - Ever since #440, the queryPlanStore field is always set, so we can remove some conditionals around it. - Instead of using the old lru-cache@6 wrapped by the apollo-server-caching package, we use the newer lru-cache@7 (which improves the algorithm internally and changes the names of methods a bit). - The get and set methods on InMemoryLRUCache were only async because they implement the abstract KeyValueCache interface: the implementations didn't actually do anything async. So we no longer need to await them or include a giant comment about how we're not awaiting them. * gateway RemoteGraphQLDataSource: throw GraphQLError, not ApolloError (#2028) This is part of apollographql/apollo-server#6057 (which is itself part of apollographql/apollo-server#6719). We are trying to break the dependency of Gateway on Server so that (among other things) it is easier to have a single version of Gateway that works with both the current AS3 and the upcoming AS4. In AS4, we are removing the ApolloError class and its subclasses. Instead, we will just use GraphQLError directly. See: https://www.apollographql.com/docs/apollo-server/v4/migration#apolloerror https://www.apollographql.com/docs/apollo-server/v4/migration#built-in-error-classes apollographql/apollo-server#6355 apollographql/apollo-server#6705 This commit changes RemoteGraphQLDataSource to throw GraphQLError instead of ApolloError. The `code` extension will still be the same. (The `name` field of the thrown Error will no longer be eg `AuthenticationError`, though; this does not affect the error as serialized in GraphQL.) This is technically slightly backwards-incompatible (eg, the method errorFromResponse is public and now returns GraphQLError instead of the tighter ApolloError) but this doesn't seem likely to affect many users. We can adjust based on feedback if necessary. * Adjust #2028 for [email protected] compatibility

trevor-scheer added 2 commits January 27, 2021 09:20

Initial constructor simplification

673b8ad

Split static / dynamic schema code paths for simplicity

0c13c8a

trevor-scheer added the 🤗 improvement label Jan 27, 2021

trevor-scheer requested review from glasser and abernix January 27, 2021 16:34

trevor-scheer added 3 commits January 27, 2021 09:48

Address TODO: relocate config tests

843905c

Address TODO: test error case

0ac082a

Add missing type to GatewayConfig union

fca3e7a

glasser requested changes Jan 28, 2021

View reviewed changes

trevor-scheer added 7 commits January 28, 2021 11:42

Add intermediary GatewayConfig types for Static and Dynamic

af14289

Remove duplicate assignment

29d6dc2

Fix bug in query plan store construction

00a5eca

Remove extraneous ?

c72b288

Reorg static initialization

525df37

Static init now happens under `load()` in a similar way to dynamic init. Additional cleanup / DRYing around these fns.

Update error message and add test case to exercise this code path

238fbec

DRY load() fns and add comments

8eaac86

trevor-scheer requested a review from glasser January 28, 2021 19:58

glasser requested changes Jan 28, 2021

View reviewed changes

trevor-scheer added 4 commits January 28, 2021 14:16

Improve argument name for query plan store size

de97278

Merge branch 'main' into trevor/simplify-gateway-startup

78bdaf5

Tighten up GatewayConfig usages

6ddb960

Push error messaging up one level so we don't catch/rethrow

d66d683

trevor-scheer requested a review from glasser January 28, 2021 21:45

Workaround for error snapshot

d318891

trevor-scheer force-pushed the trevor/simplify-gateway-startup branch from 565f217 to d318891 Compare January 28, 2021 22:22

Move all config types and predicates to a separate file

5784be9

glasser reviewed Jan 28, 2021

View reviewed changes

glasser approved these changes Jan 28, 2021

View reviewed changes

Update changelog

b31330a

trevor-scheer merged commit 3b63c5d into main Jan 29, 2021

trevor-scheer deleted the trevor/simplify-gateway-startup branch January 29, 2021 01:31

trevor-scheer mentioned this pull request Feb 9, 2021

feat(gateway): Update managed federation to fetch "cloud config" #458

Merged

3 tasks

dependabot bot mentioned this pull request Mar 18, 2021

build(deps): bump @apollo/gateway from 0.20.0 to 0.24.4 rufman/graphql-mesh#55

Closed

glasser mentioned this pull request Aug 2, 2022

gateway: remove dependency on apollo-server-caching #2029

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(gateway): Simplify startup code paths #440

chore(gateway): Simplify startup code paths #440

trevor-scheer commented Jan 27, 2021

glasser left a comment

glasser Jan 28, 2021

trevor-scheer Jan 28, 2021

glasser Jan 28, 2021

trevor-scheer Jan 28, 2021

glasser commented Jan 28, 2021

trevor-scheer commented Jan 28, 2021

glasser left a comment

glasser Jan 28, 2021

trevor-scheer Jan 28, 2021

glasser Jan 28, 2021

trevor-scheer Jan 28, 2021

glasser Jan 28, 2021

trevor-scheer Jan 28, 2021

glasser Jan 28, 2021

trevor-scheer Jan 28, 2021

glasser Jan 28, 2021

trevor-scheer Jan 28, 2021

glasser Jan 28, 2021

glasser Jan 28, 2021

chore(gateway): Simplify startup code paths #440

chore(gateway): Simplify startup code paths #440

Conversation

trevor-scheer commented Jan 27, 2021

glasser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glasser commented Jan 28, 2021

trevor-scheer commented Jan 28, 2021

glasser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment