apollo-server-core: Update schema reporting plugin to support gateways

[sachindshinde]

To summarize, this PR:

• Updates Apollo Server to support a new schemaDidLoadOrUpdate event hook.
• Updates the Apollo Server schema reporting plugin to work when the server is configured with a gateway.

Note that this PR is dependent on apollographql/federation#738 :

• This PR has FIXMEs that require knowing the release version of gateway that lands the other PR.
• To use schema reporting with gateways, users must use a gateway with at least the above release version.

This PR specifically:

• Changes GatewayInterfaceto have a new method onSchemaLoadOrUpdate, for registering listeners that are notified on both schema load and schema update, and for providing the core schema in addition to the API schema.
  • This method is implemented in gateway-js: Add onSchemaLoadOrUpdate() method to register listeners for schema loads/updates and give them the core supergraph SDL federation#738 .
  • The onSchemaChange method has been marked deprecated and changed to optional, to support its eventual removal from the gateway API.
• Creates a new TS class SchemaManager for consolidating schema-tracking logic and abstracting away the gateway. This class specifically:
  • Acts as a source-of-truth for the schema and schema-derived data, updating it as needed.
  • Supports registering listeners for (and notifying listeners of) schema load/update events.
  • Is async-safe, i.e. it ensures linearizability for the TS class's public methods (provided start() is called and completes before stop()).
• Adds a new schemaDidLoadOrUpdate event hook so that plugins can listen to schema load/update events.
  • ApolloServerBase._start() registers such event hooks as schema load/update event listeners for the SchemaManager.
• Updates ApolloServerPluginSchemaReporting to use the new plugin event hook.
  • The hook, when called, creates a new SchemaReporter instance and starts it after stopping the old SchemaReporter instance (if it exists).
• Removes some states and properties from ServerState that are no longer necessary due to SchemaManager.
  • The two initialized states have been consolidated, and the invoking serverWillStart state has been removed.
  • The property schemaDerivedData has effectively been replaced by schemaManager, while loadedSchema has been removed entirely.
• Updates a test that was relying on a race condition between start() and stop() to resolve in a particular way to instead manually wait on start() before calling stop().
• Updates plugin docs, migration docs, and changelog.

[glasser]

I haven't reviewed the code yet, because your excellent PR description made it easy to reason about the design before doing so.

I think this is a bit of a non-starter:

Have an additional argument for specifying the gateway instance when using schema reporting in a federated graph.

While we can make ApolloServer pass gateway automatically when it creates the schema reporting plugin, I think requiring users to do it themselves when they are calling ApolloServerPluginSchemaReporting is too error-prone. If you leave it out then it just silently doesn't show updates? Maybe there's some way to log a warning or throw in that case but I'm not sure how.

I think something like this would be better:

We could make a new hook so that plugins can get access to the gateway instance, although I was hesitant to modify the plugin API to that extent (this is also just a use case of one plugin, so might be a bit hard to figure out the general shape at this point).

However instead of it being "provide gateway to plugin", I'd rather just introduce a new schemaDidChange callback on GraphQLServerListener, and make apollo-server invoke it on schema change.

(I'd suggest that it takes an object with keys as an argument rather than two positional arguments, to be consistent with the rest of the plugin API. So it will be shaped differently from onSchemaChange, but that's OK?)

(I think it's fine to "miss" changes that happen before serverWillStart — the plugin will get whatever the schema is at the time of serverWillStart, and any afterwards, and it's OK if it misses some schemas that predate it running. As long as no change after serverWillStart is missed!)

What do you think? Let me know if you'd like me to review the code now or wait for an update based on this (or if you think the current implementation is better of course).

[glasser]

If you'd like to factor this one out into its own PR we could get it merged fast!

[sachindshinde]

Sounds good, moved this change to #5222 🙂

[sachindshinde]

@glasser

While we can make ApolloServer pass gateway automatically when it creates the schema reporting plugin, I think requiring users to do it themselves when they are calling ApolloServerPluginSchemaReporting is too error-prone. If you leave it out then it just silently doesn't show updates? Maybe there's some way to log a warning or throw in that case but I'm not sure how.

Before this PR, the code in serverWillStart in packages/apollo-server-core/src/plugin/schemaReporting/index.ts would check whether the schema is federated via schemaIsFederated(), and throw if it was. This PR keeps that check, but only throws if no gateway instance was provided. (Sorry for any confusion here, I think it phrased it badly in the PR description, so I've updated it to clarify here.)

However instead of it being "provide gateway to plugin", I'd rather just introduce a new schemaDidChange callback on GraphQLServerListener, and make apollo-server invoke it on schema change.

I'd be down for introducing a new schemaDidChange callback for plugins to specify on GraphQLServerListener.

I'd suggest that it takes an object with keys as an argument rather than two positional arguments, to be consistent with the rest of the plugin API. So it will be shaped differently from onSchemaChange, but that's OK?

Yep, that sounds fine to me.

(I think it's fine to "miss" changes that happen before serverWillStart — the plugin will get whatever the schema is at the time of serverWillStart, and any afterwards, and it's OK if it misses some schemas that predate it running. As long as no change after serverWillStart is missed!)

Agreed here, should be fine to miss updates prior to serverWillStart running.

What do you think? Let me know if you'd like me to review the code now or wait for an update based on this (or if you think the current implementation is better of course).

I've updated this PR in the last seven commits to introduce a new schemaDidChange hook/callback, which takes in a GraphQLSchemaContext object with an API schema and an optional core schema SDL.

The gist of the changes is that:

• ApolloServerBase._start() adds any schemaDidChange plugin callbacks as listeners to gateway schema changes via onSchemaChange.
  • There's a bit of complexity here due to an edge-case where the gateway updates while serverWillStart is executing, in which case schemaDidChange may miss updates. To get around this, I added a listener to gateway schema changes to record the latest gateway schema, and manually call schemaDidChange with that schema before adding schemaDidChange as a listener.
    • This could be cleaned up slightly if we start tracking coreSchemaSdl in SchemaDerivedData, though I'm not sure if we want to start doing that.
  • There's a bit of thorniness in that errors thrown by schemaDidChange get handled by whatever notifies onSchemaChange listeners in gateway, although I'm not sure if there's a nice way around this here.
• In ApolloServerPluginSchemaReporting, we no longer take in a gateway. Most of the code of serverWillStart shifts into the schemaDidChange callback.
  • I've changed the second positional argument of SchemaChangeCallback to be optional as you've suggested here. This would mean that gateways being the wrong version results in a runtime error instead of a compile-time error, but IMO that's fine. (We can give better error messaging for runtime errors, and we already have a lot of runtime checks being done in ApolloServerBase._start() regardless so I don't think it'll be surprising.)
  • Note the code currently assumes onSchemaChange callbacks are called on the initial schema load. If that changes, I'll need to modify ApolloServerBase._start() a bit to use the output of load() when the onSchemaChange callback hasn't been called.
  • Note the code currently assumes schemaDidChange callbacks would be similarly called on initial schema load. If we want different semantics there, I'll need to modify serverWillStart's typings to pass an (optional) core schema SDL and the serverWillStart callback in ApolloServerPluginSchemaReporting to create a SchemaReporter.
    • There's a caveat here for schemaDidChange that we ought to make clear in the future docs, specifically that schema changes may be elided if there are newer changes that haven't been consumed. This would mean schemaDidChange may not technically be called on the initial schema load if there were new changes pending at the time. But this is fine for the single application we have (schema reporting), and it's not hard to change this behavior in the future if users request it. (We'd essentially keep a queue in the place of the latest gateway schema, and run through the queue before adding schemaDidChange as a listener for gateway schema changes.)

Let me know if this looks fine, if so I'll modify the PR description accordingly for the new approach 🙂

[glasser]

Can you leave a comment noting that this will only be left out with old versions of Gateway and that we should consider making this required for AS3 (which can definitely have a minimum version of Gateway)? (Though my long comment suggests we can probably make this required now.)

[sachindshinde]

This argument may still be undefined I think, specifically if schemaDidLoadOrUpdate is used by a plugin when a gateway isn't provided (which is the case for non-federated schema reporting).

[glasser]

Would it make the correctness a lot harder to make this method async (ideally just plain Promise async as opposed to the weirder ValueOrPromise)? Making all plugin methods async is in the air for AS3 (#4999). I can imagine it might make the logic harder to implement correctly.

[sachindshinde]

(Paraphrasing from a Slack convo we had with Trevor) We've decided to keep this sync, as there's concerns about listeners making expensive async calls stalling execution. If users want to do async things in response to a schema load/update, they can queue the changes and manage async queue consumption themselves.

[glasser]

I continue to not like the idea of calling schemaDidChange redundantly with the serverDidStart with the same schema. This ties in to what we are discussing with @trevor-scheer over at apollographql/federation#738 (comment)
I'd really prefer "did change" to actually mean "did change" and not "here is the same information we already gave you again".

If we're concerned about the serverWillStart overlapping with update thing, that makes sense: but I think we should make the change I want in 738 and then instead of asserting that latestApiSchema must be set, use whether or not it is set to call schemaDidChange only in the case of this overlap.

Here's a thought: what if in addition to gateway.onSchemaChange, we also allowed you to pass an onSchemaChange handler to gateway.load, with the semantics that this handler would be installed immediately after the initial load and it's gateway's job to make sure no changes are missed? Would that let us simplify things?

Or hmm.

Hmm.

OK, having looked at the whole PR, I do see that there is something simplifying about having a callback that is called every time the schema is updated, including the first time. Maybe my objection is just to the "changed" terminology. Can we come up with a more precise term that doesn't imply a change from a previous value? schemaDidLoad probably implies only the first time.

If we're doing that, we could consider changing #738 so that instead of adding a second argument to the callback, we just add a new onSchemaWhatever whose callback takes an object with keys instead of separate arguments.

And ooh! I think this will let us make the ApolloServer schemaDidWhatever take coreSupergraphSdl as a non-? argument. The idea is that we can move the runtime check into the schema loading code from the schema reporter. We can basically do:

• If any plugin registered schemaDidWhatever, and there is a gateway, then gateway.onSchemaWhatever must exist or it's an immediate startup error. (ie, it's an error to combine schema reporting with an old gateway, which shouldn't break any existing users since that's already an error.)
• However, if no plugin registered schemaDidWhatever, then we can fall back to gateway.onSchemaChange if onSchemaWhatever doesn't exist.

[sachindshinde]

I've changed the name to schemaDidLoadOrUpdate and changed apollographql/federation#738 to have a new onSchemaLoadOrUpdate method that takes an object with keys/properties.

And ooh! I think this will let us make the ApolloServer schemaDidWhatever take coreSupergraphSdl as a non-? argument.

I think that argument still ends up needing to be ? to be for the non-federated schema reporting case.

The idea is that we can move the runtime check into the schema loading code from the schema reporter. We can basically do:

• If any plugin registered schemaDidWhatever, and there is a gateway, then gateway.onSchemaWhatever must exist or it's an immediate startup error. (ie, it's an error to combine schema reporting with an old gateway, which shouldn't break any existing users since that's already an error.)

Sounds good, I've added a check for this near the end of _start(). Note that I'm beginning to think no error is currently thrown when trying to use the schema reporting plugin with a gateway (or at least, I don't see any code that throws when that's the case). However, our autoreg pipeline silently throws away reports from gateways.

• However, if no plugin registered schemaDidWhatever, then we can fall back to gateway.onSchemaChange if onSchemaWhatever doesn't exist.

We have to register gateway listeners early in _start() before we ask plugins whether they provide schemaDidLoadOrUpdate, so the new code just uses schemaDidLoadOrUpdate if available or falls back to onSchemaChange (regardless of plugin registration).

[glasser]

I'm pretty sure schemaIsFederated is looking for the case that this is a subgraph schema, so neither of the places you use this bool seem right.

Are we planning to do anything about schema reporting from subgraphs?

[sachindshinde]

You're right, good catch! I've reverted this part of the code, so that we use schemaIsFederated only to throw for schemas that look like implementing service schemas.

Are we planning to do anything about schema reporting from subgraphs?

I'm not in the loop enough about future plans there. For now I think we intend to keep the status quo, but we might start having reporting there in the future.

[glasser]

It seems like this needs to be better synchronized with the logic in the existing onSchemaChange. eg, we shouldn't call it if we don't actually update the schema because we're stopping or something, and we shouldn't call it until after the schema is actually updated (ie, schemaDerivedData is successfully changed). Probably this means we need to save the handlers somewhere and call them from the existing onSchemaChange.

[sachindshinde]

Agree here, I've changed _start() to add listeners to a set and the gateway listener created in startGatewayAndLoadSchema() to notify listeners in the set after the schema derived data is updated.

[sachindshinde]

@glasser @trevor-scheer
Update here, I've changed the PR to be more in line with what's mentioned above and in Slack. Specifically:

• The new hook has been renamed to didSchemaLoadOrUpdate, to better indicate that is called on both initial schema load and on schema updates.
• In startGatewayAndLoadSchema(), we use onSchemaLoadOrUpdate instead of onSchemaChange if available.
  • Previously, the gateway listener would only store the schema (and derived data) if the phase was started, but this leads to a potential issue where:
    • gateway.load() runs and returns the initial schema.
    • Somewhere in between then and the setting of the phase to started, an await occurs (e.g. executing hooks), halting execution.
    • Gateway's schema updates and notifies listeners, but our gateway listeners ignore the change because the state isn't started.
    • Execution resumes from the await, and the rest of _start() continues assuming the schema is that initial schema.
    • AS continues to believe the schema is the initial schema until the next gateway schema update, which may be arbitrarily far into the future. (Effectively, an update has been missed.)
  • This PR changes the listener to also store the schema if the phase is starting or invoking serverWillStart.
• We don't add multiple listeners to gateway, and instead add didSchemaLoadOrUpdate listeners to a set.
  • If a gateway is provided, and any plugin provides a didSchemaLoadOrUpdate listener, we throw in _start(), saying gateway must be updated.
  • Before adding a listener to the set, we call the listener with the latest schema in this.state.schemaDerivedData.
• The onSchemaLoadOrUpdate/onSchemaChange listener added in startGatewayAndLoadSchema() then forwards to the didSchemaLoadOrUpdate listeners in the set after storing the schema.

Once the code's approved, I'll update the PR description and the CHANGELOG.md.

[sachindshinde]

Update here on testing/dogfooding.

Gateway schema reporting is currently running in Studio staging, and appears to be working fine for both the old GCS pipeline and the new configdelivery pipeline.

I've locally tested gateways with a service list (to test the loadStatic() path) and plain no-gateway/monolith apollo server, and schema reporting is working there as well.

[glasser]

I mean, this complexity is exactly why I wanted to differentiate between initial load and later updates, and why my instinct was to move away from "use the same callback for initial load and later update". I feel like the old logic of "it's the job of gateway.load to ensure that at the moment it resolves its promise, that's the current schema, and as long as you don't yield between await load() and updating the state and starting to listen for changes, it works out.

I don't want to jerk you back and forth here... but I thought the point of doing onSchemaLoadOrUpdate was to make this stuff less complex, not more complex. This is really hard to follow right now.

[sachindshinde]

I mean, this complexity is exactly why I wanted to differentiate between initial load and later updates, and why my instinct was to move away from "use the same callback for initial load and later update". I feel like the old logic of "it's the job of gateway.load to ensure that at the moment it resolves its promise, that's the current schema, and as long as you don't yield between await load() and updating the state and starting to listen for changes, it works out.

I'm not sure separating initial load and later updates would help for this specific problem. Specifically, the following can happen:

• While executing _start(), await gateway.load() runs, which executes gateway.load().
• At some point during the execution of gateway.load(), it returns a promise that resolves to the initial schema, and execution suspends while waiting on that promise. Let's suppose the promise resolves at time A.
• At some point later (time B), _start() resumes execution (now that the promise has resolved, execution is unblocked).
• Between A and B, it's possible for other code to execute, including the code that updates the schema.

Basically it's hard to keep the initial schema non-stale when it's dynamic.

I don't want to jerk you back and forth here... but I thought the point of doing onSchemaLoadOrUpdate was to make this stuff less complex, not more complex. This is really hard to follow right now.

FWIW, I think once AS3 lands and we can require gateways to define onSchemaLoadOrUpdate, this changes to a:

this.state = {
  // ...
  schemaDerivedData: this.state.schemaDerivedData,
}

like the other changes to this.state, and this.state.schemaDerivedData effectively becomes the source of truth.

[glasser]

Maybe we could do a (noninvasive) "polyfill"? Like we can define a wrapper "gateway" which, if gateway.onSchemaLoadOrUpdate doesn't exist, we wrap around the gateway. It passes through methods other than onSchemaLoadOrUpdate and load, and handles onSchemaLoadOrUpdate by delegating to onSchemaChange and also keeping it around to call after load? So we could put all the back compat logic in this one wrapper class, and keep the main startup logic clean?

[sachindshinde]

@glasser I ended up going with something a bit more general than a polyfill here, it's at packages/apollo-server-core/src/utils/schemaManager.ts (it wraps gateway and provides onSchemaLoadOrUpdate, but it also wraps schemas in the non-gateway case and is responsible for updating schema-derived data).

[glasser]

To me, the difference between the "starting" and "invoking serverWillStart" states is that the latter has the (initial) schema set up and derived, and the former doesn't. I'm not sure what the point is of having them as separate states otherwise. I guess you could merge them. But really I think it shouldn't be rocket science for us to get it working with distinct states that represent distinct moments in moving forward?

[sachindshinde]

I ended up eliding out the starting state since it was never distinguished from invoking serverWillStart in the updated PR.

[glasser]

This is all far too complicated for me to understand or have any hope of successfully maintaining. Are there other approaches we can use to deal with the possibility of receiving schema change updates during startup?

For example, could we just buffer them? If a schema change comes in during 'invoking serverWillStart' we save it to some field (and I think it's fine if we just have one field rather than an array — if two schema changes come in we can just ignore the non-last one), and after serverWillStart is done we can fire off any buffered schema?

ie, that would let us just consistently use const schemaDerivedData for all relevant calls during the course of _start. And instead of changing the hook from "ignore in non-started state" to "ignore in all but three states", it would be "buffer in the starting/serverWillStart states, call in started, ignore otherwise".

[sachindshinde]

This is all far too complicated for me to understand or have any hope of successfully maintaining. Are there other approaches we can use to deal with the possibility of receiving schema change updates during startup?

In the refactor, I've tried to address this by confining most of this code to non-async functions, which I feel makes it easier to reason about since such functions can't be suspended during execution.

For example, could we just buffer them? If a schema change comes in during 'invoking serverWillStart' we save it to some field (and I think it's fine if we just have one field rather than an array — if two schema changes come in we can just ignore the non-last one), and after serverWillStart is done we can fire off any buffered schema?

I added some buffering in the SchemaManager class, there's more details about it in the SchemaManager.onSchemaLoadOrUpdate docstring.

[sachindshinde]

@glasser Finally got some time to come back to this. I've wrapped all modes of operation (old gateway, new gateway, and non-gateway) into a class SchemaManager that:

• Provides a onSchemaLoadOrUpdate() method for listener registration that's relatively consistent across modes. (It also buffers the latest event for new listeners).
• Tracks all schema-derived data, updating it as needed via a provider callback passed in at construction.
• Confines most of the tricky code to non-async functions, which I've generally found makes reasoning about correctness a lot easier.

After moving schemaDerivedData, I was able to consolidate the two initialized states together, and consolidate the starting and invoking serverWillStart states together.

Let me know if this looks mostly fine, and if so I'll update the PR description.

[glasser]

Can you rebase onto release-3.0? Not planning any more AS2 releases.

[sachindshinde]

@glasser Sounds good, I've rebased this branch on release-3.0 and squashed a bunch of old commits away so the commit history should be cleaner.

[glasser]

Looks great. Let's make apollographql/federation#738 mergeable before we merge this and merge them both when we're both free together.