Make communicating with Apollo's commercial products into ordinary plugins #4453

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

packages/apollo-engine-reporting/src/agent.ts

@@ -702,7 +629,7 @@ export class EngineReportingAgent<TContext = any> {
          method: 'POST',
          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

packages/apollo-server-core/src/ApolloServer.ts

+      // 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

packages/apollo-server-core/src/__tests__/ApolloServerBase.test.ts

+  // 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

packages/apollo-server-core/src/__tests__/ApolloServerBase.test.ts

@@ -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

packages/apollo-server-core/src/__tests__/ApolloServerBase.test.ts

    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

packages/apollo-server-core/src/determineApolloConfig.ts

+      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

packages/apollo-server-core/src/plugin/schemaReporting/index.ts

+        );
+      }
+
+      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

packages/apollo-server-core/src/plugin/schemaReporting/index.ts

+      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

packages/apollo-server-core/src/plugin/traceTreeBuilder.ts

@@ -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

packages/apollo-server-core/src/__tests__/ApolloServerBase.test.ts

@@ -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

packages/apollo-server-core/src/determineApolloConfig.ts

+      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.

packages/apollo-reporting-protobuf/package.json

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

packages/apollo-server-core/src/ApolloServer.ts

+      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

packages/apollo-server-core/src/ApolloServer.ts

-          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.

packages/apollo-server-core/src/plugin/usageReporting/legacyOptions.ts

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

packages/apollo-server-core/src/plugin/usageReporting/plugin.ts

+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.

packages/apollo-server-core/src/plugin/usageReporting/plugin.ts

packages/apollo-server-core/src/plugin/schemaReporting/schemaReporter.ts

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?

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.)

In #4561 we removed `@apollo/federation` and `@apollo/gateway` from the apollo-server repo. This means that the acephei example doesn't know where to import them from. Adding them to examples/acephei/package.json led to very strange errors so let's just add them as top-level dev dependencies. Additionally, removing `@apollo/federation` seemed to make it so that TypeScript can't find `apollo-env` any more, but it also seems like maybe the references to it in the tsconfig.json lines aren't necessary? Not sure what's going on here but leaving them in led to build errors and removing them didn't. Finally, update the uses of `engine:` to use the new post-#4453 APIs, and spell the version "set up" as two words.

apollographql/apollo-server#4453 removes the old product name "Engine" from the Apollo Server API. Most relevantly for the JS Gateway: - The package `apollo-engine-reporting-protobuf` (used in the gateway to build federated traces) is renamed to `apollo-reporting-protobuf`. - The `GraphQLService` interface implemented by `ApolloGateway` now passes configuration related to Apollo's hosted services in the `apollo` key instead of the `engine` key, and the values in that object change slightly too (`apiKeyHash` changes to `keyHash`, and `graphVariant` is always provided). Apollo Server continues to pass the old `engine` value as well for compatibility purposes. After this change, `ApolloGateway` can read its configuration from either key too. There is no constraint on the order in which you upgrade Apollo Server and Apollo Gateway, though it is still a good idea to upgrade them at the same time (eg, so you only are using one protobuf package). Also update a comment in federation-js, and upgrade the top-level `@types/node-fetch` dependency so that (after running `npm dedup`) there's no need for a `gateway-js/node_modules` directory.

In #4561 we removed `@apollo/federation` and `@apollo/gateway` from the apollo-server repo. This means that the acephei example doesn't know where to import them from. Adding them to examples/acephei/package.json led to very strange errors so let's just add them as top-level dev dependencies. Additionally, removing `@apollo/federation` seemed to make it so that TypeScript can't find `apollo-env` any more, but it also seems like maybe the references to it in the tsconfig.json lines aren't necessary? Not sure what's going on here but leaving them in led to build errors and removing them didn't. Finally, update the uses of `engine:` to use the new post-#4453 APIs, and spell the version "set up" as two words.

apollographql/apollo-server#4453 removes the old product name "Engine" from the Apollo Server API. Most relevantly for the JS Gateway: - The package `apollo-engine-reporting-protobuf` (used in the gateway to build federated traces) is renamed to `apollo-reporting-protobuf`. - The `GraphQLService` interface implemented by `ApolloGateway` now passes configuration related to Apollo's hosted services in the `apollo` key instead of the `engine` key, and the values in that object change slightly too (`apiKeyHash` changes to `keyHash`, and `graphVariant` is always provided). Apollo Server continues to pass the old `engine` value as well for compatibility purposes. After this change, `ApolloGateway` can read its configuration from either key too. There is no constraint on the order in which you upgrade Apollo Server and Apollo Gateway, though it is still a good idea to upgrade them at the same time (eg, so you only are using one protobuf package). Also update a comment in federation-js, and upgrade the top-level `@types/node-fetch` dependency so that (after running `npm dedup`) there's no need for a `gateway-js/node_modules` directory.

Relates to #4453. Co-authored-by: Jesse Rosenberger <git@jro.cc>

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 10 commits September 18, 2020 14:38

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

brgaulin 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 added a commit that referenced this pull request Oct 5, 2020

fix: Missing lru-cache dependency (#4600)

6a51396

Relates to #4453. Co-authored-by: Jesse Rosenberger <git@jro.cc>

glasser pushed a commit that referenced this pull request Oct 5, 2020

fix: Missing lru-cache dependency (#4600)

7b2b115

Relates to #4453. Co-authored-by: Jesse Rosenberger <git@jro.cc>

abernix mentioned this pull request Oct 21, 2020

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

Closed

github-actions bot locked as resolved and limited conversation to collaborators Apr 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make communicating with Apollo's commercial products into ordinary plugins #4453

Make communicating with Apollo's commercial products into ordinary plugins #4453

glasser commented Aug 4, 2020 •

edited

Loading

glasser commented Aug 4, 2020

glasser commented Aug 5, 2020

glasser commented Aug 5, 2020

glasser commented Aug 5, 2020

glasser commented Aug 5, 2020

trevor-scheer Aug 6, 2020

trevor-scheer Aug 6, 2020

glasser Aug 19, 2020

trevor-scheer Aug 6, 2020

trevor-scheer Aug 6, 2020

glasser Aug 19, 2020

abernix Aug 21, 2020

trevor-scheer Aug 6, 2020

trevor-scheer Aug 6, 2020

glasser Aug 19, 2020

abernix Aug 21, 2020

trevor-scheer Aug 6, 2020

glasser Aug 19, 2020

trevor-scheer Aug 6, 2020

glasser Aug 19, 2020

trevor-scheer Aug 6, 2020

glasser Aug 19, 2020

abernix left a comment

abernix Aug 21, 2020

abernix Aug 21, 2020

abernix Aug 21, 2020

glasser Aug 21, 2020

glasser Aug 25, 2020 •

edited

Loading

abernix Aug 21, 2020

abernix Aug 21, 2020

glasser commented Aug 26, 2020

		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);

Make communicating with Apollo's commercial products into ordinary plugins #4453

Make communicating with Apollo's commercial products into ordinary plugins #4453

Conversation

glasser commented Aug 4, 2020 • edited Loading

glasser commented Aug 4, 2020

glasser commented Aug 5, 2020

glasser commented Aug 5, 2020

glasser commented Aug 5, 2020

glasser commented Aug 5, 2020

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

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

abernix 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 Aug 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glasser commented Aug 26, 2020

glasser commented Aug 4, 2020 •

edited

Loading

glasser Aug 25, 2020 •

edited

Loading