feat: Automatic schema reporting to Apollo Graph Manager by jsegaran · Pull Request #4084 · apollographql/apollo-server

jsegaran · 2020-05-08T07:03:09Z

Note: Schema reporting is not currently supported for managed federation

The Apollo Graph Manager schema registry is a free service of Apollo Graph Manager that allows tracking a graph (or variants of it) over time.

Previously, it was necessary to use the apollo schema:push command (usually in a CI pipeline) in order to push the latest state of the graph to Apollo Graph Manager and provide this visibility.

Apollo schema reporting protocol, is a new specification for GraphQL servers to automatically report schemas to the Apollo Graph Manager schema registry.

This PR implements this protocol for Apollo Server's reporting facilities and can be enabled by providing a Graph Manager API key (available from Apollo Graph Manager) in the APOLLO_KEY environment variable and setting the experimental_schemaReporting option to true in the Apollo Server constructor options, like so:

const server = new ApolloServer({
  typeDefs,
  resolvers,
  engine: {
    experimental_schemaReporting: true,
    /* Other existing options can remain the same. */
  },
});

When schema-reporting is enabled, Apollo server will send the running schema and server information to Apollo Graph Manager. The extra runtime information will be used to power auto-promotion which will set a schema as active for a variant, based on the algorithm described in the preview documentation.

When enabled, a schema reporting interval is initiated by the apollo-engine-reporting agent. It will loop until the ApolloServer instance is stopped, periodically calling back to Apollo Graph Manager to send information. The life-cycle of this loop is managed by the reporting agent.

Additionally, this PR deprecates schemaHash in metrics reporting in favor of using the same executableSchemaId used within this new schema reporting protocol.

Enrico2

The description here needs to be more detailed; this PR is doing 2 things, using the new schemaId field in metrics and adding support for schema reporting.

Had to stop mid-review, will come back after iteration..

abernix

I've left a few more asks within, but this is looking sharp.

abernix

I'll do some follow-up commits directly to this PR, but I think this mostly LGTM and is ready for an alpha.

abernix · 2020-05-19T07:59:10Z

  schema: string | GraphQLSchema,
 ): string {
+  // Can't call digest on this object twice. Creating new object each function call
+  const sha256 = module.require('crypto').createHash('sha256');


Note to self, I'll tweak this a bit before merging.

abernix · 2020-05-19T08:08:35Z

+      },
+    });
+
+    await expect(schemaReporter.reportServerInfo(false)).rejects.toThrow();


I will add a change that checks the error message contents here for specific errors.

Done with 3ddddd4

Having a code editor is nice.

The specificity of the `try`/`catch` around JSON parsing will ensure that HTML parsed as errors is insinuated clearly, whereas any general `fetch` error will propagate up to the error boundary provided by the `catch` that lives within the `reportingLoop` function after `schemaReporter.reportServerInfo` is invoked.

Ref: https://github.com/apollographql/apollo-server/pull/4084/files#r427109239 Ref: https://github.com/apollographql/apollo-server/pull/4084/files#r427109399

abernix · 2020-05-19T09:02:55Z

+      },
+    });
+
+    await expect(schemaReporter.reportServerInfo(false)).rejects.toThrow();


Done with 3ddddd4

Ref: #4084 (comment)

In the future, this might change back, but this can just be a top-level `import` right now. Ref: https://github.com/apollographql/apollo-server/pull/4084/files#r427103593

glasser

Looks great on the whole!

glasser · 2020-05-19T22:30:14Z

@@ -0,0 +1,64 @@
+/* tslint:disable */


Can you add an npm script to generate this file to package.json, or at least an explanation somewhere?

glasser · 2020-05-19T22:31:00Z

  endpointUrl?: string;
+  /**
+   * The URL to the Apollo Graph Manager ingress endpoint.
+   * (Previously, this was `endpointUrl`, which will be removed in AS3).


period goes inside parentheses

glasser · 2020-05-19T22:31:06Z

+   * **(Experimental)** Enable schema reporting from this server with
+   * Apollo Graph Manager.
+   *
+   * The use of this option avoids the need to rgister schemas manually within


glasser · 2020-05-19T22:36:39Z

+
+  /**
+   * The schema reporter waits before starting reporting.
+   * By default, the report waits some random amount of time between 0 and 10 seconds.


For what it's worth I find this approach to documenting the option a bit confusing.

The point is that there is a randomized delay, from 0 to 10 seconds. It always does that.

This lets you override the 10 second value, not the fact that it's a random amount of time.

This line

By default, the report waits some random amount of time between 0 and 10 seconds.
really should be more like
The report waits a random amount of time between 0 and (by default) 10 seconds.

It's also a bit odd for the first line of this whole comment to be a fact about how SR works rather than directly describing what the option does.

So something more like:

The maximum amount of time in milliseconds that the schema reporter waits before starting to report; defaults to 10 seconds.
The schema reporter waits a random amount of time up to this amount before reporting.
A longer interval leads to more staggered starts which means it is less likely
multiple servers will get asked to upload the same schema.

If this server runs in AWS Lambda or in other environments where processes are short-lived,
consider setting this value to a smaller number.

glasser · 2020-05-19T23:19:41Z

+  private readonly logger: Logger = console;
+  private readonly graphVariant: string;
+
+  private reports: { [executableSchemaId: string]: Report } = Object.create(


(note: i wrote all the below before I realized that the only thing you did to these objects is prettify them. oops. well, it's still true. and i still want to fix it but i don't want to ask you to, so, um, have #4139)

If I understand correctly, there's a strong requirement here that reports, reportSizes, and reportHeaders all have the same keys all the time, and there's a bunch of implicit dependencies on that fact.

Can you instead combine them into a single map whose value type is a tiny class with three fields?

Additionally, the type {[x: string]: Foo} is actually problematic. It tells TypeScript "no matter what string I index by, I'll get a Foo out". But that's not how this data structure actually works! All of these should be changed into {[x: string]: Foo | undefined}, which in fact does reveal a bunch of places where we fetch an object from them and don't check to see if it was actually there...

glasser · 2020-05-19T23:45:38Z

+export class SchemaReporter {
+  // These mirror the gql variables
+  private readonly serverInfo: EdgeServerInfo;
+  private readonly executableSchemaDocument: any;


can be string, no?

glasser · 2020-05-19T23:48:52Z

+    this.currentSchemaReporter = schemaReporter;
+    const logger = this.logger;
+
+    setTimeout(function() {


It's a little awkward that stop doesn't clearTimeout this timeout or any of the ones in reportingLoop; even if they won't do anything, you can imagine a context where this will require a program (like a test?) to stay running for up to 10 seconds before doing nothing! Leaking setTimeout return values isn't a great habit to get into anyway. Why not make inner from reportingLoop be a method on SchemaReporter, and save the return value from the active timeout on SchemaReporter, and clearTimeout on stop?

glasser · 2020-05-19T23:50:31Z

+  ): Promise<SchemaReportingServerInfoResult> {
+    const request: GraphQLRequest = {
+      query: reportServerInfoGql,
+      operationName: 'ReportServerInfo',


There's no need to include this unless you have more than one (named) operation in query.

glasser · 2020-05-19T23:50:44Z

+    const request: GraphQLRequest = {
+      query: reportServerInfoGql,
+      operationName: 'ReportServerInfo',
+      variables: variables,


can be just variables,

glasser · 2020-05-19T23:52:52Z

+        [
+          "Couldn't report server info to Apollo Graph Manager.",
+          'Parsing response as JSON failed.',
+          'If this continues please reach out to support@apollographql.com',


Putting just a space between our email address and the error will read strangely. Newline? Period?

jsegaran commented May 8, 2020

View reviewed changes

Comment thread packages/apollo-engine-reporting/src/agent.ts Outdated

jsegaran commented May 8, 2020

View reviewed changes

Comment thread packages/apollo-engine-reporting/src/schemaReporter.ts Outdated

zionts reviewed May 8, 2020

View reviewed changes

Comment thread packages/apollo-engine-reporting/src/schemaReporter.ts Outdated

Enrico2 suggested changes May 8, 2020

View reviewed changes

glasser reviewed May 12, 2020

View reviewed changes

Comment thread packages/apollo-engine-reporting/src/schemaReporter.ts

jsegaran commented May 12, 2020

View reviewed changes

Comment thread packages/apollo-engine-reporting/src/federatedPlugin.ts

jsegaran force-pushed the jsegaran/schema_reporting branch 2 times, most recently from 94eaeb5 to b3ca921 Compare May 13, 2020 13:17

jsegaran changed the base branch from abernix/migrate-engine-reporting-exts-to-plugin-api to release-2.14.0 May 13, 2020 15:06

jsegaran force-pushed the jsegaran/schema_reporting branch 11 times, most recently from 7a4e6ef to 30c11bc Compare May 14, 2020 15:30

jsegaran marked this pull request as ready for review May 14, 2020 16:13

jsegaran force-pushed the jsegaran/schema_reporting branch from 47da111 to 24bca7a Compare May 14, 2020 16:21

jsegaran requested review from Enrico2, glasser and zionts May 14, 2020 16:32

Joshua Segaran added 5 commits May 14, 2020 12:35

Initial Schema Reporting commit

ad3073e

Remove apollo link

764b1ce

Rename all schema hash and schema id to executable schema Id

67c57bf

Remove override executable schema id generator

de1523b

Add and fix tests

0413c20

jsegaran force-pushed the jsegaran/schema_reporting branch from f7d857d to 977a89e Compare May 18, 2020 02:30

abernix requested changes May 18, 2020

View reviewed changes

Joshua Segaran added 3 commits May 18, 2020 08:17

Parially address comments

f9751fe

Add error handling around http

2551f93

Adds test

490a02e

jsegaran requested a review from abernix May 18, 2020 13:49

Don't normalize GraphQL Schema in apollo-engine-reporting

ce2c8bf

abernix reviewed May 19, 2020

View reviewed changes

abernix added 6 commits May 19, 2020 11:11

Apply suggestions from my own code review

a25a785

Merge branch 'release-2.14.0' into jsegaran/schema_reporting

f08b1ca

Fix mistakes in my own GitHub suggestion.

5b8a766

Having a code editor is nice.

Tweak error message phrasing.

4836f75

Apply specificity to the errors we are expecting to encounter.

3ddddd4

Ref: https://github.com/apollographql/apollo-server/pull/4084/files#r427109239 Ref: https://github.com/apollographql/apollo-server/pull/4084/files#r427109399

abernix approved these changes May 19, 2020

View reviewed changes

abernix added 5 commits May 19, 2020 09:07

DRY up repeated error message for unexpected response shape.

003b435

Ref: #4084 (comment)

Update "Engine" reference to "Apollo Graph Manager".

cb56ca2

Fix my own markdown error.

e3b95e9

Just allow any type for the unexpected error msg.

8598437

Don't dynamically import crypto to use createHash.

bf5379a

In the future, this might change back, but this can just be a top-level `import` right now. Ref: https://github.com/apollographql/apollo-server/pull/4084/files#r427103593

abernix changed the title ~~Apollo schema reporting~~ feat: Automatic schema reporting to Apollo Graph Manager May 19, 2020

abernix merged commit 8b92145 into release-2.14.0 May 19, 2020

jsegaran mentioned this pull request May 19, 2020

docs+changelog: For #4084 and graphVariant. #4138

Merged

glasser reviewed May 19, 2020

View reviewed changes

abernix added a commit that referenced this pull request May 21, 2020

Editorial for #4084.

05dee50

abernix added a commit that referenced this pull request May 21, 2020

Hoist the CHANGELOG.md entry about #4084

c4921fa

abernix pushed a commit that referenced this pull request May 21, 2020

docs+changelog: For #4084 and graphVariant. (#4138)

23dfaca

nigelhanlon mentioned this pull request Jan 26, 2021

Implement Schema Reporting Protocol nearform/mercurius-apollo-registry#1

Closed

abernix deleted the jsegaran/schema_reporting branch February 5, 2021 06:57

github-actions Bot locked as resolved and limited conversation to collaborators Mar 16, 2023

Conversation

jsegaran commented May 8, 2020 • edited by abernix Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Enrico2 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

abernix left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

abernix left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

glasser left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

jsegaran commented May 8, 2020 •

edited by abernix

Loading