gateway-js: Add onSchemaLoadOrUpdate() method to register listeners for schema loads/updates and give them the core supergraph SDL #738

sachindshinde · 2021-05-06T02:22:30Z

For gateway schema reporting, we would like to know the (core) supergraph SDL when the schema is loaded or updates. To that end, this PR:

Adds a new public method onSchemaLoadOrUpdate() that registers listeners for schema loads/updates.
- Unlike onSchemaChange(), this method ensures listeners are called during initial schema load.
  - Specifically, onSchemaChange() would not notify listeners when the schema is loaded via loadStatic(), but onSchemaLoadOrUpdate() does notify.
- Unlike onSchemaChange(), this method provides listeners with both the API schema and the core supergraph SDL.
- Unlike onSchemaChange(), this method provides listeners data via properties on a single argument object instead of multiple arguments.
Deprecates onSchemaChange() in favor of onSchemaLoadOrUpdate().
Factors out some common logic into updateWithSchemaAndNotify().
Changes the notification process to notify all listeners even when one throws (previously, any listener throwing would prevent other listeners from being notified).
Runs prettier on gateway-js/src/index.ts.
Updates the gateway-js/CHANGELOG.md.

glasser · 2021-05-06T04:53:30Z

Is there a high level design of "gateway schema reporting" available?

sachindshinde · 2021-05-07T05:55:57Z

@glasser
There's Josh's quip doc "Schema Reporting from the Gateway", but I've been told it's out-of-date. (The rather big change is that we're not changing the API for reportServerInfo, and instead just passing the supergraph core SDL as the executableSchema.)

I've made the PR to update the plugin at apollographql/apollo-server#5187 ; that PR's description goes more into detail about the approach used.

glasser · 2021-05-12T21:46:44Z

gateway-js/src/index.ts

@@ -40,7 +40,11 @@ import { getVariableValues } from 'graphql/execution/values';
 import fetcher from 'make-fetch-happen';


For what it's worth: I'm personally a big fan of prettier, but others who work more closely on this project are less so (or at least, about introducing prettier-only changes). Definitely don't squash this PR when you merge it. I think @trevor-scheer works on this file most closely (eg, maybe he has some outstanding changes this would clash with) — Trevor, do you think having a prettier commit in this PR is helpful or not?

Either is ok with me. I'm personally a fan of it, but the state of prettier in this repo (unenforced) puts me on the fence and I don't feel too strongly in any case.

Same, I'm also a fan of prettier. I'd be fine with keeping the prettier commit in the PR, but unsquashed as @glasser suggested. (If we do have a large rewrite pending that clashes, let me know and I can drop the commit.)

glasser · 2021-05-12T21:54:41Z

gateway-js/src/index.ts

    // TODO(trevor): #580 redundant parse
    this.parsedSupergraphSdl = parse(supergraphSdl);
-    this.queryPlanner = new QueryPlanner(schema);
+    this.updateWithSchemaAndNotify(schema);


I think it's a good idea to make loadStatic and loadDynamic more consistent, but I think this change may be in the wrong direction. I think it would make more sense if updateSchema didn't call the listeners when invoked via loadDynamic (and only when invoked on polling). ie, the onSchemaChange callback should truly be about schema change, not also the initial load. (It might be reasonable to do it this way if load() didn't return a schema and onSchemaChange was the only way the schema was delivered, but otherwise I don't think we need a no-op onSchemaChange on every startup.)

I think that it's a no-op in the full system including apollo-server, because apollo-server's onSchemaChange callback (at least as of v2.22) does nothing if the server is not yet in the started state.

@trevor-scheer do you agree?

(Factoring out the listener invocation code seems reasonable either way. I believe we left a fair amount of copy-and-paste because we expected to be able to get rid of some of the code soon, but maybe that isn't the case any more.)

Hmm, I think the bit about load is a fair point but our current API encourages users to specifically not handle calling load themselves and to let AS handle that. Still totally fine to do it the "old way", but at this point I think it's undocumented.

Is there a reason you think it's bad to call the update listeners on initial load? Noisy/not semantic maybe? I can appreciate that viewpoint.

I mean, I agree that this API is for use by AS, and AS explicitly doesn't care about getting onSchemaChange callbacks called on the initial load (its callback does nothing if the server isn't started yet). So the fact that it's for use by AS suggests that if we want to resolve the inconsistency in the current behavior (which I think is a good idea) we should do it in the direction of fewer redundant calls rather than more?

onSchemaChange is not necessarily just for AS, that could reasonably be used in userland for observability (though I see now it's undocumented). We use it for promise driven tests, so we may need to update some if its behavior is changed.

I definitely agree we should make it consistent. I also agree less redundant calls is better, though I'm not convinced this is redundant (for the userland use case).

@trevor-scheer OK, I'm back from the long weekend, let's resolve this :) Sounds like we agree it should be consistent.

My instinct is that calling the onSchemaChange before load resolves with the original schema seems odd to me, and not a great fit for the "change" in its name. So I lean towards not calling it for the original load. But this is an area you are more deeply involved with, so how about you choose which way to make it consistent?

I agree with @trevor-scheer here. While our own code only uses onSchemaChange() for schema reporting, it's part of the public API (albeit undocumented) and load() is expected to be called by Apollo Server, so I'd err on the side of exposing the initial schema load to onSchemaChange callbacks.

See somewhat rambly apollographql/apollo-server#5187 (comment) for an idea of how to resolve this: basically, I think y'all are right that calling it always is reasonable, but let's make a more clear name for it. Coming up with a new name also gives us the opportunity to make this callback take an object rather than positional arguments, and to more easily detect whether the new version of the gateway is running.

(If you go through with that plan, then I think it's fine to consistently call callbacks registered either via the new name or the old name with the first schema.)

glasser · 2021-05-12T22:06:34Z

gateway-js/src/index.ts

@@ -703,7 +702,7 @@ export class ApolloGateway implements GraphQLService {
    };
  }

-  public onSchemaChange(callback: SchemaChangeCallback): Unsubscriber {
+  public onSchemaChange(callback: (schema: GraphQLSchema, supergraphSdl: string) => void): Unsubscriber {


This seems like a backwards-incompatible change (for TypeScript users) where existing Apollo Server won't be able to pass in the function it currently passes in. Though maybe I'm wrong due to how assignability works in TypeScript. Perhaps you could write a tiny test that shows that passing in a single-arg callback still compiles?

Hmm. OK, I played around and you're right that this won't break using old AS with new Gateway. But I think the other way around may be a challenge: passing a two-arg function to an old Gateway that expects one arg will not compile. And in fact that makes sense: if you use new AS with old Gateway you will have code in your callback that expects to get two strings but the second one won't be there.

I suggest that we define this callback type as (schema: GraphQLSchema, supergraphSdl?: string) => void, and in apollo-server you check if supergraphSdl is defined and blow up with some error about needing to upgrade @apollo/gateway if it isn't there (or do some other behavior that is ok with no supergraphSdl).

Do you intend to eventually migrate the SchemaChangeCallback type? Leaving it around indefinitely with the wrong typing sounds confusing. Not sure exactly what the constraints here are.

Ah you allude to this in apollographql/apollo-server#5187

This is dependent on #738 . Specifically, when this PR lands, users will need to have access to a version of @apollo/gateway containing the referenced PR, or else their TS presumably won't compile (specifically, I believe they'll observe that ApolloGateway won't implement the interface GraphQLService, although I haven't checked this yet).

I see that that PR updates the SchemaChangeCallback type too.

I think you're right about what the problem will be. Making the second arg optional would help here. (Or making an entirely new callback type and onSchemaChangeWithSupergraphSdl entry point! I think second arg optional is better though.)

I'm down with making the second positional argument in SchemaChangeCallback optional, and having the check essentially be a runtime check instead of a compile-time check. We can give better messaging with a runtime check, and we do a fair number of runtime checks already at gateway start so it's not unexpected. I've updated apollographql/apollo-server#5187 to make the argument optional.

sachindshinde · 2021-05-20T00:05:32Z

@glasser @trevor-scheer
Update here, following the feedback given on PRs and in Slack, this PR has been changed to:

Not change any behavior for onSchemaChange() (i.e. still one argument for the callback, and still no notification on initial schema load for loadStatic()).
- Common logic has still been refactored out though into updateWithSchemaAndNotify().
Add a new method onSchemaLoadOrUpdate() that:
- Is notified on both initial schema load and later schema updates.
- Passes a single object type to the callback instead of positional arguments.
- Passes both the core supergraph SDL and the API schema to the callback.
Mark onSchemaChange() as deprecated, in favor of onSchemaLoadOrUpdate().
If a listener throws, other listeners will still execute. (Previously, one listener throwing made other listeners not execute.)

If this sounds fine, let me know and I'll update the PR description and CHANGELOG.md.

glasser

This looks right. Do be sure to update the PR description (and eventual squash commit) to describe what the latest version does, and the CHANGELOG as you mentioned in it.

Re the two comments below: mostly just take a glance to make sure your PR hasn't made anything worse (eg made it more likely that an error could lead to those fields getting out of sync with the actual schema), and perhaps add some TODO comments? I mean if you're inspired to introduce SchemaDerivedData I wouldn't complain but I'm sure you want to merge too.

glasser · 2021-05-20T22:55:42Z

gateway-js/src/index.ts

@@ -389,10 +399,9 @@ export class ApolloGateway implements GraphQLService {
      throw e;
    }

-    this.schema = schema;
    // TODO(trevor): #580 redundant parse
    this.parsedSupergraphSdl = parse(supergraphSdl);


thought: should this be part of updateWithSchemaAndNotify too?

Hmmm, looking at the code there's two things that make me a bit wary about moving it into updateWithSchemaAndNotify():

Based on this block:
// TODO(trevor): #580 redundant parse // This may throw, so we'll calculate early (specifically before making any updates) // In the case that it throws, the gateway will: // * on initial load, throw the error // * on update, log the error and don't update const parsedSupergraphSdl = parse(result.supergraphSdl);
It looks like we parse early to avoid accidental state update (we could double-parse, but I think we've been trying to avoid redundant parses).

We don't update this.parsedSupergraphSdl in updateByComposition(), which initially gave me the impression we only update it when we're handed a core supergraph schema directly and not when we get a service list. But looking at loadStatic(), we do actually update this.parsedSupergraphSdl when running this.createSchemaFromServiceList(), so I'm not sure why we don't update it in updateByComposition(). Maybe @trevor-scheer might know more here?

glasser · 2021-05-20T22:56:39Z

gateway-js/src/index.ts

-            ((e && e.message) || e),
-        );
-      }
+      this.updateWithSchemaAndNotify(schema, supergraphSdl);


Thought: should the this.queryPlanStore.flush() above be part of updateWithSchemaAndNotify too? It would seem you would want to do that as close as possible to changing the current schema. (Though I do feel like moving to more of a "schema derived data" that is swapped out at once like in ApolloServer would solve both of these thoughts.)

Yep, I'm down to move it into updateWithSchemaAndNotify(). We weren't doing it before in loadStatic(), but that's probably because it's only called at initial schema load, so it just ends up being a no-op.

I agree that "schema derived data" would be nice in gateway, although I'll leave that to future PRs. (I've added a TODO about it.)

sachindshinde · 2021-05-22T00:37:11Z

I've updated the PR description and the CHANGELOG.md for the new changes.

I won't merge this PR in until apollographql/apollo-server#5187 gets approved. (There's a somewhat lingering question above about this.parsedSupergraphSdl, but no behavior is changing in this PR with regards to it, so I figure we can handle it later if it's not something simple.)

sachindshinde · 2021-06-25T05:17:00Z

Note that there will be a slight merge-conflict with #807 (shouldn't be hard to fix, but worth noting).

…x bug where loadStatic() doesn't notify listeners

…d revert addition of optional argument and behavior change to loadStatic() for onSchemaChange()

…nd onSchemaLoadOrUpdate

glasser

I did a quick re-skim and this looks good!

#5187) 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 `GatewayInterface`to 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 apollographql/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.

WUTICHAI5661102493

Th

sachindshinde requested review from trevor-scheer and glasser May 6, 2021 02:22

sachindshinde mentioned this pull request May 7, 2021

apollo-server-core: Update schema reporting plugin to support gateways apollographql/apollo-server#5187

Merged

glasser requested changes May 12, 2021

View reviewed changes

sachindshinde force-pushed the sachin/add-supergraph-sdl-to-schema-change-listeners branch from 10bdc9e to e32bd44 Compare May 20, 2021 00:13

sachindshinde requested a review from glasser May 20, 2021 00:16

glasser approved these changes May 20, 2021

View reviewed changes

sachindshinde changed the title ~~gateway-js: Ensure loadStatic() notifies schema change listeners and give listeners the supergraph SDL~~ gateway-js: Add onSchemaLoadOrUpdate() method to register listeners for schema loads/updates and give them the core supergraph SDL May 24, 2021

sachindshinde force-pushed the sachin/add-supergraph-sdl-to-schema-change-listeners branch 2 times, most recently from 4ca97d3 to 571c6de Compare June 22, 2021 14:09

sachindshinde force-pushed the sachin/add-supergraph-sdl-to-schema-change-listeners branch from 571c6de to bd26ee1 Compare June 25, 2021 05:15

sachindshinde force-pushed the sachin/add-supergraph-sdl-to-schema-change-listeners branch from bd26ee1 to bdf77a3 Compare July 7, 2021 15:48

sachindshinde added 3 commits July 22, 2021 14:46

Factor out common code between loadStatic() and loadDynamic(), and fi…

56666fa

…x bug where loadStatic() doesn't notify listeners

Pass supergraph SDL to schema change listeners

b80671e

Run prettier

f2f1d60

glasser changed the base branch from main to release-gateway-0.35.0 July 22, 2021 18:59

sachindshinde added 5 commits July 22, 2021 15:05

Add new method onSchemaLoadOrUpdate() to replace onSchemaChange(), an…

8b95c72

…d revert addition of optional argument and behavior change to loadStatic() for onSchemaChange()

Move queryPlanStore.flush() to inside updateWithSchemaAndNotify()

5285993

Update CHANGELOG.md

882a8e3

Add TODO about "schema derived state"

459e713

Update CHANGELOG.md to highlight discrepancy between onSchemaChange a…

c80ac2b

…nd onSchemaLoadOrUpdate

sachindshinde force-pushed the sachin/add-supergraph-sdl-to-schema-change-listeners branch from bdf77a3 to c80ac2b Compare July 22, 2021 19:10

glasser approved these changes Jul 22, 2021

View reviewed changes

sachindshinde merged commit 502efb7 into release-gateway-0.35.0 Jul 22, 2021

sachindshinde deleted the sachin/add-supergraph-sdl-to-schema-change-listeners branch July 22, 2021 20:02

WUTICHAI5661102493 reviewed Jul 22, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gateway-js: Add onSchemaLoadOrUpdate() method to register listeners for schema loads/updates and give them the core supergraph SDL #738

gateway-js: Add onSchemaLoadOrUpdate() method to register listeners for schema loads/updates and give them the core supergraph SDL #738

sachindshinde commented May 6, 2021 •

edited

Loading

glasser commented May 6, 2021

sachindshinde commented May 7, 2021

glasser May 12, 2021

trevor-scheer May 12, 2021

sachindshinde May 19, 2021

glasser May 12, 2021

trevor-scheer May 12, 2021

glasser May 12, 2021

trevor-scheer May 13, 2021

glasser May 18, 2021

sachindshinde May 19, 2021

glasser May 19, 2021 •

edited

Loading

glasser May 12, 2021

glasser May 12, 2021

sachindshinde May 19, 2021

sachindshinde commented May 20, 2021 •

edited

Loading

glasser left a comment

glasser May 20, 2021

sachindshinde May 21, 2021

glasser May 20, 2021

sachindshinde May 21, 2021 •

edited

Loading

sachindshinde commented May 22, 2021

sachindshinde commented Jun 25, 2021

glasser left a comment

WUTICHAI5661102493 left a comment

		@@ -40,7 +40,11 @@ import { getVariableValues } from 'graphql/execution/values';
		import fetcher from 'make-fetch-happen';

gateway-js: Add onSchemaLoadOrUpdate() method to register listeners for schema loads/updates and give them the core supergraph SDL #738

gateway-js: Add onSchemaLoadOrUpdate() method to register listeners for schema loads/updates and give them the core supergraph SDL #738

Conversation

sachindshinde commented May 6, 2021 • edited Loading

glasser commented May 6, 2021

sachindshinde commented May 7, 2021

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

glasser May 19, 2021 • edited Loading

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

sachindshinde commented May 20, 2021 • edited Loading

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

sachindshinde May 21, 2021 • edited Loading

Choose a reason for hiding this comment

sachindshinde commented May 22, 2021

sachindshinde commented Jun 25, 2021

glasser left a comment

Choose a reason for hiding this comment

WUTICHAI5661102493 left a comment

Choose a reason for hiding this comment

sachindshinde commented May 6, 2021 •

edited

Loading

glasser May 19, 2021 •

edited

Loading

sachindshinde commented May 20, 2021 •

edited

Loading

sachindshinde May 21, 2021 •

edited

Loading