Make communicating with Apollo's commercial products into ordinary plugins by glasser · Pull Request #4453 · apollographql/apollo-server

glasser · 2020-08-04T22:03:16Z

Currently, the EngineReportingAgent is deeply embedded in ApolloServer, configured via a special engine option to new ApolloServer. Additionally, Engine is no longer the name of the product.

This PR will remove this special-casing and replace EngineReportingAgent with a few standard ApolloServerPlugins. It will replace the engine constructor argument with an apollo argument that's used only to configure the API key and graph ID/variant, not for detailed configuration of reporting semantics. Other than this apollo argument (which will also be passed to the plugin's serverWillStart hook) and backwards-compatibility handling, the only special treatment of these Apollo platform plugins will be that if you do actively configure an Apollo API key and don't set up a usage reporting plugin yourself or actively disable it, a plugin will be set up with the default configuration.

glasser · 2020-08-04T22:03:53Z

One note: I definitely want to make the tests not print a zillion No graph variant provided. Defaulting to current. warnings! Will get to that next.

glasser · 2020-08-05T06:20:41Z

Added a small number of explicit graphVariants to silence those warnings.

glasser · 2020-08-05T15:38:27Z

Naming thought: ApolloConfigInput becomes ApolloConfig, ApolloConfig becomes ApolloContext?

glasser · 2020-08-05T17:04:35Z

Updated this PR's description to be broader than the first step (the existing commit's description maintains) and added a checklist.

glasser · 2020-08-05T23:27:38Z

I'm kinda unsure about naming for the plugins. Our whole-package plugins export them as default exports, so they don't really have names.

trevor-scheer · 2020-08-06T21:27:16Z

          headers: {
            'user-agent': 'apollo-engine-reporting',
-            'x-api-key': this.apiKey,
+            'x-api-key': this.apolloConfig.key!,


Non-blocking / nit: this.apolloConfig is actually more of a WithRequired<ApolloConfig, 'key'> due to the assertion in the constructor. Refining that type would mean we don't have to assert its existence here or anywhere else it's being used.

trevor-scheer · 2020-08-06T21:38:15Z

+      // mode and warn about deprecation
+      if (apollo) {
+        throw new Error("You cannot provide both `engine` and `apollo` to `new ApolloServer()`. " +
+     "For details on how to migrate all of your options out of `engine`, see FIXME(no-engine) URL MISSING");


I know this is WIP, just noting FIXMEs for later.

Thanks. It's in the PR description checklist too.

trevor-scheer · 2020-08-06T21:42:30Z

+  // FIXME(no-engine): This should be changed to check for a deprecation
+  // warning for any use of `engine` (which we can't really do until splitting
+  // out the plugins).
+  it.skip('spits out a deprecation warning when passed a schemaTag in construction', () => {


Noting FIXME

trevor-scheer · 2020-08-06T21:47:16Z

@@ -112,32 +116,47 @@ describe('environment variables', () => {
  it('constructs a reporting agent with the ENGINE_API_KEY (deprecated) environment variable and warns', async () => {
    // set the variables
    process.env.ENGINE_API_KEY = 'just:fake:stuff';


Nothing you've done here, but is this polluting the global test env? Do we need to delete it as cleanup? I'm actually not sure if jest persists process.env between tests.

I had the same thought, but there's a beforeEach/afterEach above.

@trevor-scheer fwiw, I think process.env within the same Jest test-runner worker would be persisted (i.e., would not be reset to its state at startup), yes. (But the beforeEach / afterEach is the key here.)

trevor-scheer · 2020-08-06T21:48:06Z

    process.env.ENGINE_API_KEY = 'just:fake:stuff';
    process.env.APOLLO_KEY = 'also:fake:stuff';


Same concern here, maybe.

trevor-scheer · 2020-08-06T22:00:37Z

+      throw new Error(
+        'Cannot set more than one of apollo.graphVariant, ' +
+          'engine.graphVariant, and engine.schemaTag. Please use apollo.graphVariant.',
+      );
+    }
+    apolloConfig.graphVariant = engine.graphVariant;
+  } else if (typeof engine === 'object' && engine.schemaTag) {
+    // No need to warn here, because ApolloServer's constructor should warn about
+    // the existence of `engine` at all.
+    apolloConfig.graphVariant = engine.schemaTag;
+  } else if (APOLLO_GRAPH_VARIANT) {
+    if (ENGINE_SCHEMA_TAG) {
+      throw new Error(
+        '`APOLLO_GRAPH_VARIANT` and `ENGINE_SCHEMA_TAG` (deprecated) environment variables must not both be set.',
+      );


Do we want to warn and carry on gracefully in these throw cases? Or are we throwing because if the values are different we don't know which is the right one to pick?

I know this complicates the logic a bit but we could carry on if:

they're identical (safe)

we just decide that newer config option takes more precedence (arguably less safe)

This throw is preserving existing behavior. Has anyone complained about it yet?

In general I think "make sure untouched configuration still works" is a good goal but "let people write confusing config that is half old and half new" isn't positive.

I think the half-old/half-new requirement was because of the apollo CLI (or other tooling) respecting one set of environment variables and Server respecting something different. We can probably leave this now that I think the CLI is sorted out.

trevor-scheer · 2020-08-06T22:11:22Z

+        );
+      }
+
+      const executableSchema = overrideReportedSchema ?? printSchema(schema);


Executable schema generally means an actual GraphQLSchema, so the naming here is a bit misleading. printedSchema?

For some reason, executableSchema is the name of the parameter in our GraphQL API. So I think matching is good?

trevor-scheer · 2020-08-06T22:14:39Z

+      const schemaReporter = new SchemaReporter({
+        serverInfo,
+        schemaSdl: executableSchema,
+        apiKey: apollo.key!,


Seems like this shouldn't be necessary, we assert apollo.key's existence above.

It is necessary (presumably TypeScript isn't positive that apollo.key isn't from some sort of getter?) but I can fix it in another way by saving to a const.

trevor-scheer · 2020-08-06T22:20:35Z

@@ -0,0 +1,271 @@
+// This class is a helper for ApoloServerPluginUsageReporting and


Suggested change

// This class is a helper for ApoloServerPluginUsageReporting and

// This class is a helper for ApolloServerPluginUsageReporting and

abernix

Thank you so much for putting in the effort to organize and de-Engine-ize this code!

I've left a number of comments throughout, but I think my notable points are around the:

Un-conditional importing of modules which were previously avoided when Apollo Cloud services were disabled. This includes Node.js built-in APIs (like crypto, zlib, os) but also heavier dependencies, like apollo-reporting-protobuf. Not only had we switched to conditionally importing these some time ago (to support non-Node.js runtimes, like JavaScriptCore and V8), but also to improve the cold-boot time in Worker/Edge/Lambda-type environments.
The __internal_plugin_id__ (and if there is another way)

abernix · 2020-08-21T18:50:54Z

@@ -112,32 +116,47 @@ describe('environment variables', () => {
  it('constructs a reporting agent with the ENGINE_API_KEY (deprecated) environment variable and warns', async () => {
    // set the variables
    process.env.ENGINE_API_KEY = 'just:fake:stuff';


@trevor-scheer fwiw, I think process.env within the same Jest test-runner worker would be persisted (i.e., would not be reset to its state at startup), yes. (But the beforeEach / afterEach is the key here.)

abernix · 2020-08-21T18:58:16Z

+      throw new Error(
+        'Cannot set more than one of apollo.graphVariant, ' +
+          'engine.graphVariant, and engine.schemaTag. Please use apollo.graphVariant.',
+      );
+    }
+    apolloConfig.graphVariant = engine.graphVariant;
+  } else if (typeof engine === 'object' && engine.schemaTag) {
+    // No need to warn here, because ApolloServer's constructor should warn about
+    // the existence of `engine` at all.
+    apolloConfig.graphVariant = engine.schemaTag;
+  } else if (APOLLO_GRAPH_VARIANT) {
+    if (ENGINE_SCHEMA_TAG) {
+      throw new Error(
+        '`APOLLO_GRAPH_VARIANT` and `ENGINE_SCHEMA_TAG` (deprecated) environment variables must not both be set.',
+      );


I think the half-old/half-new requirement was because of the apollo CLI (or other tooling) respecting one set of environment variables and Server respecting something different. We can probably leave this now that I think the CLI is sorted out.

abernix · 2020-08-21T19:46:03Z

+      const { cache: apqCache = requestOptions.cache!, ...apqOtherOptions } =
+        requestOptions.persistedQueries || Object.create(null);


Looks a lot like Prettier ran on this file and made a bunch of changes. This one in particular certainly is questionable.

Ah sorry, yeah, I am planning to de-prettier files that are not new (however, I am using prettier on all new files because as a multi-lingual developer I no longer possess the ability to use my brain to make choices about formatting in N different languages).

PR is now de-prettiered (or well, the part outside of core/src/plugin).

abernix · 2020-08-21T19:47:27Z

-          const message = "This data graph is missing a valid configuration.";
-          this.logger.error(message + " " + (err && err.message || err));
+          const message = 'This data graph is missing a valid configuration.';
+          this.logger.error(message + ' ' + ((err && err.message) || err));
          throw new Error(
-            message + " More details may be available in the server logs.");


Not sure how you feel about backing these out, but it would be nice.

abernix · 2020-08-21T20:42:40Z

+import os from 'os';
+import { gzip } from 'zlib';
+// FIXME(no-engine): make sure these deps are declared
+import retry from 'async-retry';
+import { defaultEngineReportingSignature } from 'apollo-graphql';
+import {
+  Report,
+  ReportHeader,
+  Trace,
+  TracesAndStats,
+} from 'apollo-reporting-protobuf';


Just my same concern about Node.js APIs being now in the direct line of imports / module resolution.

glasser · 2020-08-26T00:33:12Z

@abernix This is moderately close to "code complete" (I've finished and fixed up the commit that gets rid of apollo-engine-reporting), though there are still a number of things to track down, including of course docs/changelog/migration notes.

One thing I just learned is how the Lambda/Cloud Functions/Azure ApolloServer constructors default engine.sendReportsImmediately to true. I think a better way to implement this would be:

ApolloServer defines an overloadable boolean method with some name like serverLifetimeIsShort() (this is not a very good name). The base class version returns false.
The Lambda etc ApolloServer objects override these to return true.
We add a boolean of the same name to GraphQLServiceContext (ie, the argument to serverWillStart)
The usage reporting plugin defaults sendReportsImmediately to true if it is not explicitly specified and if serverLifetimeIsShort

Does that sound like a reasonable thing to have in the API? I think it may be helpful to other plugins as well. Do you have a better name in mind?

This option was previously not explicitly documented except on the plugins page. Add a basic description and mention that the built-in plugins enable themselves in some contexts. (I didn't mention that the JSON tracing and cache-control plugins work the same way; the fact that those features are implemented as plugins is an internal implementation as those plugins are not directly exposed to users.) There are many other ApolloServer options not mentioned on this reference page! In case you are curious, they are: - cache - cacheControl - dataSources - executor - extensions - fieldResolver - gateway - modules - parseOptions - tracing - uploads

I dropped the reference to debugPrintReports (which is still in the plugin reference) because it barely works (the actual traces are not included).

This unifies all of our APIs behind `*.api.apollographql.com` and removes references to the obsolete names "engine", "apollodata", and "edge server reporting".

I was originally planning to log a deprecation warning when the `engine` option is passed, but I currently think that's overkill. Migrating isn't particularly hard but it's also not particularly urgent for current users, as long as new users don't have to be exposed to the old API. Instead restore the previous behavior of logging a deprecation warning for `engine.schemaTag` specifically. Opinions welcome.

Primarily docs, comments, and strings changes. Fix an URL added incorrectly in this PR. Remove a comment mentioning GraphQLServerOptions.reporting which was removed in v2.15.0.

Delete maskErrorDetails integration test and replace it with a unit test.

it's helpful to have an actionable message when something goes wrong aside from a simple "contact support" if there is a way to achieve the action that caused the error. in this case, the user is likely trying to set up Apollo Studio with their service, which they can do if they follow along with the documentation. this change adds a link to said documentation to immediately unblock users who run into this error.

(By avery and glasser!)

This doesn't change the run-time mechanism but does remove it from the public ApolloServerPlugin interface while still allowing for type safety within this package.

There are very few uses of `logger.info` in Apollo Server (though custom plugins could use it). This mostly means that startup logs in the plugins will be visible by default. Users can override this by providing their own `logger`.

@sachindshinde

The Apollo API has recently changed such that `reportServerInfo` will now return error for schemas that don't parse or validate. This PR changes `apollo-engine-reporting` to check in the `EngineReportingAgent` constructor whether override schemas successfully parse and validate, so that we may return early before talking to the Apollo API. (Originally #4492 by @sachindshinde, merged directly into release-2.18.0; because the main change being released with v2.18 is #4453 which entirely replaced the file which that PR changed, this is being reconstructed as part of PR #4453.)

glasser requested review from abernix and trevor-scheer August 4, 2020 22:03

glasser force-pushed the glasser/server-will-stop branch 2 times, most recently from 2faa90d to 3d61611 Compare August 5, 2020 06:02

glasser force-pushed the glasser/apollo-config branch 2 times, most recently from 69b390f to 526443a Compare August 5, 2020 06:19

glasser changed the title ~~Move basic configuration from engine to apollo~~ Make communicating with Apollo's commercial products into ordinary plugins Aug 5, 2020

glasser force-pushed the glasser/apollo-config branch from f0b08ea to 0d94ec6 Compare August 5, 2020 23:00

trevor-scheer reviewed Aug 6, 2020

View reviewed changes

glasser mentioned this pull request Aug 19, 2020

Add serverWillStop lifecycle hook; call stop() on signals by default #4450

Closed

glasser force-pushed the glasser/server-will-stop branch from 3d61611 to 67dec32 Compare August 19, 2020 20:10

glasser force-pushed the glasser/apollo-config branch 6 times, most recently from 9692ee5 to 9c352bb Compare August 20, 2020 09:26

abernix reviewed Aug 21, 2020

View reviewed changes

glasser force-pushed the glasser/server-will-stop branch from 67dec32 to dc5b8f5 Compare August 24, 2020 21:22

glasser force-pushed the glasser/apollo-config branch 4 times, most recently from dd2e81f to 73700cb Compare August 25, 2020 23:58

glasser force-pushed the glasser/apollo-config branch from 73700cb to 4953b86 Compare August 26, 2020 00:34

glasser and others added 20 commits September 18, 2020 14:38

docs: migration guide

28af042

docs: update data/errors to put rewriteError in plugins

cc9f243

Update CHANGELOG

726f4da

docs: update federation/metrics

786bb75

docs: update monitoring/metrics

a669591

I dropped the reference to debugPrintReports (which is still in the plugin reference) because it barely works (the actual traces are not included).

Update operation registry plugin README

dea18ce

Update URLs used to communicate with Apollo's servers

f9bad52

This unifies all of our APIs behind `*.api.apollographql.com` and removes references to the obsolete names "engine", "apollodata", and "edge server reporting".

Tentatively commit to this being released in 2.18

3519009

Audit whole repo for 'engine' and 'graph manager'

0679a02

Primarily docs, comments, and strings changes. Fix an URL added incorrectly in this PR. Remove a comment mentioning GraphQLServerOptions.reporting which was removed in v2.15.0.

Update some integration tests to pass new arguments

d852220

Delete maskErrorDetails integration test and replace it with a unit test.

Clarify a behavior change in the default behavior for inline tracing

5fc9fe1

Update two links added to errors on this branch to use shortlinks

850469c

(By avery and glasser!)

CHANGELOG tweaks (break up into more bullets, other changes)

dd5f752

Various feedback from @abernix code review

5bf7d3e

Move __internal_plugin_id__ to an internal type

191ce14

This doesn't change the run-time mechanism but does remove it from the public ApolloServerPlugin interface while still allowing for type safety within this package.

Change the default logger level to INFO

7a7c118

There are very few uses of `logger.info` in Apollo Server (though custom plugins could use it). This mostly means that startup logs in the plugins will be visible by default. Users can override this by providing their own `logger`.

glasser force-pushed the glasser/apollo-config branch from 8a17501 to a897b59 Compare September 18, 2020 21:39

glasser changed the base branch from main to release-2.18.0 September 18, 2020 21:39

glasser merged commit 9501238 into release-2.18.0 Sep 18, 2020

glasser deleted the glasser/apollo-config branch September 18, 2020 21:48

glasser mentioned this pull request Sep 22, 2020

examples/acephei: update for #4561 and #4453 #4583

Merged

devbdavis mentioned this pull request Sep 23, 2020

ApolloServerPlugin extends AnyFunctionMap causing Typescript Compilation Error #4585

Closed

glasser mentioned this pull request Sep 23, 2020

[BE-615] AER: use a static identifier for GraphQL validation failures #3465

Merged

StephenBarlow mentioned this pull request Oct 2, 2020

Replicate changes from glasser apollographql/federation#201

Merged

abernix mentioned this pull request Oct 5, 2020

Add docs "sendReportsImmediately" option #4399

Closed

abernix mentioned this pull request Oct 21, 2020

Including operation name in the query execution apollographql/federation#195

Closed

		process.env.ENGINE_API_KEY = 'just:fake:stuff';
		process.env.APOLLO_KEY = 'also:fake:stuff';

		@@ -0,0 +1,271 @@
		// This class is a helper for ApoloServerPluginUsageReporting and

	// This class is a helper for ApoloServerPluginUsageReporting and
	// This class is a helper for ApolloServerPluginUsageReporting and

		const { cache: apqCache = requestOptions.cache!, ...apqOtherOptions } =
		requestOptions.persistedQueries \|\| Object.create(null);

Conversation

glasser commented Aug 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

glasser commented Aug 4, 2020

Uh oh!

glasser commented Aug 5, 2020

Uh oh!

glasser commented Aug 5, 2020

Uh oh!

glasser commented Aug 5, 2020

Uh oh!

glasser commented Aug 5, 2020

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!

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!

abernix 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

glasser Aug 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

glasser commented Aug 26, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

glasser commented Aug 4, 2020 •

edited

Loading

glasser Aug 25, 2020 •

edited

Loading