From de7ba72b50ff539fc33b59cf88553527b6e99a70 Mon Sep 17 00:00:00 2001
From: Jesse Rosenberger <git@jro.cc>
Date: Mon, 30 Mar 2020 15:02:44 +0300
Subject: [PATCH 01/27] refactor: Graph Manager (Engine) reporting "extensions"
 become "plugins".

Similar to 6009d8a00 (#3991) and 68cbc938 (#3997), which migrated the
tracing and cache-control extensions to the (newer) request pipeline plugin
API, this commit introduces:

 - Internally, a `plugin` named export which is utilized by the `agent`'s
   `newExtension` method to provide a plugin which is instrumented to
   transmit metrics to Apollo Graph Manager.  This plugin is meant to
   replicate the behavior of the `EngineReportingExtension` class which,
   as of this commit, still lives besides it.
 - Externally, a `federatedPlugin` exported on the main module of the
   `apollo-engine-reporting` package.  This plugin is meant to replicate the
   behavior of the `EngineFederatedTracingExtension` class (also exported on
   the main module) which, again as of this commit, still lives besides it!

Again, the delta of the commits seemed more confusing by allowing a natural
`diff` to be made of it, I've left the extensions in place so they can be
compared - presumably side-by-side in an editor - on the same commit.  An
(immediate) subsequent commit will remove the extension.
---
 packages/apollo-engine-reporting/package.json |   1 +
 .../src/__tests__/extension.test.ts           |  56 ++----
 packages/apollo-engine-reporting/src/agent.ts |  11 +-
 .../apollo-engine-reporting/src/extension.ts  | 174 ++++++++++++++++++
 .../src/federatedExtension.ts                 |  62 +++++++
 packages/apollo-engine-reporting/src/index.ts |   2 +-
 .../src/treeBuilder.ts                        |   2 +-
 .../apollo-engine-reporting/tsconfig.json     |   1 +
 .../apollo-server-core/src/ApolloServer.ts    |  70 ++++---
 9 files changed, 297 insertions(+), 82 deletions(-)

diff --git a/packages/apollo-engine-reporting/package.json b/packages/apollo-engine-reporting/package.json
index df8036f35cb..fa666882864 100644
--- a/packages/apollo-engine-reporting/package.json
+++ b/packages/apollo-engine-reporting/package.json
@@ -18,6 +18,7 @@
     "apollo-server-errors": "file:../apollo-server-errors",
     "apollo-server-types": "file:../apollo-server-types",
     "async-retry": "^1.2.1",
+    "apollo-server-plugin-base": "file:../apollo-server-plugin-base",
     "graphql-extensions": "file:../graphql-extensions"
   },
   "peerDependencies": {
diff --git a/packages/apollo-engine-reporting/src/__tests__/extension.test.ts b/packages/apollo-engine-reporting/src/__tests__/extension.test.ts
index 14b50c73cfd..8520f902f36 100644
--- a/packages/apollo-engine-reporting/src/__tests__/extension.test.ts
+++ b/packages/apollo-engine-reporting/src/__tests__/extension.test.ts
@@ -1,19 +1,11 @@
 import { makeExecutableSchema, addMockFunctionsToSchema } from 'graphql-tools';
-import {
-  GraphQLExtensionStack,
-  enableGraphQLExtensions,
-} from 'graphql-extensions';
 import { graphql, GraphQLError } from 'graphql';
 import { Request } from 'node-fetch';
-import {
-  EngineReportingExtension,
-  makeTraceDetails,
-  makeHTTPRequestHeaders,
-} from '../extension';
+import { makeTraceDetails, makeHTTPRequestHeaders, plugin } from '../extension';
 import { Headers } from 'apollo-server-env';
-import { InMemoryLRUCache } from 'apollo-server-caching';
 import { AddTraceArgs } from '../agent';
 import { Trace } from 'apollo-engine-reporting-protobuf';
+import pluginTestHarness from 'apollo-server-core/dist/utils/pluginTestHarness';
 
 it('trace construction', async () => {
   const typeDefs = `
@@ -53,41 +45,33 @@ it('trace construction', async () => {
 
   const schema = makeExecutableSchema({ typeDefs });
   addMockFunctionsToSchema({ schema });
-  enableGraphQLExtensions(schema);
 
   const traces: Array<AddTraceArgs> = [];
   async function addTrace(args: AddTraceArgs) {
     traces.push(args);
   }
 
-  const reportingExtension = new EngineReportingExtension(
-    {},
-    addTrace,
-    'schema-hash',
-  );
-  const stack = new GraphQLExtensionStack([reportingExtension]);
-  const requestDidEnd = stack.requestDidStart({
-    request: new Request('http://localhost:123/foo'),
-    queryString: query,
-    requestContext: {
-      request: {
-        query,
-        operationName: 'q',
-        extensions: {
-          clientName: 'testing suite',
-        },
+  const pluginInstance = plugin({ /* no options!*/ }, addTrace);
+
+  pluginTestHarness({
+    pluginInstance,
+    schema,
+    graphqlRequest: {
+      query,
+      operationName: 'q',
+      extensions: {
+        clientName: 'testing suite',
       },
-      context: {},
-      cache: new InMemoryLRUCache(),
+      http: new Request('http://localhost:123/foo'),
+    },
+    executor: async ({ request: { query: source }}) => {
+      return await graphql({
+        schema,
+        source,
+      });
     },
-    context: {},
-  });
-  await graphql({
-    schema,
-    source: query,
-    contextValue: { _extensionStack: stack },
   });
-  requestDidEnd();
+
   // XXX actually write some tests
 });
 
diff --git a/packages/apollo-engine-reporting/src/agent.ts b/packages/apollo-engine-reporting/src/agent.ts
index dd522203b71..d98861bea3b 100644
--- a/packages/apollo-engine-reporting/src/agent.ts
+++ b/packages/apollo-engine-reporting/src/agent.ts
@@ -11,10 +11,11 @@ import {
 import { fetch, RequestAgent, Response } from 'apollo-server-env';
 import retry from 'async-retry';
 
-import { EngineReportingExtension } from './extension';
+import { plugin } from './extension';
 import { GraphQLRequestContext, Logger, SchemaHash } from 'apollo-server-types';
 import { InMemoryLRUCache } from 'apollo-server-caching';
 import { defaultEngineReportingSignature } from 'apollo-graphql';
+import { ApolloServerPlugin } from "apollo-server-plugin-base";
 
 export interface ClientInfo {
   clientName?: string;
@@ -278,12 +279,8 @@ export class EngineReportingAgent<TContext = any> {
     handleLegacyOptions(this.options);
   }
 
-  public newExtension(schemaHash: SchemaHash): EngineReportingExtension<TContext> {
-    return new EngineReportingExtension<TContext>(
-      this.options,
-      this.addTrace.bind(this),
-      schemaHash,
-    );
+  public newExtension(): ApolloServerPlugin<TContext> {
+    return plugin(this.options, this.addTrace.bind(this));
   }
 
   public async addTrace({
diff --git a/packages/apollo-engine-reporting/src/extension.ts b/packages/apollo-engine-reporting/src/extension.ts
index aacc702dcb1..00e0a18da6c 100644
--- a/packages/apollo-engine-reporting/src/extension.ts
+++ b/packages/apollo-engine-reporting/src/extension.ts
@@ -22,6 +22,9 @@ import {
   SendValuesBaseOptions,
 } from './agent';
 import { EngineReportingTreeBuilder } from './treeBuilder';
+import { ApolloServerPlugin } from "apollo-server-plugin-base";
+
+type Mutable<T> = { -readonly [P in keyof T]: T[P] };
 
 const clientNameHeaderKey = 'apollographql-client-name';
 const clientReferenceIdHeaderKey = 'apollographql-client-reference-id';
@@ -208,6 +211,177 @@ export class EngineReportingExtension<TContext = any>
   }
 }
 
+// This plugin is instantiated once at server start-up. Each request that the
+// server processes will invoke the `requestDidStart` method which will produce
+// a trace (in protobuf Trace format) for that single request. When the request
+// is done, it passes the Trace back to its associated EngineReportingAgent via
+// the addTrace callback. This class isn't for direct use; its constructor is a
+// private API for communicating with EngineReportingAgent.
+export const plugin = <TContext>(
+  options: EngineReportingOptions<TContext> = Object.create(null),
+  addTrace: (args: AddTraceArgs) => Promise<void>,
+  // schemaHash: string,
+): ApolloServerPlugin<TContext> => {
+  const logger: Logger = options.logger || console;
+  const generateClientInfo: GenerateClientInfo<TContext> =
+    options.generateClientInfo || defaultGenerateClientInfo;
+
+
+  return {
+    requestDidStart(requestContext) {
+      let queryString: string | undefined;
+      const treeBuilder: EngineReportingTreeBuilder =
+        new EngineReportingTreeBuilder({
+          rewriteError: options.rewriteError,
+          logger: requestContext.logger || logger,
+        });
+
+      const metrics: NonNullable<typeof requestContext['metrics']> =
+        ((requestContext as Mutable<typeof requestContext>)
+          .metrics = requestContext.metrics || Object.create(null));
+
+      treeBuilder.startTiming();
+      metrics.startHrTime = treeBuilder.startHrTime;
+
+      if (requestContext.request.http) {
+        treeBuilder.trace.http = new Trace.HTTP({
+          method:
+            Trace.HTTP.Method[
+              requestContext.request.http
+                .method as keyof typeof Trace.HTTP.Method
+            ] || Trace.HTTP.Method.UNKNOWN,
+          // Host and path are not used anywhere on the backend, so let's not bother
+          // trying to parse request.url to get them, which is a potential
+          // source of bugs because integrations have different behavior here.
+          // On Node's HTTP module, request.url only includes the path
+          // (see https://nodejs.org/api/http.html#http_message_url)
+          // The same is true on Lambda (where we pass event.path)
+          // But on environments like Cloudflare we do get a complete URL.
+          host: null,
+          path: null,
+        });
+      }
+
+      let preflightDone: boolean = false;
+      function ensurePreflight() {
+        if (preflightDone) return;
+        preflightDone = true;
+
+        if (options.sendHeaders) {
+          if (requestContext.request.http && treeBuilder.trace.http) {
+            makeHTTPRequestHeaders(
+              treeBuilder.trace.http,
+              requestContext.request.http.headers,
+              options.sendHeaders,
+            );
+          }
+        }
+
+        if (metrics.persistedQueryHit) {
+          treeBuilder.trace.persistedQueryHit = true;
+        }
+        if (metrics.persistedQueryRegister) {
+          treeBuilder.trace.persistedQueryRegister = true;
+        }
+
+        // Generally, we'll get queryString here and not parsedQuery; we only get
+        // parsedQuery if you're using an OperationStore. In normal cases we'll
+        // get our documentAST in the execution callback after it is parsed.
+        queryString = requestContext.source;
+
+        if (requestContext.request.variables) {
+          treeBuilder.trace.details = makeTraceDetails(
+            requestContext.request.variables,
+            options.sendVariableValues,
+            queryString,
+          );
+        }
+
+        const clientInfo = generateClientInfo(requestContext);
+        if (clientInfo) {
+          // While clientAddress could be a part of the protobuf, we'll ignore it for
+          // now, since the backend does not group by it and Engine frontend will not
+          // support it in the short term
+          const { clientName, clientVersion, clientReferenceId } = clientInfo;
+          // the backend makes the choice of mapping clientName => clientReferenceId if
+          // no custom reference id is provided
+          treeBuilder.trace.clientVersion = clientVersion || '';
+          treeBuilder.trace.clientReferenceId = clientReferenceId || '';
+          treeBuilder.trace.clientName = clientName || '';
+        }
+      }
+
+      let endDone: boolean = false;
+      function didEnd() {
+        if (endDone) return;
+        endDone = true;
+        treeBuilder.stopTiming();
+
+        treeBuilder.trace.fullQueryCacheHit = !!metrics.responseCacheHit;
+        treeBuilder.trace.forbiddenOperation = !!metrics.forbiddenOperation;
+        treeBuilder.trace.registeredOperation = !!metrics.registeredOperation;
+
+        // If the user did not explicitly specify an operation name (which we
+        // would have saved in `executionDidStart`), but the request pipeline made
+        // it far enough to figure out what the operation name must be and store
+        // it on requestContext.operationName, use that name.  (Note that this
+        // depends on the assumption that the RequestContext passed to
+        // requestDidStart, which does not yet have operationName, will be mutated
+        // to add operationName later.)
+        const operationName = requestContext.operationName || '';
+
+        // If this was a federated operation and we're the gateway, add the query plan
+        // to the trace.
+        if (metrics.queryPlanTrace) {
+          treeBuilder.trace.queryPlan = metrics.queryPlanTrace;
+        }
+
+        addTrace({
+          operationName,
+          queryHash: requestContext.queryHash!,
+          documentAST: requestContext.document,
+          queryString,
+          trace: treeBuilder.trace,
+          schemaHash: requestContext.schemaHash,
+        });
+      }
+
+      return {
+        parsingDidStart() {
+          ensurePreflight();
+        },
+
+        validationDidStart() {
+          ensurePreflight();
+        },
+
+        didResolveOperation() {
+          ensurePreflight();
+        },
+
+        executionDidStart() {
+          ensurePreflight();
+          return didEnd;
+        },
+
+        willResolveField(...args) {
+          const [, , , info] = args;
+          return treeBuilder.willResolveField(info);
+          // We could save the error into the trace during the end handler, but
+          // it won't have all the information that graphql-js adds to it later,
+          // like 'locations'.
+        },
+
+        didEncounterErrors({ errors }) {
+          ensurePreflight();
+          treeBuilder.didEncounterErrors(errors);
+          didEnd();
+        },
+      };
+    }
+  };
+};
+
 // Helpers for producing traces.
 
 function defaultGenerateClientInfo({ request }: GraphQLRequestContext) {
diff --git a/packages/apollo-engine-reporting/src/federatedExtension.ts b/packages/apollo-engine-reporting/src/federatedExtension.ts
index 711c95fbf6e..a811cd04149 100644
--- a/packages/apollo-engine-reporting/src/federatedExtension.ts
+++ b/packages/apollo-engine-reporting/src/federatedExtension.ts
@@ -4,6 +4,68 @@ import { Trace } from 'apollo-engine-reporting-protobuf';
 import { GraphQLRequestContext } from 'apollo-server-types';
 
 import { EngineReportingTreeBuilder } from './treeBuilder';
+import { ApolloServerPlugin } from "apollo-server-plugin-base";
+import { EngineReportingOptions } from "./agent";
+
+type FederatedReportingOptions<TContext> = Pick<EngineReportingOptions<TContext>, 'rewriteError'>
+
+export const plugin = <TContext>(
+  options: FederatedReportingOptions<TContext> = Object.create(null),
+): ApolloServerPlugin<TContext> => {
+  return {
+    requestDidStart(requestContext) {
+      const treeBuilder: EngineReportingTreeBuilder =
+        new EngineReportingTreeBuilder({
+          rewriteError: options.rewriteError,
+        });
+
+      // XXX Provide a mechanism to customize this logic.
+      const http = requestContext.request.http;
+      if (
+        !http ||
+        !http.headers ||
+        http.headers.get('apollo-federation-include-trace') !== 'ftv1'
+      ) {
+        return;
+      }
+
+      treeBuilder.startTiming();
+
+      return {
+        willResolveField(...args) {
+          const [ , , , info] = args;
+          return treeBuilder.willResolveField(info);
+        },
+
+        didEncounterErrors({ errors }) {
+          treeBuilder.didEncounterErrors(errors);
+        },
+
+        willSendResponse({ response }) {
+          // We record the end time at the latest possible time: right before serializing the trace.
+          // If we wait any longer, the time we record won't actually be sent anywhere!
+          treeBuilder.stopTiming();
+
+          const encodedUint8Array = Trace.encode(treeBuilder.trace).finish();
+          const encodedBuffer = Buffer.from(
+            encodedUint8Array,
+            encodedUint8Array.byteOffset,
+            encodedUint8Array.byteLength,
+          );
+
+          const extensions =
+            response.extensions || (response.extensions = Object.create(null));
+
+          if (typeof extensions.ftv1 !== "undefined") {
+            throw new Error("The `ftv1` `extensions` were already present.");
+          }
+
+          extensions.ftv1 = encodedBuffer.toString('base64');
+        }
+      }
+    },
+  }
+};
 
 export class EngineFederatedTracingExtension<TContext = any>
   implements GraphQLExtension<TContext> {
diff --git a/packages/apollo-engine-reporting/src/index.ts b/packages/apollo-engine-reporting/src/index.ts
index 5770edf02c3..36c7e53a85f 100644
--- a/packages/apollo-engine-reporting/src/index.ts
+++ b/packages/apollo-engine-reporting/src/index.ts
@@ -1,2 +1,2 @@
 export { EngineReportingOptions, EngineReportingAgent } from './agent';
-export { EngineFederatedTracingExtension } from './federatedExtension';
+export { plugin as federatedPlugin } from './federatedExtension';
diff --git a/packages/apollo-engine-reporting/src/treeBuilder.ts b/packages/apollo-engine-reporting/src/treeBuilder.ts
index a10535bbe23..f38283d572f 100644
--- a/packages/apollo-engine-reporting/src/treeBuilder.ts
+++ b/packages/apollo-engine-reporting/src/treeBuilder.ts
@@ -78,7 +78,7 @@ export class EngineReportingTreeBuilder {
     };
   }
 
-  public didEncounterErrors(errors: GraphQLError[]) {
+  public didEncounterErrors(errors: readonly GraphQLError[]) {
     errors.forEach(err => {
       if (
         err instanceof PersistedQueryNotFoundError ||
diff --git a/packages/apollo-engine-reporting/tsconfig.json b/packages/apollo-engine-reporting/tsconfig.json
index ca382f1094c..1e8b57f68f2 100644
--- a/packages/apollo-engine-reporting/tsconfig.json
+++ b/packages/apollo-engine-reporting/tsconfig.json
@@ -10,5 +10,6 @@
     { "path": "../graphql-extensions" },
     { "path": "../apollo-server-errors" },
     { "path": "../apollo-server-types" },
+    { "path": "../apollo-server-plugin-base" },
   ]
 }
diff --git a/packages/apollo-server-core/src/ApolloServer.ts b/packages/apollo-server-core/src/ApolloServer.ts
index 4ae62ad9889..56afdc34a1c 100644
--- a/packages/apollo-server-core/src/ApolloServer.ts
+++ b/packages/apollo-server-core/src/ApolloServer.ts
@@ -225,10 +225,6 @@ export class ApolloServerBase {
     this.parseOptions = parseOptions;
     this.context = context;
 
-    // Plugins will be instantiated if they aren't already, and this.plugins
-    // is populated accordingly.
-    this.ensurePluginInstantiation(plugins);
-
     // While reading process.env is slow, a server should only be constructed
     // once per run, so we place the env check inside the constructor. If env
     // should be used outside of the constructor context, place it as a private
@@ -413,6 +409,11 @@ export class ApolloServerBase {
     } else {
       throw new Error("Unexpected error: Unable to resolve a valid GraphQLSchema.  Please file an issue with a reproduction of this error, if possible.");
     }
+
+    // Plugins will be instantiated if they aren't already, and this.plugins
+    // is populated accordingly.
+    this.ensurePluginInstantiation(plugins);
+
   }
 
   // used by integrations to synchronize the path with subscriptions, some
@@ -562,39 +563,6 @@ export class ApolloServerBase {
 
     const extensions = [];
 
-    const schemaIsFederated = this.schemaIsFederated(schema);
-    const { engine } = this.config;
-    // Keep this extension second so it wraps everything, except error formatting
-    if (this.engineReportingAgent) {
-      if (schemaIsFederated) {
-        // XXX users can configure a federated Apollo Server to send metrics, but the
-        // Gateway should be responsible for that. It's possible that users are running
-        // their own gateway or running a federated service on its own. Nonetheless, in
-        // the likely case it was accidental, we warn users that they should only report
-        // metrics from the Gateway.
-        this.logger.warn(
-          "It looks like you're running a federated schema and you've configured your service " +
-            'to report metrics to Apollo Graph Manager. You should only configure your Apollo gateway ' +
-            'to report metrics to Apollo Graph Manager.',
-        );
-      }
-      extensions.push(() =>
-        this.engineReportingAgent!.newExtension(schemaHash),
-      );
-    } else if (engine !== false && schemaIsFederated) {
-      // We haven't configured this app to use Engine directly. But it looks like
-      // we are a federated service backend, so we should be capable of including
-      // our trace in a response extension if we are asked to by the gateway.
-      const {
-        EngineFederatedTracingExtension,
-      } = require('apollo-engine-reporting');
-      const rewriteError =
-        engine && typeof engine === 'object' ? engine.rewriteError : undefined;
-      extensions.push(
-        () => new EngineFederatedTracingExtension({ rewriteError }),
-      );
-    }
-
     // Note: doRunQuery will add its own extensions if you set tracing,
     // or cacheControl.
     extensions.push(...(_extensions || []));
@@ -789,6 +757,34 @@ export class ApolloServerBase {
     // User's plugins, provided as an argument to this method, will be added
     // at the end of that list so they take precidence.
     // A follow-up commit will actually introduce this.
+    // Also, TODO, remove this comment.
+
+    const federatedSchema = this.schema && this.schemaIsFederated(this.schema);
+    const { engine } = this.config;
+    // Keep this extension second so it wraps everything, except error formatting
+    if (this.engineReportingAgent) {
+      if (federatedSchema) {
+        // XXX users can configure a federated Apollo Server to send metrics, but the
+        // Gateway should be responsible for that. It's possible that users are running
+        // their own gateway or running a federated service on its own. Nonetheless, in
+        // the likely case it was accidental, we warn users that they should only report
+        // metrics from the Gateway.
+        this.logger.warn(
+          "It looks like you're running a federated schema and you've configured your service " +
+            'to report metrics to Apollo Graph Manager. You should only configure your Apollo gateway ' +
+            'to report metrics to Apollo Graph Manager.',
+        );
+      }
+      pluginsToInit.push(this.engineReportingAgent!.newExtension());
+    } else if (engine !== false && federatedSchema) {
+      // We haven't configured this app to use Engine directly. But it looks like
+      // we are a federated service backend, so we should be capable of including
+      // our trace in a response extension if we are asked to by the gateway.
+      const { federatedPlugin } = require('apollo-engine-reporting');
+      const rewriteError =
+        engine && typeof engine === 'object' ? engine.rewriteError : undefined;
+      pluginsToInit.push(federatedPlugin({ rewriteError }));
+    }
 
     pluginsToInit.push(...plugins);
     this.plugins = pluginsToInit.map(plugin => {

From 78a4cb77edc7c7f11b68f5969a6e85b75e75b6e7 Mon Sep 17 00:00:00 2001
From: Jesse Rosenberger <git@jro.cc>
Date: Tue, 14 Apr 2020 17:24:42 +0300
Subject: [PATCH 02/27] fix: Keep special-cased errors (e.g. APQ not found) as
 unreported.

This fixes the failing tests which correctly surfaced on the last commit.

Previously, prior to the new plugin API, the Apollo Engine Reporting
mechanism was implemented using `graphql-extensions`, the API for which
didn't invoke `requestDidStart` until _after_ APQ had been negotiated.

The new plugin API starts its `requestDidStart` _before_ APQ validation and
various other assertions which weren't included in the `requestDidStart`
life-cycle, even if they perhaps should be in terms of error reporting.

The new plugin API is able to properly capture such errors within its
`didEncounterErrors` lifecycle hook (thanks to
https://github.com/apollographql/apollo-server/pull/3614, which
intentionally captures these failures so plugin authors can accurately react
to them), however, for behavioral consistency reasons, we will still
special-case those errors and maintain the legacy behavior to avoid a
breaking change.  We can reconsider this in a future version of Apollo
Engine Reporting (AS3, perhaps!).

Ref: https://github.com/apollographql/apollo-server/pull/3614
Ref: https://github.com/apollographql/apollo-server/issues/3627
Ref: https://github.com/apollographql/apollo-server/issues/3638
---
 .../apollo-engine-reporting/src/extension.ts  | 44 +++++++++++++++++++
 .../src/treeBuilder.ts                        | 14 +-----
 2 files changed, 45 insertions(+), 13 deletions(-)

diff --git a/packages/apollo-engine-reporting/src/extension.ts b/packages/apollo-engine-reporting/src/extension.ts
index 00e0a18da6c..d3964fd0914 100644
--- a/packages/apollo-engine-reporting/src/extension.ts
+++ b/packages/apollo-engine-reporting/src/extension.ts
@@ -3,6 +3,7 @@ import {
   WithRequired,
   Logger,
   SchemaHash,
+  InvalidGraphQLRequestError,
 } from 'apollo-server-types';
 import { Request, Headers } from 'apollo-server-env';
 import {
@@ -23,6 +24,10 @@ import {
 } from './agent';
 import { EngineReportingTreeBuilder } from './treeBuilder';
 import { ApolloServerPlugin } from "apollo-server-plugin-base";
+import {
+  PersistedQueryNotFoundError,
+  PersistedQueryNotSupportedError,
+} from 'apollo-server-errors';
 
 type Mutable<T> = { -readonly [P in keyof T]: T[P] };
 
@@ -373,6 +378,12 @@ export const plugin = <TContext>(
         },
 
         didEncounterErrors({ errors }) {
+          // We don't report some special-cased errors to Graph Manager.
+          // See the definition of this function for the reasons.
+          if (allUnreportableSpecialCasedErrors(errors)) {
+            return;
+          }
+
           ensurePreflight();
           treeBuilder.didEncounterErrors(errors);
           didEnd();
@@ -382,6 +393,39 @@ export const plugin = <TContext>(
   };
 };
 
+/**
+ * Previously, prior to the new plugin API, the Apollo Engine Reporting
+ * mechanism was implemented using `graphql-extensions`, the API for which
+ * didn't invoke `requestDidStart` until _after_ APQ had been negotiated.
+ *
+ * The new plugin API starts its `requestDidStart` _before_ APQ validation and
+ * various other assertions which weren't included in the `requestDidStart`
+ * life-cycle, even if they perhaps should be in terms of error reporting.
+ *
+ * The new plugin API is able to properly capture such errors within its
+ * `didEncounterErrors` lifecycle hook, however, for behavioral consistency
+ * reasons, we will still special-case those errors and maintain the legacy
+ * behavior to avoid a breaking change.  We can reconsider this in a future
+ * version of Apollo Engine Reporting (AS3, perhaps!).
+ *
+ * @param errors A list of errors to scan for special-cased instances.
+ */
+function allUnreportableSpecialCasedErrors(
+  errors: readonly GraphQLError[],
+): boolean {
+  return errors.every(err => {
+    if (
+      err instanceof PersistedQueryNotFoundError ||
+      err instanceof PersistedQueryNotSupportedError ||
+      err instanceof InvalidGraphQLRequestError
+    ) {
+      return true;
+    }
+
+    return false;
+  });
+}
+
 // Helpers for producing traces.
 
 function defaultGenerateClientInfo({ request }: GraphQLRequestContext) {
diff --git a/packages/apollo-engine-reporting/src/treeBuilder.ts b/packages/apollo-engine-reporting/src/treeBuilder.ts
index f38283d572f..27bfe9376ba 100644
--- a/packages/apollo-engine-reporting/src/treeBuilder.ts
+++ b/packages/apollo-engine-reporting/src/treeBuilder.ts
@@ -1,10 +1,6 @@
 import { GraphQLError, GraphQLResolveInfo, ResponsePath } from 'graphql';
 import { Trace, google } from 'apollo-engine-reporting-protobuf';
-import {
-  PersistedQueryNotFoundError,
-  PersistedQueryNotSupportedError,
-} from 'apollo-server-errors';
-import { InvalidGraphQLRequestError, Logger } from 'apollo-server-types';
+import { Logger } from 'apollo-server-types';
 
 function internalError(message: string) {
   return new Error(`[internal apollo-server error] ${message}`);
@@ -80,14 +76,6 @@ export class EngineReportingTreeBuilder {
 
   public didEncounterErrors(errors: readonly GraphQLError[]) {
     errors.forEach(err => {
-      if (
-        err instanceof PersistedQueryNotFoundError ||
-        err instanceof PersistedQueryNotSupportedError ||
-        err instanceof InvalidGraphQLRequestError
-      ) {
-        return;
-      }
-
       // This is an error from a federated service. We will already be reporting
       // it in the nested Trace in the query plan.
       //

From c67a6dfe0c5d352ac92a44a80711b2c89bbc820b Mon Sep 17 00:00:00 2001
From: Jesse Rosenberger <git@jro.cc>
Date: Wed, 15 Apr 2020 13:35:34 +0300
Subject: [PATCH 03/27] eliminate!: Remove now deprecated
 `EngineReportingExtension`.

The plugin implementation brought in de7ba72b50ff5 supersedes the need for
this implementation!
---
 package-lock.json                             |   4 +-
 packages/apollo-engine-reporting/package.json |   3 +-
 .../{extension.test.ts => plugin.test.ts}     |   2 +-
 packages/apollo-engine-reporting/src/agent.ts |   2 +-
 .../src/federatedExtension.ts                 | 147 -------------
 .../src/federatedPlugin.ts                    |  66 ++++++
 packages/apollo-engine-reporting/src/index.ts |   2 +-
 .../src/{extension.ts => plugin.ts}           | 193 +-----------------
 .../apollo-engine-reporting/tsconfig.json     |   1 -
 9 files changed, 74 insertions(+), 346 deletions(-)
 rename packages/apollo-engine-reporting/src/__tests__/{extension.test.ts => plugin.test.ts} (99%)
 delete mode 100644 packages/apollo-engine-reporting/src/federatedExtension.ts
 create mode 100644 packages/apollo-engine-reporting/src/federatedPlugin.ts
 rename packages/apollo-engine-reporting/src/{extension.ts => plugin.ts} (66%)

diff --git a/package-lock.json b/package-lock.json
index 20a701000e3..1b638d12e8d 100644
--- a/package-lock.json
+++ b/package-lock.json
@@ -4305,9 +4305,9 @@
         "apollo-server-caching": "file:packages/apollo-server-caching",
         "apollo-server-env": "file:packages/apollo-server-env",
         "apollo-server-errors": "file:packages/apollo-server-errors",
+        "apollo-server-plugin-base": "file:packages/apollo-server-plugin-base",
         "apollo-server-types": "file:packages/apollo-server-types",
-        "async-retry": "^1.2.1",
-        "graphql-extensions": "file:packages/graphql-extensions"
+        "async-retry": "^1.2.1"
       }
     },
     "apollo-engine-reporting-protobuf": {
diff --git a/packages/apollo-engine-reporting/package.json b/packages/apollo-engine-reporting/package.json
index fa666882864..05f26ccdd9a 100644
--- a/packages/apollo-engine-reporting/package.json
+++ b/packages/apollo-engine-reporting/package.json
@@ -18,8 +18,7 @@
     "apollo-server-errors": "file:../apollo-server-errors",
     "apollo-server-types": "file:../apollo-server-types",
     "async-retry": "^1.2.1",
-    "apollo-server-plugin-base": "file:../apollo-server-plugin-base",
-    "graphql-extensions": "file:../graphql-extensions"
+    "apollo-server-plugin-base": "file:../apollo-server-plugin-base"
   },
   "peerDependencies": {
     "graphql": "^0.12.0 || ^0.13.0 || ^14.0.0 || ^15.0.0"
diff --git a/packages/apollo-engine-reporting/src/__tests__/extension.test.ts b/packages/apollo-engine-reporting/src/__tests__/plugin.test.ts
similarity index 99%
rename from packages/apollo-engine-reporting/src/__tests__/extension.test.ts
rename to packages/apollo-engine-reporting/src/__tests__/plugin.test.ts
index 8520f902f36..2a8d3313b12 100644
--- a/packages/apollo-engine-reporting/src/__tests__/extension.test.ts
+++ b/packages/apollo-engine-reporting/src/__tests__/plugin.test.ts
@@ -1,7 +1,7 @@
 import { makeExecutableSchema, addMockFunctionsToSchema } from 'graphql-tools';
 import { graphql, GraphQLError } from 'graphql';
 import { Request } from 'node-fetch';
-import { makeTraceDetails, makeHTTPRequestHeaders, plugin } from '../extension';
+import { makeTraceDetails, makeHTTPRequestHeaders, plugin } from '../plugin';
 import { Headers } from 'apollo-server-env';
 import { AddTraceArgs } from '../agent';
 import { Trace } from 'apollo-engine-reporting-protobuf';
diff --git a/packages/apollo-engine-reporting/src/agent.ts b/packages/apollo-engine-reporting/src/agent.ts
index d98861bea3b..71de2c2f08c 100644
--- a/packages/apollo-engine-reporting/src/agent.ts
+++ b/packages/apollo-engine-reporting/src/agent.ts
@@ -11,7 +11,7 @@ import {
 import { fetch, RequestAgent, Response } from 'apollo-server-env';
 import retry from 'async-retry';
 
-import { plugin } from './extension';
+import { plugin } from './plugin';
 import { GraphQLRequestContext, Logger, SchemaHash } from 'apollo-server-types';
 import { InMemoryLRUCache } from 'apollo-server-caching';
 import { defaultEngineReportingSignature } from 'apollo-graphql';
diff --git a/packages/apollo-engine-reporting/src/federatedExtension.ts b/packages/apollo-engine-reporting/src/federatedExtension.ts
deleted file mode 100644
index a811cd04149..00000000000
--- a/packages/apollo-engine-reporting/src/federatedExtension.ts
+++ /dev/null
@@ -1,147 +0,0 @@
-import { GraphQLResolveInfo, GraphQLError } from 'graphql';
-import { GraphQLExtension } from 'graphql-extensions';
-import { Trace } from 'apollo-engine-reporting-protobuf';
-import { GraphQLRequestContext } from 'apollo-server-types';
-
-import { EngineReportingTreeBuilder } from './treeBuilder';
-import { ApolloServerPlugin } from "apollo-server-plugin-base";
-import { EngineReportingOptions } from "./agent";
-
-type FederatedReportingOptions<TContext> = Pick<EngineReportingOptions<TContext>, 'rewriteError'>
-
-export const plugin = <TContext>(
-  options: FederatedReportingOptions<TContext> = Object.create(null),
-): ApolloServerPlugin<TContext> => {
-  return {
-    requestDidStart(requestContext) {
-      const treeBuilder: EngineReportingTreeBuilder =
-        new EngineReportingTreeBuilder({
-          rewriteError: options.rewriteError,
-        });
-
-      // XXX Provide a mechanism to customize this logic.
-      const http = requestContext.request.http;
-      if (
-        !http ||
-        !http.headers ||
-        http.headers.get('apollo-federation-include-trace') !== 'ftv1'
-      ) {
-        return;
-      }
-
-      treeBuilder.startTiming();
-
-      return {
-        willResolveField(...args) {
-          const [ , , , info] = args;
-          return treeBuilder.willResolveField(info);
-        },
-
-        didEncounterErrors({ errors }) {
-          treeBuilder.didEncounterErrors(errors);
-        },
-
-        willSendResponse({ response }) {
-          // We record the end time at the latest possible time: right before serializing the trace.
-          // If we wait any longer, the time we record won't actually be sent anywhere!
-          treeBuilder.stopTiming();
-
-          const encodedUint8Array = Trace.encode(treeBuilder.trace).finish();
-          const encodedBuffer = Buffer.from(
-            encodedUint8Array,
-            encodedUint8Array.byteOffset,
-            encodedUint8Array.byteLength,
-          );
-
-          const extensions =
-            response.extensions || (response.extensions = Object.create(null));
-
-          if (typeof extensions.ftv1 !== "undefined") {
-            throw new Error("The `ftv1` `extensions` were already present.");
-          }
-
-          extensions.ftv1 = encodedBuffer.toString('base64');
-        }
-      }
-    },
-  }
-};
-
-export class EngineFederatedTracingExtension<TContext = any>
-  implements GraphQLExtension<TContext> {
-  private enabled = false;
-  private done = false;
-  private treeBuilder: EngineReportingTreeBuilder;
-
-  public constructor(options: {
-    rewriteError?: (err: GraphQLError) => GraphQLError | null;
-  }) {
-    this.treeBuilder = new EngineReportingTreeBuilder({
-      rewriteError: options.rewriteError,
-    });
-  }
-
-  public requestDidStart(o: {
-    requestContext: GraphQLRequestContext<TContext>;
-  }) {
-    // XXX Provide a mechanism to customize this logic.
-    const http = o.requestContext.request.http;
-    if (
-      http &&
-      http.headers.get('apollo-federation-include-trace') === 'ftv1'
-    ) {
-      this.enabled = true;
-    }
-
-    if (this.enabled) {
-      this.treeBuilder.startTiming();
-    }
-  }
-
-  public willResolveField(
-    _source: any,
-    _args: { [argName: string]: any },
-    _context: TContext,
-    info: GraphQLResolveInfo,
-  ): ((error: Error | null, result: any) => void) | void {
-    if (this.enabled) {
-      return this.treeBuilder.willResolveField(info);
-    }
-  }
-
-  public didEncounterErrors(errors: GraphQLError[]) {
-    if (this.enabled) {
-      this.treeBuilder.didEncounterErrors(errors);
-    }
-  }
-
-  // The ftv1 extension is a base64'd Trace protobuf containing only the
-  // durationNs, startTime, endTime, and root fields.
-  //
-  // Note: format() is only called after executing an operation, and
-  // specifically isn't called for parse or validation errors. Parse and validation
-  // errors in a federated backend will get reported to the end user as a downstream
-  // error but will not get reported to Engine (because Engine filters out downstream
-  // errors)! See #3091.
-  public format(): [string, string] | undefined {
-    if (!this.enabled) {
-      return;
-    }
-    if (this.done) {
-      throw Error('format called twice?');
-    }
-
-    // We record the end time at the latest possible time: right before serializing the trace.
-    // If we wait any longer, the time we record won't actually be sent anywhere!
-    this.treeBuilder.stopTiming();
-    this.done = true;
-
-    const encodedUint8Array = Trace.encode(this.treeBuilder.trace).finish();
-    const encodedBuffer = Buffer.from(
-      encodedUint8Array,
-      encodedUint8Array.byteOffset,
-      encodedUint8Array.byteLength,
-    );
-    return ['ftv1', encodedBuffer.toString('base64')];
-  }
-}
diff --git a/packages/apollo-engine-reporting/src/federatedPlugin.ts b/packages/apollo-engine-reporting/src/federatedPlugin.ts
new file mode 100644
index 00000000000..5bad85c252b
--- /dev/null
+++ b/packages/apollo-engine-reporting/src/federatedPlugin.ts
@@ -0,0 +1,66 @@
+import { Trace } from 'apollo-engine-reporting-protobuf';
+import { EngineReportingTreeBuilder } from './treeBuilder';
+import { ApolloServerPlugin } from "apollo-server-plugin-base";
+import { EngineReportingOptions } from "./agent";
+
+type FederatedReportingOptions<TContext> = Pick<EngineReportingOptions<TContext>, 'rewriteError'>
+
+const federatedPlugin = <TContext>(
+  options: FederatedReportingOptions<TContext> = Object.create(null),
+): ApolloServerPlugin<TContext> => {
+  return {
+    requestDidStart(requestContext) {
+      const treeBuilder: EngineReportingTreeBuilder =
+        new EngineReportingTreeBuilder({
+          rewriteError: options.rewriteError,
+        });
+
+      // XXX Provide a mechanism to customize this logic.
+      const http = requestContext.request.http;
+      if (
+        !http ||
+        !http.headers ||
+        http.headers.get('apollo-federation-include-trace') !== 'ftv1'
+      ) {
+        return;
+      }
+
+      treeBuilder.startTiming();
+
+      return {
+        willResolveField(...args) {
+          const [ , , , info] = args;
+          return treeBuilder.willResolveField(info);
+        },
+
+        didEncounterErrors({ errors }) {
+          treeBuilder.didEncounterErrors(errors);
+        },
+
+        willSendResponse({ response }) {
+          // We record the end time at the latest possible time: right before serializing the trace.
+          // If we wait any longer, the time we record won't actually be sent anywhere!
+          treeBuilder.stopTiming();
+
+          const encodedUint8Array = Trace.encode(treeBuilder.trace).finish();
+          const encodedBuffer = Buffer.from(
+            encodedUint8Array,
+            encodedUint8Array.byteOffset,
+            encodedUint8Array.byteLength,
+          );
+
+          const extensions =
+            response.extensions || (response.extensions = Object.create(null));
+
+          if (typeof extensions.ftv1 !== "undefined") {
+            throw new Error("The `ftv1` `extensions` were already present.");
+          }
+
+          extensions.ftv1 = encodedBuffer.toString('base64');
+        }
+      }
+    },
+  }
+};
+
+export default federatedPlugin;
diff --git a/packages/apollo-engine-reporting/src/index.ts b/packages/apollo-engine-reporting/src/index.ts
index 36c7e53a85f..82b73e73ad9 100644
--- a/packages/apollo-engine-reporting/src/index.ts
+++ b/packages/apollo-engine-reporting/src/index.ts
@@ -1,2 +1,2 @@
 export { EngineReportingOptions, EngineReportingAgent } from './agent';
-export { plugin as federatedPlugin } from './federatedExtension';
+export { default as federatedPlugin } from './federatedPlugin';
diff --git a/packages/apollo-engine-reporting/src/extension.ts b/packages/apollo-engine-reporting/src/plugin.ts
similarity index 66%
rename from packages/apollo-engine-reporting/src/extension.ts
rename to packages/apollo-engine-reporting/src/plugin.ts
index d3964fd0914..a0179b30ef6 100644
--- a/packages/apollo-engine-reporting/src/extension.ts
+++ b/packages/apollo-engine-reporting/src/plugin.ts
@@ -1,18 +1,10 @@
 import {
   GraphQLRequestContext,
-  WithRequired,
   Logger,
-  SchemaHash,
   InvalidGraphQLRequestError,
 } from 'apollo-server-types';
-import { Request, Headers } from 'apollo-server-env';
-import {
-  GraphQLResolveInfo,
-  DocumentNode,
-  ExecutionArgs,
-  GraphQLError,
-} from 'graphql';
-import { GraphQLExtension, EndHandler } from 'graphql-extensions';
+import { Headers } from 'apollo-server-env';
+import { GraphQLError } from 'graphql';
 import { Trace } from 'apollo-engine-reporting-protobuf';
 
 import {
@@ -35,187 +27,6 @@ const clientNameHeaderKey = 'apollographql-client-name';
 const clientReferenceIdHeaderKey = 'apollographql-client-reference-id';
 const clientVersionHeaderKey = 'apollographql-client-version';
 
-// EngineReportingExtension is the per-request GraphQLExtension which creates a
-// trace (in protobuf Trace format) for a single request. When the request is
-// done, it passes the Trace back to its associated EngineReportingAgent via the
-// addTrace callback in its constructor. This class isn't for direct use; its
-// constructor is a private API for communicating with EngineReportingAgent.
-// Its public methods all implement the GraphQLExtension interface.
-export class EngineReportingExtension<TContext = any>
-  implements GraphQLExtension<TContext> {
-  private logger: Logger = console;
-  private treeBuilder: EngineReportingTreeBuilder;
-  private explicitOperationName?: string | null;
-  private queryString?: string;
-  private documentAST?: DocumentNode;
-  private options: EngineReportingOptions<TContext>;
-  private addTrace: (args: AddTraceArgs) => Promise<void>;
-  private generateClientInfo: GenerateClientInfo<TContext>;
-
-  public constructor(
-    options: EngineReportingOptions<TContext>,
-    addTrace: (args: AddTraceArgs) => Promise<void>,
-    private schemaHash: SchemaHash,
-  ) {
-    this.options = {
-      ...options,
-    };
-    if (options.logger) this.logger = options.logger;
-    this.addTrace = addTrace;
-    this.generateClientInfo =
-      options.generateClientInfo || defaultGenerateClientInfo;
-
-    this.treeBuilder = new EngineReportingTreeBuilder({
-      rewriteError: options.rewriteError,
-      logger: this.logger,
-    });
-  }
-
-  public requestDidStart(o: {
-    request: Request;
-    queryString?: string;
-    parsedQuery?: DocumentNode;
-    variables?: Record<string, any>;
-    context: TContext;
-    extensions?: Record<string, any>;
-    requestContext: WithRequired<
-      GraphQLRequestContext<TContext>,
-      'metrics' | 'queryHash'
-    >;
-  }): EndHandler {
-    this.treeBuilder.startTiming();
-    o.requestContext.metrics.startHrTime = this.treeBuilder.startHrTime;
-
-    // Generally, we'll get queryString here and not parsedQuery; we only get
-    // parsedQuery if you're using an OperationStore. In normal cases we'll get
-    // our documentAST in the execution callback after it is parsed.
-    const queryHash = o.requestContext.queryHash;
-    this.queryString = o.queryString;
-    this.documentAST = o.parsedQuery;
-
-    this.treeBuilder.trace.http = new Trace.HTTP({
-      method:
-        Trace.HTTP.Method[o.request.method as keyof typeof Trace.HTTP.Method] ||
-        Trace.HTTP.Method.UNKNOWN,
-      // Host and path are not used anywhere on the backend, so let's not bother
-      // trying to parse request.url to get them, which is a potential
-      // source of bugs because integrations have different behavior here.
-      // On Node's HTTP module, request.url only includes the path
-      // (see https://nodejs.org/api/http.html#http_message_url)
-      // The same is true on Lambda (where we pass event.path)
-      // But on environments like Cloudflare we do get a complete URL.
-      host: null,
-      path: null,
-    });
-
-    if (this.options.sendHeaders) {
-      makeHTTPRequestHeaders(
-        this.treeBuilder.trace.http,
-        o.request.headers,
-        this.options.sendHeaders,
-      );
-
-      if (o.requestContext.metrics.persistedQueryHit) {
-        this.treeBuilder.trace.persistedQueryHit = true;
-      }
-      if (o.requestContext.metrics.persistedQueryRegister) {
-        this.treeBuilder.trace.persistedQueryRegister = true;
-      }
-    }
-
-    if (o.variables) {
-      this.treeBuilder.trace.details = makeTraceDetails(
-        o.variables,
-        this.options.sendVariableValues,
-        o.queryString,
-      );
-    }
-
-    const clientInfo = this.generateClientInfo(o.requestContext);
-    if (clientInfo) {
-      // While clientAddress could be a part of the protobuf, we'll ignore it for
-      // now, since the backend does not group by it and Engine frontend will not
-      // support it in the short term
-      const { clientName, clientVersion, clientReferenceId } = clientInfo;
-      // the backend makes the choice of mapping clientName => clientReferenceId if
-      // no custom reference id is provided
-      this.treeBuilder.trace.clientVersion = clientVersion || '';
-      this.treeBuilder.trace.clientReferenceId = clientReferenceId || '';
-      this.treeBuilder.trace.clientName = clientName || '';
-    }
-
-    return () => {
-      this.treeBuilder.stopTiming();
-
-      this.treeBuilder.trace.fullQueryCacheHit = !!o.requestContext.metrics
-        .responseCacheHit;
-      this.treeBuilder.trace.forbiddenOperation = !!o.requestContext.metrics
-        .forbiddenOperation;
-      this.treeBuilder.trace.registeredOperation = !!o.requestContext.metrics
-        .registeredOperation;
-
-      // If the user did not explicitly specify an operation name (which we
-      // would have saved in `executionDidStart`), but the request pipeline made
-      // it far enough to figure out what the operation name must be and store
-      // it on requestContext.operationName, use that name.  (Note that this
-      // depends on the assumption that the RequestContext passed to
-      // requestDidStart, which does not yet have operationName, will be mutated
-      // to add operationName later.)
-      const operationName =
-        this.explicitOperationName || o.requestContext.operationName || '';
-      const documentAST = this.documentAST || o.requestContext.document;
-
-      // If this was a federated operation and we're the gateway, add the query plan
-      // to the trace.
-      if (o.requestContext.metrics.queryPlanTrace) {
-        this.treeBuilder.trace.queryPlan =
-          o.requestContext.metrics.queryPlanTrace;
-      }
-
-      this.addTrace({
-        operationName,
-        queryHash,
-        documentAST,
-        queryString: this.queryString || '',
-        trace: this.treeBuilder.trace,
-        schemaHash: this.schemaHash,
-      });
-    };
-  }
-
-  public executionDidStart(o: { executionArgs: ExecutionArgs }) {
-    // If the operationName is explicitly provided, save it. Note: this is the
-    // operationName provided by the user. It might be empty if they're relying on
-    // the "just use the only operation I sent" behavior, even if that operation
-    // has a name.
-    //
-    // It's possible that execution is about to fail because this operation
-    // isn't actually in the document. We want to know the name in that case
-    // too, which is why it's important that we save the name now, and not just
-    // rely on requestContext.operationName (which will be null in this case).
-    if (o.executionArgs.operationName) {
-      this.explicitOperationName = o.executionArgs.operationName;
-    }
-    this.documentAST = o.executionArgs.document;
-  }
-
-  public willResolveField(
-    _source: any,
-    _args: { [argName: string]: any },
-    _context: TContext,
-    info: GraphQLResolveInfo,
-  ): ((error: Error | null, result: any) => void) | void {
-    return this.treeBuilder.willResolveField(info);
-    // We could save the error into the trace during the end handler, but it
-    // won't have all the information that graphql-js adds to it later, like
-    // 'locations'.
-  }
-
-  public didEncounterErrors(errors: GraphQLError[]) {
-    this.treeBuilder.didEncounterErrors(errors);
-  }
-}
-
 // This plugin is instantiated once at server start-up. Each request that the
 // server processes will invoke the `requestDidStart` method which will produce
 // a trace (in protobuf Trace format) for that single request. When the request
diff --git a/packages/apollo-engine-reporting/tsconfig.json b/packages/apollo-engine-reporting/tsconfig.json
index 1e8b57f68f2..ea64c712362 100644
--- a/packages/apollo-engine-reporting/tsconfig.json
+++ b/packages/apollo-engine-reporting/tsconfig.json
@@ -7,7 +7,6 @@
   "include": ["src/**/*"],
   "exclude": ["**/__tests__", "**/__mocks__"],
   "references": [
-    { "path": "../graphql-extensions" },
     { "path": "../apollo-server-errors" },
     { "path": "../apollo-server-types" },
     { "path": "../apollo-server-plugin-base" },

From dce4a241435107d463126224d22f403cfd4110e6 Mon Sep 17 00:00:00 2001
From: Jesse Rosenberger <git@jro.cc>
Date: Mon, 27 Apr 2020 17:33:00 +0000
Subject: [PATCH 04/27] fix!: Rename AERs `newExtension` to `newPlugin` to
 match new usage.

Ref: #3998
---
 packages/apollo-engine-reporting/src/agent.ts   | 2 +-
 packages/apollo-server-core/src/ApolloServer.ts | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/packages/apollo-engine-reporting/src/agent.ts b/packages/apollo-engine-reporting/src/agent.ts
index 71de2c2f08c..6931425e666 100644
--- a/packages/apollo-engine-reporting/src/agent.ts
+++ b/packages/apollo-engine-reporting/src/agent.ts
@@ -279,7 +279,7 @@ export class EngineReportingAgent<TContext = any> {
     handleLegacyOptions(this.options);
   }
 
-  public newExtension(): ApolloServerPlugin<TContext> {
+  public newPlugin(): ApolloServerPlugin<TContext> {
     return plugin(this.options, this.addTrace.bind(this));
   }
 
diff --git a/packages/apollo-server-core/src/ApolloServer.ts b/packages/apollo-server-core/src/ApolloServer.ts
index 56afdc34a1c..aad31015c9c 100644
--- a/packages/apollo-server-core/src/ApolloServer.ts
+++ b/packages/apollo-server-core/src/ApolloServer.ts
@@ -775,7 +775,7 @@ export class ApolloServerBase {
             'to report metrics to Apollo Graph Manager.',
         );
       }
-      pluginsToInit.push(this.engineReportingAgent!.newExtension());
+      pluginsToInit.push(this.engineReportingAgent!.newPlugin());
     } else if (engine !== false && federatedSchema) {
       // We haven't configured this app to use Engine directly. But it looks like
       // we are a federated service backend, so we should be capable of including

From a8ab841e2cdd0ba8030c914329003b8ec30a438b Mon Sep 17 00:00:00 2001
From: Jesse Rosenberger <git@jro.cc>
Date: Mon, 27 Apr 2020 17:41:42 +0000
Subject: [PATCH 05/27] no-op: Add back comment about `ftv1` trace format.

This was inadvertently removed in the re-factor.

Ref: https://github.com/apollographql/apollo-server/pull/3998/files#r414901517
---
 packages/apollo-engine-reporting/src/federatedPlugin.ts | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/packages/apollo-engine-reporting/src/federatedPlugin.ts b/packages/apollo-engine-reporting/src/federatedPlugin.ts
index 5bad85c252b..81d168f9b18 100644
--- a/packages/apollo-engine-reporting/src/federatedPlugin.ts
+++ b/packages/apollo-engine-reporting/src/federatedPlugin.ts
@@ -5,6 +5,8 @@ import { EngineReportingOptions } from "./agent";
 
 type FederatedReportingOptions<TContext> = Pick<EngineReportingOptions<TContext>, 'rewriteError'>
 
+// This ftv1 plugin produces a base64'd Trace protobuf containing only the
+// durationNs, startTime, endTime, and root fields.
 const federatedPlugin = <TContext>(
   options: FederatedReportingOptions<TContext> = Object.create(null),
 ): ApolloServerPlugin<TContext> => {

From fc05e8d6a32973cbca6561eb140d240040117e66 Mon Sep 17 00:00:00 2001
From: Jesse Rosenberger <git@jro.cc>
Date: Tue, 28 Apr 2020 11:34:34 +0000
Subject: [PATCH 06/27] Use optional chaining when accessing optional
 `request.http.headers`.

...and destructuring.
---
 packages/apollo-engine-reporting/src/federatedPlugin.ts | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/packages/apollo-engine-reporting/src/federatedPlugin.ts b/packages/apollo-engine-reporting/src/federatedPlugin.ts
index 81d168f9b18..bf1b04627f6 100644
--- a/packages/apollo-engine-reporting/src/federatedPlugin.ts
+++ b/packages/apollo-engine-reporting/src/federatedPlugin.ts
@@ -11,19 +11,14 @@ const federatedPlugin = <TContext>(
   options: FederatedReportingOptions<TContext> = Object.create(null),
 ): ApolloServerPlugin<TContext> => {
   return {
-    requestDidStart(requestContext) {
+    requestDidStart({ request: { http } }) {
       const treeBuilder: EngineReportingTreeBuilder =
         new EngineReportingTreeBuilder({
           rewriteError: options.rewriteError,
         });
 
       // XXX Provide a mechanism to customize this logic.
-      const http = requestContext.request.http;
-      if (
-        !http ||
-        !http.headers ||
-        http.headers.get('apollo-federation-include-trace') !== 'ftv1'
-      ) {
+      if (http?.headers.get('apollo-federation-include-trace') !== 'ftv1') {
         return;
       }
 

From 48eab7efa0893182452680ba383df218da862273 Mon Sep 17 00:00:00 2001
From: Jesse Rosenberger <git@jro.cc>
Date: Tue, 28 Apr 2020 12:27:11 +0000
Subject: [PATCH 07/27] Ensure `metrics` is present before plugin
 initialization.

This eliminates the need to guard for the presence of `metrics` on the
`requestContext` within plugins who are calling `requestDidStart`.

Ref: https://github.com/apollographql/apollo-server/pull/3998#discussion_r414906840
---
 packages/apollo-server-core/src/ApolloServer.ts   |  1 +
 .../apollo-server-core/src/requestPipeline.ts     | 15 ++++++++++-----
 packages/apollo-server-types/src/index.ts         |  2 +-
 3 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/packages/apollo-server-core/src/ApolloServer.ts b/packages/apollo-server-core/src/ApolloServer.ts
index aad31015c9c..5f020df75e0 100644
--- a/packages/apollo-server-core/src/ApolloServer.ts
+++ b/packages/apollo-server-core/src/ApolloServer.ts
@@ -869,6 +869,7 @@ export class ApolloServerBase {
       request,
       context: options.context || Object.create(null),
       cache: options.cache!,
+      metrics: {},
       response: {
         http: {
           headers: new Headers(),
diff --git a/packages/apollo-server-core/src/requestPipeline.ts b/packages/apollo-server-core/src/requestPipeline.ts
index 6393d82c95a..d7958763c98 100644
--- a/packages/apollo-server-core/src/requestPipeline.ts
+++ b/packages/apollo-server-core/src/requestPipeline.ts
@@ -126,6 +126,16 @@ export async function processGraphQLRequest<TContext>(
   // all of our own machinery will certainly set it now.
   const logger = requestContext.logger || console;
 
+  // If request context's `metrics` already exists, preserve it, but _ensure_ it
+  // exists there and shorthand it for use throughout this function. As of this
+  // comment, the sole known case where `metrics` already exists is when the
+  // `captureTraces` property is present and set to the result of the boolean
+  // `reporting` option on the legacy (V1) server options, here:
+  // https://git.io/Jfmsb. I suspect this disappears when this is the direct
+  // entry into request processing, rather than through, e.g. `runHttpQuery`.
+  const metrics = requestContext.metrics =
+    requestContext.metrics || Object.create(null);
+
   let cacheControlExtension: CacheControlExtension | undefined;
   const extensionStack = initializeExtensionStack();
   (requestContext.context as any)._extensionStack = extensionStack;
@@ -137,11 +147,6 @@ export async function processGraphQLRequest<TContext>(
 
   await initializeDataSources();
 
-  const metrics = requestContext.metrics || Object.create(null);
-  if (!requestContext.metrics) {
-    requestContext.metrics = metrics;
-  }
-
   const request = requestContext.request;
 
   let { query, extensions } = request;
diff --git a/packages/apollo-server-types/src/index.ts b/packages/apollo-server-types/src/index.ts
index fcf42426b1d..e9c7cdd5445 100644
--- a/packages/apollo-server-types/src/index.ts
+++ b/packages/apollo-server-types/src/index.ts
@@ -104,7 +104,7 @@ export interface GraphQLRequestContext<TContext = Record<string, any>> {
    */
   readonly errors?: ReadonlyArray<GraphQLError>;
 
-  readonly metrics?: GraphQLRequestMetrics;
+  readonly metrics: GraphQLRequestMetrics;
 
   debug?: boolean;
 }

From 8ca5d112b38796c28a1e2d7211af169136c8bf5b Mon Sep 17 00:00:00 2001
From: Jesse Rosenberger <git@jro.cc>
Date: Tue, 28 Apr 2020 12:33:53 +0000
Subject: [PATCH 08/27] Remove guard around `metrics` which is unnecessary
 after 48eab7efa.

---
 packages/apollo-engine-reporting/src/plugin.ts | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/packages/apollo-engine-reporting/src/plugin.ts b/packages/apollo-engine-reporting/src/plugin.ts
index a0179b30ef6..661e0c03e2b 100644
--- a/packages/apollo-engine-reporting/src/plugin.ts
+++ b/packages/apollo-engine-reporting/src/plugin.ts
@@ -21,8 +21,6 @@ import {
   PersistedQueryNotSupportedError,
 } from 'apollo-server-errors';
 
-type Mutable<T> = { -readonly [P in keyof T]: T[P] };
-
 const clientNameHeaderKey = 'apollographql-client-name';
 const clientReferenceIdHeaderKey = 'apollographql-client-reference-id';
 const clientVersionHeaderKey = 'apollographql-client-version';
@@ -52,11 +50,9 @@ export const plugin = <TContext>(
           logger: requestContext.logger || logger,
         });
 
-      const metrics: NonNullable<typeof requestContext['metrics']> =
-        ((requestContext as Mutable<typeof requestContext>)
-          .metrics = requestContext.metrics || Object.create(null));
-
       treeBuilder.startTiming();
+
+      const metrics = requestContext.metrics;
       metrics.startHrTime = treeBuilder.startHrTime;
 
       if (requestContext.request.http) {

From 0658df5e50283c3985ff3e3717bdff8de2adab9d Mon Sep 17 00:00:00 2001
From: Jesse Rosenberger <git@jro.cc>
Date: Tue, 28 Apr 2020 13:21:58 +0000
Subject: [PATCH 09/27] Move `Trace.HTTP` init inside of `ensurePreflight`.

As noted in review within [[1]], there wasn't really a compelling reason for
this to be kept separately from the other tree-building bits which existed
within `ensurePreflight`.

[1]: https://github.com/apollographql/apollo-server/pull/3998#discussion_r414911267
---
 .../apollo-engine-reporting/src/plugin.ts     | 40 +++++++++----------
 1 file changed, 19 insertions(+), 21 deletions(-)

diff --git a/packages/apollo-engine-reporting/src/plugin.ts b/packages/apollo-engine-reporting/src/plugin.ts
index 661e0c03e2b..dd42f28b434 100644
--- a/packages/apollo-engine-reporting/src/plugin.ts
+++ b/packages/apollo-engine-reporting/src/plugin.ts
@@ -55,32 +55,30 @@ export const plugin = <TContext>(
       const metrics = requestContext.metrics;
       metrics.startHrTime = treeBuilder.startHrTime;
 
-      if (requestContext.request.http) {
-        treeBuilder.trace.http = new Trace.HTTP({
-          method:
-            Trace.HTTP.Method[
-              requestContext.request.http
-                .method as keyof typeof Trace.HTTP.Method
-            ] || Trace.HTTP.Method.UNKNOWN,
-          // Host and path are not used anywhere on the backend, so let's not bother
-          // trying to parse request.url to get them, which is a potential
-          // source of bugs because integrations have different behavior here.
-          // On Node's HTTP module, request.url only includes the path
-          // (see https://nodejs.org/api/http.html#http_message_url)
-          // The same is true on Lambda (where we pass event.path)
-          // But on environments like Cloudflare we do get a complete URL.
-          host: null,
-          path: null,
-        });
-      }
-
       let preflightDone: boolean = false;
       function ensurePreflight() {
         if (preflightDone) return;
         preflightDone = true;
 
-        if (options.sendHeaders) {
-          if (requestContext.request.http && treeBuilder.trace.http) {
+        if (requestContext.request.http) {
+          treeBuilder.trace.http = new Trace.HTTP({
+            method:
+              Trace.HTTP.Method[
+                requestContext.request.http
+                  .method as keyof typeof Trace.HTTP.Method
+              ] || Trace.HTTP.Method.UNKNOWN,
+            // Host and path are not used anywhere on the backend, so let's not bother
+            // trying to parse request.url to get them, which is a potential
+            // source of bugs because integrations have different behavior here.
+            // On Node's HTTP module, request.url only includes the path
+            // (see https://nodejs.org/api/http.html#http_message_url)
+            // The same is true on Lambda (where we pass event.path)
+            // But on environments like Cloudflare we do get a complete URL.
+            host: null,
+            path: null,
+          });
+
+          if (options.sendHeaders) {
             makeHTTPRequestHeaders(
               treeBuilder.trace.http,
               requestContext.request.http.headers,

From fc966a4b5d383d7a0d893839e4b05d9c5b87afaa Mon Sep 17 00:00:00 2001
From: Jesse Rosenberger <git@jro.cc>
Date: Tue, 28 Apr 2020 13:34:10 +0000
Subject: [PATCH 10/27] Remove unnecessary `queryString` assignment and related
 comment.

---
 packages/apollo-engine-reporting/src/plugin.ts | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/packages/apollo-engine-reporting/src/plugin.ts b/packages/apollo-engine-reporting/src/plugin.ts
index dd42f28b434..4416d422567 100644
--- a/packages/apollo-engine-reporting/src/plugin.ts
+++ b/packages/apollo-engine-reporting/src/plugin.ts
@@ -43,7 +43,6 @@ export const plugin = <TContext>(
 
   return {
     requestDidStart(requestContext) {
-      let queryString: string | undefined;
       const treeBuilder: EngineReportingTreeBuilder =
         new EngineReportingTreeBuilder({
           rewriteError: options.rewriteError,
@@ -94,16 +93,11 @@ export const plugin = <TContext>(
           treeBuilder.trace.persistedQueryRegister = true;
         }
 
-        // Generally, we'll get queryString here and not parsedQuery; we only get
-        // parsedQuery if you're using an OperationStore. In normal cases we'll
-        // get our documentAST in the execution callback after it is parsed.
-        queryString = requestContext.source;
-
         if (requestContext.request.variables) {
           treeBuilder.trace.details = makeTraceDetails(
             requestContext.request.variables,
             options.sendVariableValues,
-            queryString,
+            requestContext.source,
           );
         }
 
@@ -150,7 +144,7 @@ export const plugin = <TContext>(
           operationName,
           queryHash: requestContext.queryHash!,
           documentAST: requestContext.document,
-          queryString,
+          queryString: requestContext.source,
           trace: treeBuilder.trace,
           schemaHash: requestContext.schemaHash,
         });

From 766c7384c8296a5a552973ede24f013fb355dab5 Mon Sep 17 00:00:00 2001
From: Jesse Rosenberger <git@jro.cc>
Date: Tue, 28 Apr 2020 13:47:47 +0000
Subject: [PATCH 11/27] Destructure some `requestContext` properties for
 brevity.

---
 .../apollo-engine-reporting/src/plugin.ts     | 28 +++++++++++--------
 1 file changed, 17 insertions(+), 11 deletions(-)

diff --git a/packages/apollo-engine-reporting/src/plugin.ts b/packages/apollo-engine-reporting/src/plugin.ts
index 4416d422567..289114eece4 100644
--- a/packages/apollo-engine-reporting/src/plugin.ts
+++ b/packages/apollo-engine-reporting/src/plugin.ts
@@ -43,15 +43,23 @@ export const plugin = <TContext>(
 
   return {
     requestDidStart(requestContext) {
+      // We still need the entire `requestContext` to pass through to the
+      // `generateClientInfo` method, but we'll destructure for brevity within.
+      const {
+        metrics,
+        logger: requestLogger,
+        schemaHash,
+        request: { http, variables },
+      } = requestContext;
+
       const treeBuilder: EngineReportingTreeBuilder =
         new EngineReportingTreeBuilder({
           rewriteError: options.rewriteError,
-          logger: requestContext.logger || logger,
+          logger: requestLogger || logger,
         });
 
       treeBuilder.startTiming();
 
-      const metrics = requestContext.metrics;
       metrics.startHrTime = treeBuilder.startHrTime;
 
       let preflightDone: boolean = false;
@@ -59,13 +67,11 @@ export const plugin = <TContext>(
         if (preflightDone) return;
         preflightDone = true;
 
-        if (requestContext.request.http) {
+        if (http) {
           treeBuilder.trace.http = new Trace.HTTP({
             method:
-              Trace.HTTP.Method[
-                requestContext.request.http
-                  .method as keyof typeof Trace.HTTP.Method
-              ] || Trace.HTTP.Method.UNKNOWN,
+              Trace.HTTP.Method[http.method as keyof typeof Trace.HTTP.Method]
+                || Trace.HTTP.Method.UNKNOWN,
             // Host and path are not used anywhere on the backend, so let's not bother
             // trying to parse request.url to get them, which is a potential
             // source of bugs because integrations have different behavior here.
@@ -80,7 +86,7 @@ export const plugin = <TContext>(
           if (options.sendHeaders) {
             makeHTTPRequestHeaders(
               treeBuilder.trace.http,
-              requestContext.request.http.headers,
+              http.headers,
               options.sendHeaders,
             );
           }
@@ -93,9 +99,9 @@ export const plugin = <TContext>(
           treeBuilder.trace.persistedQueryRegister = true;
         }
 
-        if (requestContext.request.variables) {
+        if (variables) {
           treeBuilder.trace.details = makeTraceDetails(
-            requestContext.request.variables,
+            variables,
             options.sendVariableValues,
             requestContext.source,
           );
@@ -146,7 +152,7 @@ export const plugin = <TContext>(
           documentAST: requestContext.document,
           queryString: requestContext.source,
           trace: treeBuilder.trace,
-          schemaHash: requestContext.schemaHash,
+          schemaHash,
         });
       }
 

From f4aad306d1737dc411635c9538aaef992776ccf0 Mon Sep 17 00:00:00 2001
From: Jesse Rosenberger <git@jro.cc>
Date: Tue, 28 Apr 2020 13:52:10 +0000
Subject: [PATCH 12/27] chore!: Use `document` / `source` rather than
 `documentAST` / `queryString`.

The use of `document` and `source` is the standard within the plugin API and
the request pipeline, so these names should be more natural going forward.

This wouldn't have been possible without a breaking change, but we're
already doing that.
---
 packages/apollo-engine-reporting/src/agent.ts | 30 +++++++++----------
 .../apollo-engine-reporting/src/plugin.ts     |  4 +--
 2 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/packages/apollo-engine-reporting/src/agent.ts b/packages/apollo-engine-reporting/src/agent.ts
index 6931425e666..0bde648580f 100644
--- a/packages/apollo-engine-reporting/src/agent.ts
+++ b/packages/apollo-engine-reporting/src/agent.ts
@@ -204,8 +204,8 @@ export interface AddTraceArgs {
   operationName: string;
   queryHash: string;
   schemaHash: SchemaHash;
-  queryString?: string;
-  documentAST?: DocumentNode;
+  source?: string;
+  document?: DocumentNode;
 }
 
 const serviceHeaderDefaults = {
@@ -286,9 +286,9 @@ export class EngineReportingAgent<TContext = any> {
   public async addTrace({
     trace,
     queryHash,
-    documentAST,
+    document,
     operationName,
-    queryString,
+    source,
     schemaHash,
   }: AddTraceArgs): Promise<void> {
     // Ignore traces that come in after stop().
@@ -320,8 +320,8 @@ export class EngineReportingAgent<TContext = any> {
 
     const signature = await this.getTraceSignature({
       queryHash,
-      documentAST,
-      queryString,
+      document,
+      source,
       operationName,
     });
 
@@ -486,17 +486,17 @@ export class EngineReportingAgent<TContext = any> {
   private async getTraceSignature({
     queryHash,
     operationName,
-    documentAST,
-    queryString,
+    document,
+    source,
   }: {
     queryHash: string;
     operationName: string;
-    documentAST?: DocumentNode;
-    queryString?: string;
+    document?: DocumentNode;
+    source?: string;
   }): Promise<string> {
-    if (!documentAST && !queryString) {
+    if (!document && !source) {
       // This shouldn't happen: one of those options must be passed to runQuery.
-      throw new Error('No queryString or parsedQuery?');
+      throw new Error('No document or source?');
     }
 
     const cacheKey = signatureCacheKey(queryHash, operationName);
@@ -511,7 +511,7 @@ export class EngineReportingAgent<TContext = any> {
       return cachedSignature;
     }
 
-    if (!documentAST) {
+    if (!document) {
       // We didn't get an AST, possibly because of a parse failure. Let's just
       // use the full query string.
       //
@@ -519,12 +519,12 @@ export class EngineReportingAgent<TContext = any> {
       //     hides literals, you might end up sending literals for queries
       //     that fail parsing or validation. Provide some way to mask them
       //     anyway?
-      return queryString as string;
+      return source as string;
     }
 
     const generatedSignature = (
       this.options.calculateSignature || defaultEngineReportingSignature
-    )(documentAST, operationName);
+    )(document, operationName);
 
     // Intentionally not awaited so the cache can be written to at leisure.
     this.signatureCache.set(cacheKey, generatedSignature);
diff --git a/packages/apollo-engine-reporting/src/plugin.ts b/packages/apollo-engine-reporting/src/plugin.ts
index 289114eece4..cc950c558eb 100644
--- a/packages/apollo-engine-reporting/src/plugin.ts
+++ b/packages/apollo-engine-reporting/src/plugin.ts
@@ -149,8 +149,8 @@ export const plugin = <TContext>(
         addTrace({
           operationName,
           queryHash: requestContext.queryHash!,
-          documentAST: requestContext.document,
-          queryString: requestContext.source,
+          document: requestContext.document,
+          source: requestContext.source,
           trace: treeBuilder.trace,
           schemaHash,
         });

From 6b0c83cf3a787d9b3d035a0a171e86cbf5442d9c Mon Sep 17 00:00:00 2001
From: Jesse Rosenberger <git@jro.cc>
Date: Tue, 28 Apr 2020 17:52:33 +0000
Subject: [PATCH 13/27] Expand on comment about the `ftv1` extension.

Per @glasser's https://github.com/apollographql/apollo-server/commit/a8ab841e#r38802002
---
 packages/apollo-engine-reporting/src/federatedPlugin.ts | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/packages/apollo-engine-reporting/src/federatedPlugin.ts b/packages/apollo-engine-reporting/src/federatedPlugin.ts
index bf1b04627f6..e69e6cc92e4 100644
--- a/packages/apollo-engine-reporting/src/federatedPlugin.ts
+++ b/packages/apollo-engine-reporting/src/federatedPlugin.ts
@@ -6,7 +6,10 @@ import { EngineReportingOptions } from "./agent";
 type FederatedReportingOptions<TContext> = Pick<EngineReportingOptions<TContext>, 'rewriteError'>
 
 // This ftv1 plugin produces a base64'd Trace protobuf containing only the
-// durationNs, startTime, endTime, and root fields.
+// durationNs, startTime, endTime, and root fields.  This output is placed
+// on the `extensions`.`ftv1` property of the response.  The Apollo Gateway
+// utilizes this data to construct the full trace and submit it to Apollo
+// Graph Manager ingress.
 const federatedPlugin = <TContext>(
   options: FederatedReportingOptions<TContext> = Object.create(null),
 ): ApolloServerPlugin<TContext> => {

From 283e82f307774b68c560786c07cf25c717c69299 Mon Sep 17 00:00:00 2001
From: Jesse Rosenberger <git@jro.cc>
Date: Thu, 7 May 2020 09:59:35 +0000
Subject: [PATCH 14/27] Tweak err. message when `ftv1` is already present in
 `extensions` response.

Ref: https://github.com/apollographql/apollo-server/pull/3998/files#r414902801
---
 packages/apollo-engine-reporting/src/federatedPlugin.ts | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/packages/apollo-engine-reporting/src/federatedPlugin.ts b/packages/apollo-engine-reporting/src/federatedPlugin.ts
index e69e6cc92e4..f02ce267fca 100644
--- a/packages/apollo-engine-reporting/src/federatedPlugin.ts
+++ b/packages/apollo-engine-reporting/src/federatedPlugin.ts
@@ -52,8 +52,10 @@ const federatedPlugin = <TContext>(
           const extensions =
             response.extensions || (response.extensions = Object.create(null));
 
+          // This should only happen if another plugin is using the same name-
+          // space within the `extensions` object and got to it before us.
           if (typeof extensions.ftv1 !== "undefined") {
-            throw new Error("The `ftv1` `extensions` were already present.");
+            throw new Error("The `ftv1` extension was already present.");
           }
 
           extensions.ftv1 = encodedBuffer.toString('base64');

From e0949ec20b0e6eee1dc0f24e4d2e370e40012137 Mon Sep 17 00:00:00 2001
From: Jesse Rosenberger <git@jro.cc>
Date: Thu, 7 May 2020 10:10:17 +0000
Subject: [PATCH 15/27] Remove `TODO` comment I suggested I'd remove!

Ref: https://github.com/apollographql/apollo-server/pull/3998/files#r409729299
---
 packages/apollo-server-core/src/ApolloServer.ts | 1 -
 1 file changed, 1 deletion(-)

diff --git a/packages/apollo-server-core/src/ApolloServer.ts b/packages/apollo-server-core/src/ApolloServer.ts
index d0f935ff36a..3baa43237f7 100644
--- a/packages/apollo-server-core/src/ApolloServer.ts
+++ b/packages/apollo-server-core/src/ApolloServer.ts
@@ -757,7 +757,6 @@ export class ApolloServerBase {
     // User's plugins, provided as an argument to this method, will be added
     // at the end of that list so they take precedence.
     // A follow-up commit will actually introduce this.
-    // Also, TODO, remove this comment.
 
     const federatedSchema = this.schema && this.schemaIsFederated(this.schema);
     const { engine } = this.config;

From e813e5b73a2bb14a93af9f91fd2ede6684c1ecae Mon Sep 17 00:00:00 2001
From: Jesse Rosenberger <git@jro.cc>
Date: Thu, 7 May 2020 12:07:04 +0000
Subject: [PATCH 16/27] refactor(tests): Better helpers for APQ tests in intgr.
 testsuite.

---
 .../src/index.ts                              | 72 +++++++++++--------
 1 file changed, 42 insertions(+), 30 deletions(-)

diff --git a/packages/apollo-server-integration-testsuite/src/index.ts b/packages/apollo-server-integration-testsuite/src/index.ts
index a5dd026108f..a2618f328b7 100644
--- a/packages/apollo-server-integration-testsuite/src/index.ts
+++ b/packages/apollo-server-integration-testsuite/src/index.ts
@@ -18,7 +18,12 @@ import {
 
 import request from 'supertest';
 
-import { GraphQLOptions, Config } from 'apollo-server-core';
+import {
+  GraphQLOptions,
+  Config,
+  PersistedQueryOptions,
+  KeyValueCache,
+} from 'apollo-server-core';
 import gql from 'graphql-tag';
 import { ValueOrPromise } from 'apollo-server-types';
 import { GraphQLRequestListener } from "apollo-server-plugin-base";
@@ -1221,12 +1226,7 @@ export default (createApp: CreateAppFunc, destroyApp?: DestroyAppFunc) => {
         },
       };
 
-      let didEncounterErrors: jest.Mock<
-        ReturnType<GraphQLRequestListener['didEncounterErrors']>,
-        Parameters<GraphQLRequestListener['didEncounterErrors']>
-      >;
-
-      function createMockCache() {
+      function createMockCache(): KeyValueCache {
         const map = new Map<string, string>();
         return {
           set: jest.fn(async (key, val) => {
@@ -1237,10 +1237,13 @@ export default (createApp: CreateAppFunc, destroyApp?: DestroyAppFunc) => {
         };
       }
 
-      beforeEach(async () => {
-        didEncounterErrors = jest.fn();
-        const cache = createMockCache();
-        app = await createApp({
+      let didEncounterErrors: jest.Mock<
+        ReturnType<GraphQLRequestListener['didEncounterErrors']>,
+        Parameters<GraphQLRequestListener['didEncounterErrors']>
+      >;
+
+      function createApqApp(apqOptions: PersistedQueryOptions = {}) {
+        return createApp({
           graphqlOptions: {
             schema,
             plugins: [
@@ -1252,22 +1255,21 @@ export default (createApp: CreateAppFunc, destroyApp?: DestroyAppFunc) => {
             ],
             persistedQueries: {
               cache,
+              ...apqOptions,
             },
           },
         });
+      }
+
+      let cache: KeyValueCache;
+
+      beforeEach(async () => {
+        cache = createMockCache();
+        didEncounterErrors = jest.fn();
       });
 
       it('when ttlSeconds is set, passes ttl to the apq cache set call', async () => {
-        const cache = createMockCache();
-        app = await createApp({
-          graphqlOptions: {
-            schema,
-            persistedQueries: {
-              cache: cache,
-              ttl: 900,
-            },
-          },
-        });
+        app = await createApqApp({ ttl: 900 });
 
         await request(app)
           .post('/graphql')
@@ -1287,15 +1289,7 @@ export default (createApp: CreateAppFunc, destroyApp?: DestroyAppFunc) => {
 
       it('when ttlSeconds is unset, ttl is not passed to apq cache',
         async () => {
-          const cache = createMockCache();
-          app = await createApp({
-            graphqlOptions: {
-              schema,
-              persistedQueries: {
-                cache: cache,
-              },
-            },
-          });
+          app = await createApqApp();
 
           await request(app)
             .post('/graphql')
@@ -1315,6 +1309,8 @@ export default (createApp: CreateAppFunc, destroyApp?: DestroyAppFunc) => {
       );
 
       it('errors when version is not specified', async () => {
+        app = await createApqApp();
+
         const result = await request(app)
           .get('/graphql')
           .query({
@@ -1346,6 +1342,8 @@ export default (createApp: CreateAppFunc, destroyApp?: DestroyAppFunc) => {
       });
 
       it('errors when version is unsupported', async () => {
+        app = await createApqApp();
+
         const result = await request(app)
           .get('/graphql')
           .query({
@@ -1378,6 +1376,8 @@ export default (createApp: CreateAppFunc, destroyApp?: DestroyAppFunc) => {
       });
 
       it('errors when hash is mismatched', async () => {
+        app = await createApqApp();
+
         const result = await request(app)
           .get('/graphql')
           .query({
@@ -1410,6 +1410,8 @@ export default (createApp: CreateAppFunc, destroyApp?: DestroyAppFunc) => {
       });
 
       it('returns PersistedQueryNotFound on the first try', async () => {
+        app = await createApqApp();
+
         const result = await request(app)
           .post('/graphql')
           .send({
@@ -1432,6 +1434,8 @@ export default (createApp: CreateAppFunc, destroyApp?: DestroyAppFunc) => {
         );
       });
       it('returns result on the second try', async () => {
+        app = await createApqApp();
+
         await request(app)
           .post('/graphql')
           .send({
@@ -1465,6 +1469,8 @@ export default (createApp: CreateAppFunc, destroyApp?: DestroyAppFunc) => {
       });
 
       it('returns with batched persisted queries', async () => {
+        app = await createApqApp();
+
         const errors = await request(app)
           .post('/graphql')
           .send([
@@ -1510,6 +1516,8 @@ export default (createApp: CreateAppFunc, destroyApp?: DestroyAppFunc) => {
       });
 
       it('returns result on the persisted query', async () => {
+        app = await createApqApp();
+
         await request(app)
           .post('/graphql')
           .send({
@@ -1532,6 +1540,8 @@ export default (createApp: CreateAppFunc, destroyApp?: DestroyAppFunc) => {
       });
 
       it('returns error when hash does not match', async () => {
+        app = await createApqApp();
+
         const response = await request(app)
           .post('/graphql')
           .send({
@@ -1549,6 +1559,8 @@ export default (createApp: CreateAppFunc, destroyApp?: DestroyAppFunc) => {
       });
 
       it('returns correct result using get request', async () => {
+        app = await createApqApp();
+
         await request(app)
           .post('/graphql')
           .send({

From d91358911429b0db4866ae0b43878becb5e49134 Mon Sep 17 00:00:00 2001
From: Jesse Rosenberger <git@jro.cc>
Date: Thu, 7 May 2020 12:26:51 +0000
Subject: [PATCH 17/27] wip didResolveSource

---
 docs/source/integrations/plugins.md           | 17 ++++++++++
 .../src/__tests__/runQuery.test.ts            | 29 ++++++++++++++++
 .../apollo-server-core/src/requestPipeline.ts | 11 ++++++
 .../src/index.ts                              | 34 ++++++++++++++++++-
 .../apollo-server-plugin-base/src/index.ts    |  5 +++
 packages/apollo-server-types/src/index.ts     |  4 ++-
 6 files changed, 98 insertions(+), 2 deletions(-)

diff --git a/docs/source/integrations/plugins.md b/docs/source/integrations/plugins.md
index df4003d8544..2c9c2600381 100644
--- a/docs/source/integrations/plugins.md
+++ b/docs/source/integrations/plugins.md
@@ -313,6 +313,23 @@ should not return a value.
 
 > If you're using TypeScript to create your plugin, implement the [ `GraphQLRequestListener` interface](https://github.com/apollographql/apollo-server/blob/master/packages/apollo-server-plugin-base/src/index.ts) from the `apollo-server-plugin-base` module to define functions for request lifecycle events.
 
+### `didResolveSource`
+
+The `didResolveSource` event is invoked after Apollo Server has determined the
+`String`-representation of the incoming operation that it will act upon.  In the
+event that this `String` was not directly passed in from the client, this
+may be retrieved from a cache store (e.g., Automated Persisted Queries).
+
+At this stage, there is not a guarantee that the operation is not malformed.
+
+```typescript
+didResolveSource?(
+  requestContext: WithRequired<
+    GraphQLRequestContext<TContext>, 'source' | 'logger'>,
+  >,
+): ValueOrPromise<void>;
+```
+
 ### `parsingDidStart`
 
 The `parsingDidStart` event fires whenever Apollo Server will parse a GraphQL
diff --git a/packages/apollo-server-core/src/__tests__/runQuery.test.ts b/packages/apollo-server-core/src/__tests__/runQuery.test.ts
index 4b0c257a3b4..54e102dae4e 100644
--- a/packages/apollo-server-core/src/__tests__/runQuery.test.ts
+++ b/packages/apollo-server-core/src/__tests__/runQuery.test.ts
@@ -501,6 +501,30 @@ describe('runQuery', () => {
       });
     });
 
+    describe('didResolveSource', () => {
+      const didResolveSource = jest.fn();
+      it('called with the source', async () => {
+        await runQuery({
+          schema,
+          queryString: '{ testString }',
+          plugins: [
+            {
+              requestDidStart() {
+                return {
+                  didResolveSource,
+                };
+              },
+            },
+          ],
+          request: new MockReq(),
+        });
+
+        expect(didResolveSource).toHaveBeenCalled();
+        expect(didResolveSource.mock.calls[0][0])
+          .toHaveProperty('source', '{ testString }');
+      });
+    });
+
     describe('parsingDidStart', () => {
       const parsingDidStart = jest.fn();
       it('called when parsing will result in an error', async () => {
@@ -888,6 +912,9 @@ describe('runQuery', () => {
         let stopAwaiting: Function;
         const toBeAwaited = new Promise(resolve => stopAwaiting = resolve);
 
+        const didResolveSource: GraphQLRequestListener['didResolveSource'] =
+          jest.fn(() => { callOrder.push('didResolveSource') });
+
         const didResolveField: GraphQLRequestListenerDidResolveField =
           jest.fn(() => callOrder.push("didResolveField"));
 
@@ -932,6 +959,7 @@ describe('runQuery', () => {
             {
               requestDidStart() {
                 return {
+                  didResolveSource,
                   executionDidStart,
                 };
               },
@@ -944,6 +972,7 @@ describe('runQuery', () => {
         expect(willResolveField).toHaveBeenCalledTimes(1);
         expect(didResolveField).toHaveBeenCalledTimes(1);
         expect(callOrder).toStrictEqual([
+          "didResolveSource",
           "executionDidStart",
           "willResolveField",
           "beforeAwaiting",
diff --git a/packages/apollo-server-core/src/requestPipeline.ts b/packages/apollo-server-core/src/requestPipeline.ts
index 8ad8536b69c..dd3b283cc43 100644
--- a/packages/apollo-server-core/src/requestPipeline.ts
+++ b/packages/apollo-server-core/src/requestPipeline.ts
@@ -49,6 +49,7 @@ import {
 import {
   ApolloServerPlugin,
   GraphQLRequestListener,
+  GraphQLRequestContextDidResolveSource,
   GraphQLRequestContextExecutionDidStart,
   GraphQLRequestContextResponseForOperation,
   GraphQLRequestContextDidResolveOperation,
@@ -214,6 +215,16 @@ export async function processGraphQLRequest<TContext>(
   requestContext.queryHash = queryHash;
   requestContext.source = query;
 
+  // Let the plugins know that we now have a STRING of what we hope will
+  // parse and validate into a document we can execute on.  Unless we have
+  // retrieved this from our APQ cache, there's no guarantee that it is
+  // syntactically correct, so this string should not be trusted as a valid
+  // document until after it's parsed and validated.
+  await dispatcher.invokeHookAsync(
+    'didResolveSource',
+    requestContext as GraphQLRequestContextDidResolveSource<TContext>,
+  );
+
   const requestDidEnd = extensionStack.requestDidStart({
     request: request.http!,
     queryString: request.query,
diff --git a/packages/apollo-server-integration-testsuite/src/index.ts b/packages/apollo-server-integration-testsuite/src/index.ts
index a2618f328b7..37aab04fe10 100644
--- a/packages/apollo-server-integration-testsuite/src/index.ts
+++ b/packages/apollo-server-integration-testsuite/src/index.ts
@@ -1242,6 +1242,11 @@ export default (createApp: CreateAppFunc, destroyApp?: DestroyAppFunc) => {
         Parameters<GraphQLRequestListener['didEncounterErrors']>
       >;
 
+      let didResolveSource: jest.Mock<
+        ReturnType<GraphQLRequestListener['didResolveSource']>,
+        Parameters<GraphQLRequestListener['didResolveSource']>
+      >;
+
       function createApqApp(apqOptions: PersistedQueryOptions = {}) {
         return createApp({
           graphqlOptions: {
@@ -1249,7 +1254,10 @@ export default (createApp: CreateAppFunc, destroyApp?: DestroyAppFunc) => {
             plugins: [
               {
                 requestDidStart() {
-                  return { didEncounterErrors };
+                  return {
+                    didResolveSource,
+                    didEncounterErrors,
+                  };
                 }
               }
             ],
@@ -1265,6 +1273,7 @@ export default (createApp: CreateAppFunc, destroyApp?: DestroyAppFunc) => {
 
       beforeEach(async () => {
         cache = createMockCache();
+        didResolveSource = jest.fn();
         didEncounterErrors = jest.fn();
       });
 
@@ -1285,6 +1294,8 @@ export default (createApp: CreateAppFunc, destroyApp?: DestroyAppFunc) => {
             ttl: 900,
           }),
         );
+        expect(didResolveSource.mock.calls[0][0])
+          .toHaveProperty('source', query);
       });
 
       it('when ttlSeconds is unset, ttl is not passed to apq cache',
@@ -1305,6 +1316,8 @@ export default (createApp: CreateAppFunc, destroyApp?: DestroyAppFunc) => {
               ttl: 900,
             }),
           );
+          expect(didResolveSource.mock.calls[0][0])
+            .toHaveProperty('source', query);
         }
       );
 
@@ -1407,6 +1420,8 @@ export default (createApp: CreateAppFunc, destroyApp?: DestroyAppFunc) => {
             })]),
           }),
         );
+
+        expect(didResolveSource).not.toHaveBeenCalled();
       });
 
       it('returns PersistedQueryNotFound on the first try', async () => {
@@ -1432,6 +1447,8 @@ export default (createApp: CreateAppFunc, destroyApp?: DestroyAppFunc) => {
             ]),
           }),
         );
+
+        expect(didResolveSource).not.toHaveBeenCalled();
       });
       it('returns result on the second try', async () => {
         app = await createApqApp();
@@ -1452,6 +1469,8 @@ export default (createApp: CreateAppFunc, destroyApp?: DestroyAppFunc) => {
           }),
         );
 
+        expect(didResolveSource).not.toHaveBeenCalled();
+
         const result = await request(app)
           .post('/graphql')
           .send({
@@ -1464,6 +1483,9 @@ export default (createApp: CreateAppFunc, destroyApp?: DestroyAppFunc) => {
         // asserted above.
         expect(didEncounterErrors).toHaveBeenCalledTimes(1);
 
+        expect(didResolveSource.mock.calls[0][0])
+          .toHaveProperty('source', query);
+
         expect(result.body.data).toEqual({ testString: 'it works' });
         expect(result.body.errors).toBeUndefined();
       });
@@ -1523,6 +1545,9 @@ export default (createApp: CreateAppFunc, destroyApp?: DestroyAppFunc) => {
           .send({
             extensions,
           });
+
+        expect(didResolveSource).not.toHaveBeenCalled();
+
         await request(app)
           .post('/graphql')
           .send({
@@ -1535,6 +1560,9 @@ export default (createApp: CreateAppFunc, destroyApp?: DestroyAppFunc) => {
             extensions,
           });
 
+        expect(didResolveSource.mock.calls[0][0])
+          .toHaveProperty('source', query);
+
         expect(result.body.data).toEqual({ testString: 'it works' });
         expect(result.body.errors).toBeUndefined();
       });
@@ -1556,6 +1584,7 @@ export default (createApp: CreateAppFunc, destroyApp?: DestroyAppFunc) => {
           });
         expect(response.status).toEqual(400);
         expect(response.error.text).toMatch(/does not match query/);
+        expect(didResolveSource).not.toHaveBeenCalled();
       });
 
       it('returns correct result using get request', async () => {
@@ -1573,6 +1602,9 @@ export default (createApp: CreateAppFunc, destroyApp?: DestroyAppFunc) => {
             extensions: JSON.stringify(extensions),
           });
         expect(result.body.data).toEqual({ testString: 'it works' });
+        expect(didResolveSource.mock.calls[0][0])
+          .toHaveProperty('source', query);
+
       });
     });
   });
diff --git a/packages/apollo-server-plugin-base/src/index.ts b/packages/apollo-server-plugin-base/src/index.ts
index a90786dfd51..aef514d38de 100644
--- a/packages/apollo-server-plugin-base/src/index.ts
+++ b/packages/apollo-server-plugin-base/src/index.ts
@@ -8,6 +8,7 @@ import {
   GraphQLResponse,
   ValueOrPromise,
   WithRequired,
+  GraphQLRequestContextDidResolveSource,
   GraphQLRequestContextParsingDidStart,
   GraphQLRequestContextValidationDidStart,
   GraphQLRequestContextDidResolveOperation,
@@ -35,6 +36,7 @@ export {
   GraphQLResponse,
   ValueOrPromise,
   WithRequired,
+  GraphQLRequestContextDidResolveSource,
   GraphQLRequestContextParsingDidStart,
   GraphQLRequestContextValidationDidStart,
   GraphQLRequestContextDidResolveOperation,
@@ -63,6 +65,9 @@ export type GraphQLRequestListenerDidResolveField =
 export interface GraphQLRequestListener<
   TContext extends BaseContext = DefaultContext
 > extends AnyFunctionMap {
+  didResolveSource?(
+    requestContext: GraphQLRequestContextDidResolveSource<TContext>,
+  ): ValueOrPromise<void>;
   parsingDidStart?(
     requestContext: GraphQLRequestContextParsingDidStart<TContext>,
   ): GraphQLRequestListenerParsingDidEnd | void;
diff --git a/packages/apollo-server-types/src/index.ts b/packages/apollo-server-types/src/index.ts
index e5707818f9e..f63a8629d8d 100644
--- a/packages/apollo-server-types/src/index.ts
+++ b/packages/apollo-server-types/src/index.ts
@@ -151,12 +151,14 @@ export type Logger = {
   error(message?: any): void;
 }
 
-export type GraphQLRequestContextParsingDidStart<TContext> =
+export type GraphQLRequestContextDidResolveSource<TContext> =
   WithRequired<GraphQLRequestContext<TContext>,
     | 'metrics'
     | 'source'
     | 'queryHash'
   >;
+export type GraphQLRequestContextParsingDidStart<TContext> =
+  GraphQLRequestContextDidResolveSource<TContext>;
 export type GraphQLRequestContextValidationDidStart<TContext> =
   GraphQLRequestContextParsingDidStart<TContext> &
   WithRequired<GraphQLRequestContext<TContext>,

From 39ff086db696e5148044b4b798e4d5d83aeb5060 Mon Sep 17 00:00:00 2001
From: Jesse Rosenberger <git@jro.cc>
Date: Thu, 7 May 2020 12:27:09 +0000
Subject: [PATCH 18/27] other stuff

---
 packages/apollo-server-core/src/__tests__/runQuery.test.ts | 6 ++++++
 packages/apollo-server-integration-testsuite/src/index.ts  | 2 +-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/packages/apollo-server-core/src/__tests__/runQuery.test.ts b/packages/apollo-server-core/src/__tests__/runQuery.test.ts
index 54e102dae4e..7e41b081932 100644
--- a/packages/apollo-server-core/src/__tests__/runQuery.test.ts
+++ b/packages/apollo-server-core/src/__tests__/runQuery.test.ts
@@ -912,6 +912,9 @@ describe('runQuery', () => {
         let stopAwaiting: Function;
         const toBeAwaited = new Promise(resolve => stopAwaiting = resolve);
 
+        const validationDidStart: GraphQLRequestListener['validationDidStart'] =
+          jest.fn(() => { callOrder.push('validationDidStart') });
+
         const didResolveSource: GraphQLRequestListener['didResolveSource'] =
           jest.fn(() => { callOrder.push('didResolveSource') });
 
@@ -960,6 +963,7 @@ describe('runQuery', () => {
               requestDidStart() {
                 return {
                   didResolveSource,
+                  validationDidStart,
                   executionDidStart,
                 };
               },
@@ -968,11 +972,13 @@ describe('runQuery', () => {
           request: new MockReq(),
         });
 
+        expect(validationDidStart).toHaveBeenCalledTimes(1);
         expect(executionDidStart).toHaveBeenCalledTimes(1);
         expect(willResolveField).toHaveBeenCalledTimes(1);
         expect(didResolveField).toHaveBeenCalledTimes(1);
         expect(callOrder).toStrictEqual([
           "didResolveSource",
+          "validationDidStart",
           "executionDidStart",
           "willResolveField",
           "beforeAwaiting",
diff --git a/packages/apollo-server-integration-testsuite/src/index.ts b/packages/apollo-server-integration-testsuite/src/index.ts
index 37aab04fe10..e39cb84d036 100644
--- a/packages/apollo-server-integration-testsuite/src/index.ts
+++ b/packages/apollo-server-integration-testsuite/src/index.ts
@@ -1289,7 +1289,7 @@ export default (createApp: CreateAppFunc, destroyApp?: DestroyAppFunc) => {
 
         expect(cache.set).toHaveBeenCalledWith(
           expect.stringMatching(/^apq:/),
-          '{testString}',
+          query,
           expect.objectContaining({
             ttl: 900,
           }),

From ba37e68112266354a2504ead6bbe94fd0dc18653 Mon Sep 17 00:00:00 2001
From: Jesse Rosenberger <git@jro.cc>
Date: Fri, 8 May 2020 11:48:46 +0000
Subject: [PATCH 19/27] changelog: #3998

I may re-visit this consideration.
---
 CHANGELOG.md | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index b1aee8a1900..1e12834e9c6 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -9,6 +9,12 @@ The version headers in this history reflect the versions of Apollo Server itself
 
 > The changes noted within this `vNEXT` section have not been released yet.  New PRs and commits which introduce changes should include an entry in this `vNEXT` section as part of their development.  When a release is being prepared, a new header will be (manually) created below and the appropriate changes within that release will be moved into the new section.
 
+- `apollo-engine-reporting`: The underlying integration of this plugin, which instruments and traces the graph's resolver performance and transmits these metrics to [Apollo Graph Manager](https://engine.apollographql.com/), has been changed from the (soon to be deprecated) `graphql-extensions` API to the new [request pipeline `plugins` API](https://www.apollographql.com/docs/apollo-server/integrations/plugins/). [PR #3998](https://github.com/apollographql/apollo-server/pull/3998)
+
+  _This change should be purely an implementation detail for a majority of users_.  There are, however, some special considerations which are worth noting:
+
+    - The federated tracing plugin's `ftv1` response on `extensions` (which is present on the response from an implementing service to the gateway) is now placed on the `extensions` _after_ the `formatResponse` hook.  Anyone leveraging the `extensions`.`ftv1` data from the `formatResponse` hook will find that it is no longer present at that phase.
+
 - _Nothing yet! Stay tuned!_
 
 ### v2.13.0

From e9edd9aef186c0185cd9ec9bc4896f6ac9c9524c Mon Sep 17 00:00:00 2001
From: Jesse Rosenberger <git@jro.cc>
Date: Fri, 8 May 2020 12:25:15 +0000
Subject: [PATCH 20/27] types(e-r): Improve the typings of `didEnd`.

---
 .../apollo-engine-reporting/src/plugin.ts     | 20 ++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/packages/apollo-engine-reporting/src/plugin.ts b/packages/apollo-engine-reporting/src/plugin.ts
index 2e963c247df..1b9715235ac 100644
--- a/packages/apollo-engine-reporting/src/plugin.ts
+++ b/packages/apollo-engine-reporting/src/plugin.ts
@@ -2,6 +2,8 @@ import {
   GraphQLRequestContext,
   Logger,
   InvalidGraphQLRequestError,
+  GraphQLRequestContextExecutionDidStart,
+  GraphQLRequestContextDidEncounterErrors,
 } from 'apollo-server-types';
 import { Headers } from 'apollo-server-env';
 import { GraphQLError } from 'graphql';
@@ -122,7 +124,11 @@ export const plugin = <TContext>(
       }
 
       let endDone: boolean = false;
-      function didEnd() {
+      function didEnd(
+        requestContext:
+          | GraphQLRequestContextExecutionDidStart<TContext>
+          | GraphQLRequestContextDidEncounterErrors<TContext>,
+      ) {
         if (endDone) return;
         endDone = true;
         treeBuilder.stopTiming();
@@ -169,11 +175,11 @@ export const plugin = <TContext>(
           ensurePreflight();
         },
 
-        executionDidStart() {
+        executionDidStart(requestContext) {
           ensurePreflight();
 
           return {
-            executionDidEnd: didEnd,
+            executionDidEnd: () => didEnd(requestContext),
             willResolveField(...args) {
               const [, , , info] = args;
               return treeBuilder.willResolveField(info);
@@ -184,16 +190,16 @@ export const plugin = <TContext>(
           };
         },
 
-        didEncounterErrors({ errors }) {
+        didEncounterErrors(requestContext) {
           // We don't report some special-cased errors to Graph Manager.
           // See the definition of this function for the reasons.
-          if (allUnreportableSpecialCasedErrors(errors)) {
+          if (allUnreportableSpecialCasedErrors(requestContext.errors)) {
             return;
           }
 
           ensurePreflight();
-          treeBuilder.didEncounterErrors(errors);
-          didEnd();
+          treeBuilder.didEncounterErrors(requestContext.errors);
+          didEnd(requestContext);
         },
       };
     }

From b981b590328a6a321956ca851fb34d19f973aa25 Mon Sep 17 00:00:00 2001
From: Jesse Rosenberger <git@jro.cc>
Date: Fri, 8 May 2020 12:26:25 +0000
Subject: [PATCH 21/27] chore(e-r): Eliminate `ensurePreflight` by using (new)
 `didResolveSource`.

Leverages new life-cycle `didResolveSource` which was introduced by
[[PR #4076]] and inspired by this [[comment]].

This is much nicer!

[PR #4076]: https://github.com/apollographql/apollo-server/pull/4076
[Comment]: https://github.com/apollographql/apollo-server/pull/3998/files#r414911049
---
 .../apollo-engine-reporting/src/plugin.ts     | 134 ++++++++----------
 1 file changed, 56 insertions(+), 78 deletions(-)

diff --git a/packages/apollo-engine-reporting/src/plugin.ts b/packages/apollo-engine-reporting/src/plugin.ts
index 1b9715235ac..5faa1390956 100644
--- a/packages/apollo-engine-reporting/src/plugin.ts
+++ b/packages/apollo-engine-reporting/src/plugin.ts
@@ -44,83 +44,46 @@ export const plugin = <TContext>(
 
 
   return {
-    requestDidStart(requestContext) {
-      // We still need the entire `requestContext` to pass through to the
-      // `generateClientInfo` method, but we'll destructure for brevity within.
-      const {
-        metrics,
-        logger: requestLogger,
-        schemaHash,
-        request: { http, variables },
-      } = requestContext;
-
-      const treeBuilder: EngineReportingTreeBuilder =
-        new EngineReportingTreeBuilder({
+    requestDidStart({
+      logger: requestLogger,
+      schemaHash,
+      metrics,
+      request: { http, variables },
+    }) {
+      const treeBuilder: EngineReportingTreeBuilder = new EngineReportingTreeBuilder(
+        {
           rewriteError: options.rewriteError,
           logger: requestLogger || logger,
-        });
+        },
+      );
 
       treeBuilder.startTiming();
 
       metrics.startHrTime = treeBuilder.startHrTime;
 
-      let preflightDone: boolean = false;
-      function ensurePreflight() {
-        if (preflightDone) return;
-        preflightDone = true;
-
-        if (http) {
-          treeBuilder.trace.http = new Trace.HTTP({
-            method:
-              Trace.HTTP.Method[http.method as keyof typeof Trace.HTTP.Method]
-                || Trace.HTTP.Method.UNKNOWN,
-            // Host and path are not used anywhere on the backend, so let's not bother
-            // trying to parse request.url to get them, which is a potential
-            // source of bugs because integrations have different behavior here.
-            // On Node's HTTP module, request.url only includes the path
-            // (see https://nodejs.org/api/http.html#http_message_url)
-            // The same is true on Lambda (where we pass event.path)
-            // But on environments like Cloudflare we do get a complete URL.
-            host: null,
-            path: null,
-          });
-
-          if (options.sendHeaders) {
-            makeHTTPRequestHeaders(
-              treeBuilder.trace.http,
-              http.headers,
-              options.sendHeaders,
-            );
-          }
-        }
-
-        if (metrics.persistedQueryHit) {
-          treeBuilder.trace.persistedQueryHit = true;
-        }
-        if (metrics.persistedQueryRegister) {
-          treeBuilder.trace.persistedQueryRegister = true;
-        }
+      if (http) {
+        treeBuilder.trace.http = new Trace.HTTP({
+          method:
+            Trace.HTTP.Method[http.method as keyof typeof Trace.HTTP.Method] ||
+            Trace.HTTP.Method.UNKNOWN,
+          // Host and path are not used anywhere on the backend, so let's not bother
+          // trying to parse request.url to get them, which is a potential
+          // source of bugs because integrations have different behavior here.
+          // On Node's HTTP module, request.url only includes the path
+          // (see https://nodejs.org/api/http.html#http_message_url)
+          // The same is true on Lambda (where we pass event.path)
+          // But on environments like Cloudflare we do get a complete URL.
+          host: null,
+          path: null,
+        });
 
-        if (variables) {
-          treeBuilder.trace.details = makeTraceDetails(
-            variables,
-            options.sendVariableValues,
-            requestContext.source,
+        if (options.sendHeaders) {
+          makeHTTPRequestHeaders(
+            treeBuilder.trace.http,
+            http.headers,
+            options.sendHeaders,
           );
         }
-
-        const clientInfo = generateClientInfo(requestContext);
-        if (clientInfo) {
-          // While clientAddress could be a part of the protobuf, we'll ignore it for
-          // now, since the backend does not group by it and Engine frontend will not
-          // support it in the short term
-          const { clientName, clientVersion, clientReferenceId } = clientInfo;
-          // the backend makes the choice of mapping clientName => clientReferenceId if
-          // no custom reference id is provided
-          treeBuilder.trace.clientVersion = clientVersion || '';
-          treeBuilder.trace.clientReferenceId = clientReferenceId || '';
-          treeBuilder.trace.clientName = clientName || '';
-        }
       }
 
       let endDone: boolean = false;
@@ -163,21 +126,37 @@ export const plugin = <TContext>(
       }
 
       return {
-        parsingDidStart() {
-          ensurePreflight();
-        },
+        didResolveSource(requestContext) {
+          if (metrics.persistedQueryHit) {
+            treeBuilder.trace.persistedQueryHit = true;
+          }
+          if (metrics.persistedQueryRegister) {
+            treeBuilder.trace.persistedQueryRegister = true;
+          }
 
-        validationDidStart() {
-          ensurePreflight();
-        },
+          if (variables) {
+            treeBuilder.trace.details = makeTraceDetails(
+              variables,
+              options.sendVariableValues,
+              requestContext.source,
+            );
+          }
 
-        didResolveOperation() {
-          ensurePreflight();
+          const clientInfo = generateClientInfo(requestContext);
+          if (clientInfo) {
+            // While clientAddress could be a part of the protobuf, we'll ignore
+            // it for now, since the backend does not group by it and Graph
+            // Manager will not support it in the short term
+            const { clientName, clientVersion, clientReferenceId } = clientInfo;
+            // the backend makes the choice of mapping clientName => clientReferenceId if
+            // no custom reference id is provided
+            treeBuilder.trace.clientVersion = clientVersion || '';
+            treeBuilder.trace.clientReferenceId = clientReferenceId || '';
+            treeBuilder.trace.clientName = clientName || '';
+          }
         },
 
         executionDidStart(requestContext) {
-          ensurePreflight();
-
           return {
             executionDidEnd: () => didEnd(requestContext),
             willResolveField(...args) {
@@ -197,7 +176,6 @@ export const plugin = <TContext>(
             return;
           }
 
-          ensurePreflight();
           treeBuilder.didEncounterErrors(requestContext.errors);
           didEnd(requestContext);
         },

From 6d4777da1f4dfaab3bf23523e9106e6c2abbab08 Mon Sep 17 00:00:00 2001
From: Jesse Rosenberger <git@jro.cc>
Date: Fri, 8 May 2020 12:34:47 +0000
Subject: [PATCH 22/27] changelog: Add note that I believe some new APQ errors
 are now traced.

@glasser, are you able to confirm this belief and that it should be okay?
---
 CHANGELOG.md | 1 +
 1 file changed, 1 insertion(+)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 1e12834e9c6..02bef1a8293 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -14,6 +14,7 @@ The version headers in this history reflect the versions of Apollo Server itself
   _This change should be purely an implementation detail for a majority of users_.  There are, however, some special considerations which are worth noting:
 
     - The federated tracing plugin's `ftv1` response on `extensions` (which is present on the response from an implementing service to the gateway) is now placed on the `extensions` _after_ the `formatResponse` hook.  Anyone leveraging the `extensions`.`ftv1` data from the `formatResponse` hook will find that it is no longer present at that phase.
+    - Automated persisted query (APQ) errors [`PERSISTED_QUERY_NOT_SUPPORTED`](https://github.com/apollographql/apollo-server/blob/b981b590328a6a321956ca851fb34d19f973aa25/packages/apollo-server-errors/src/index.ts#L214-L222) and [`Unsupported persisted query version`](https://github.com/apollographql/apollo-server/blob/b981b590328a6a321956ca851fb34d19f973aa25/packages/apollo-server-core/src/requestPipeline.ts#L165) may now be reported to Graph Manager, whereas previously they were excluded from transmission.
 
 - _Nothing yet! Stay tuned!_
 

From bacbb175f38ab8e896e30ba5b82c6b4964535b4b Mon Sep 17 00:00:00 2001
From: Jesse Rosenberger <git@jro.cc>
Date: Fri, 8 May 2020 12:45:32 +0000
Subject: [PATCH 23/27] Revert "changelog: Add note that I believe some new APQ
 errors are now traced."

This reverts commit 6d4777da1f4dfaab3bf23523e9106e6c2abbab08.
---
 CHANGELOG.md | 1 -
 1 file changed, 1 deletion(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 02bef1a8293..1e12834e9c6 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -14,7 +14,6 @@ The version headers in this history reflect the versions of Apollo Server itself
   _This change should be purely an implementation detail for a majority of users_.  There are, however, some special considerations which are worth noting:
 
     - The federated tracing plugin's `ftv1` response on `extensions` (which is present on the response from an implementing service to the gateway) is now placed on the `extensions` _after_ the `formatResponse` hook.  Anyone leveraging the `extensions`.`ftv1` data from the `formatResponse` hook will find that it is no longer present at that phase.
-    - Automated persisted query (APQ) errors [`PERSISTED_QUERY_NOT_SUPPORTED`](https://github.com/apollographql/apollo-server/blob/b981b590328a6a321956ca851fb34d19f973aa25/packages/apollo-server-errors/src/index.ts#L214-L222) and [`Unsupported persisted query version`](https://github.com/apollographql/apollo-server/blob/b981b590328a6a321956ca851fb34d19f973aa25/packages/apollo-server-core/src/requestPipeline.ts#L165) may now be reported to Graph Manager, whereas previously they were excluded from transmission.
 
 - _Nothing yet! Stay tuned!_
 

From 59b401330064551b20339417ae26fde37ffe3857 Mon Sep 17 00:00:00 2001
From: Jesse Rosenberger <git@jro.cc>
Date: Fri, 8 May 2020 15:13:57 +0000
Subject: [PATCH 24/27] fix(e-r): Do not keep traces unless we resolve the
 "source".

Previously, I introduced a work-around for this in 78a4cb77edc7c7f,
though that is no longer necessary with the introduction of
`didResolveSource` in https://github.com/apollographql/apollo-server/pull/4076

I'm thrilled to remove this!

Ref: https://github.com/apollographql/apollo-server/pull/3998#discussion_r414916905
---
 .../apollo-engine-reporting/src/plugin.ts     | 59 +++++--------------
 1 file changed, 14 insertions(+), 45 deletions(-)

diff --git a/packages/apollo-engine-reporting/src/plugin.ts b/packages/apollo-engine-reporting/src/plugin.ts
index 5faa1390956..36915c2466c 100644
--- a/packages/apollo-engine-reporting/src/plugin.ts
+++ b/packages/apollo-engine-reporting/src/plugin.ts
@@ -1,12 +1,10 @@
 import {
   GraphQLRequestContext,
   Logger,
-  InvalidGraphQLRequestError,
   GraphQLRequestContextExecutionDidStart,
   GraphQLRequestContextDidEncounterErrors,
 } from 'apollo-server-types';
 import { Headers } from 'apollo-server-env';
-import { GraphQLError } from 'graphql';
 import { Trace } from 'apollo-engine-reporting-protobuf';
 
 import {
@@ -18,10 +16,6 @@ import {
 } from './agent';
 import { EngineReportingTreeBuilder } from './treeBuilder';
 import { ApolloServerPlugin } from "apollo-server-plugin-base";
-import {
-  PersistedQueryNotFoundError,
-  PersistedQueryNotSupportedError,
-} from 'apollo-server-errors';
 
 const clientNameHeaderKey = 'apollographql-client-name';
 const clientReferenceIdHeaderKey = 'apollographql-client-reference-id';
@@ -125,8 +119,19 @@ export const plugin = <TContext>(
         });
       }
 
+      // While we start the tracing as soon as possible, we only actually report
+      // traces when we have resolved the source.  This is largely because of
+      // the APQ negotiation that takes place before that resolution happens.
+      // This is effectively bypassing the reporting of:
+      //   - PersistedQueryNotFoundError
+      //   - PersistedQueryNotSupportedError
+      //   - InvalidGraphQLRequestError
+      let didResolveSource: boolean = false;
+
       return {
         didResolveSource(requestContext) {
+          didResolveSource = true;
+
           if (metrics.persistedQueryHit) {
             treeBuilder.trace.persistedQueryHit = true;
           }
@@ -170,12 +175,9 @@ export const plugin = <TContext>(
         },
 
         didEncounterErrors(requestContext) {
-          // We don't report some special-cased errors to Graph Manager.
-          // See the definition of this function for the reasons.
-          if (allUnreportableSpecialCasedErrors(requestContext.errors)) {
-            return;
-          }
-
+          // Search above for a comment about "didResolveSource" to see which
+          // of the pre-source-resolution errors we are intentionally avoiding.
+          if (!didResolveSource) return;
           treeBuilder.didEncounterErrors(requestContext.errors);
           didEnd(requestContext);
         },
@@ -184,39 +186,6 @@ export const plugin = <TContext>(
   };
 };
 
-/**
- * Previously, prior to the new plugin API, the Apollo Engine Reporting
- * mechanism was implemented using `graphql-extensions`, the API for which
- * didn't invoke `requestDidStart` until _after_ APQ had been negotiated.
- *
- * The new plugin API starts its `requestDidStart` _before_ APQ validation and
- * various other assertions which weren't included in the `requestDidStart`
- * life-cycle, even if they perhaps should be in terms of error reporting.
- *
- * The new plugin API is able to properly capture such errors within its
- * `didEncounterErrors` lifecycle hook, however, for behavioral consistency
- * reasons, we will still special-case those errors and maintain the legacy
- * behavior to avoid a breaking change.  We can reconsider this in a future
- * version of Apollo Engine Reporting (AS3, perhaps!).
- *
- * @param errors A list of errors to scan for special-cased instances.
- */
-function allUnreportableSpecialCasedErrors(
-  errors: readonly GraphQLError[],
-): boolean {
-  return errors.every(err => {
-    if (
-      err instanceof PersistedQueryNotFoundError ||
-      err instanceof PersistedQueryNotSupportedError ||
-      err instanceof InvalidGraphQLRequestError
-    ) {
-      return true;
-    }
-
-    return false;
-  });
-}
-
 // Helpers for producing traces.
 
 function defaultGenerateClientInfo({ request }: GraphQLRequestContext) {

From b0948a826cc31bec79f0a54ea46fa2db884ac95b Mon Sep 17 00:00:00 2001
From: Jesse Rosenberger <git@jro.cc>
Date: Fri, 8 May 2020 20:37:15 +0300
Subject: [PATCH 25/27] fix: Preserve client-requested `operationName` on op.
 name resolution failure.

Addresses feedback in below referenced [[Comment]].

If operation resolution (parsing and validating the document followed by
selecting the correct operation) resulted in the population of the
`operationName`, we'll use that. (For anonymous operations,
`requestContext.operationName` is null, which we represent here as the empty
string.)

If the user explicitly specified an `operationName` in their request but
operation resolution failed (due to parse or validation errors or because
there is no operation with that name in the document), we still put _that_
user-supplied `operationName` in the trace. This allows the error to be
better understood in Graph Manager. (We are considering changing the
behavior of `operationName` in these three error cases; see [[#3465]] below for
details.)

[Comment]: https://github.com/apollographql/apollo-server/pull/3998#discussion_r422260422
[#3465]: https://github.com/apollographql/apollo-server/pull/3465
---
 .../apollo-engine-reporting/src/plugin.ts     | 25 +++++++++++++------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/packages/apollo-engine-reporting/src/plugin.ts b/packages/apollo-engine-reporting/src/plugin.ts
index 36915c2466c..59a483dcd0a 100644
--- a/packages/apollo-engine-reporting/src/plugin.ts
+++ b/packages/apollo-engine-reporting/src/plugin.ts
@@ -94,14 +94,23 @@ export const plugin = <TContext>(
         treeBuilder.trace.forbiddenOperation = !!metrics.forbiddenOperation;
         treeBuilder.trace.registeredOperation = !!metrics.registeredOperation;
 
-        // If the user did not explicitly specify an operation name (which we
-        // would have saved in `executionDidStart`), but the request pipeline made
-        // it far enough to figure out what the operation name must be and store
-        // it on requestContext.operationName, use that name.  (Note that this
-        // depends on the assumption that the RequestContext passed to
-        // requestDidStart, which does not yet have operationName, will be mutated
-        // to add operationName later.)
-        const operationName = requestContext.operationName || '';
+        // If operation resolution (parsing and validating the document followed
+        // by selecting the correct operation) resulted in the population of the
+        // `operationName`, we'll use that. (For anonymous operations,
+        // `requestContext.operationName` is null, which we represent here as
+        // the empty string.)
+        //
+        // If the user explicitly specified an `operationName` in their request
+        // but operation resolution failed (due to parse or validation errors or
+        // because there is no operation with that name in the document), we
+        // still put _that_ user-supplied `operationName` in the trace. This
+        // allows the error to be better understood in Graph Manager. (We are
+        // considering changing the behavior of `operationName` in these three
+        // error cases; https://github.com/apollographql/apollo-server/pull/3465
+        const operationName =
+          requestContext.operationName ||
+          requestContext.request.operationName ||
+          '';
 
         // If this was a federated operation and we're the gateway, add the query plan
         // to the trace.

From 62fc270641faa1d5a8b023768d1b2a7c7f2e4ece Mon Sep 17 00:00:00 2001
From: Jesse Rosenberger <git@jro.cc>
Date: Fri, 8 May 2020 18:28:51 +0000
Subject: [PATCH 26/27] Switch to new `willResolveField` object parameter,
 rather position.

Implements pattern gained by a926b7eedbb87abab2ec70fb03d7174398 in #3988.

Ref: https://github.com/apollographql/apollo-server/pull/3998
---
 packages/apollo-engine-reporting/src/federatedPlugin.ts | 3 +--
 packages/apollo-engine-reporting/src/plugin.ts          | 3 +--
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/packages/apollo-engine-reporting/src/federatedPlugin.ts b/packages/apollo-engine-reporting/src/federatedPlugin.ts
index b5660f7183f..ce2a5580668 100644
--- a/packages/apollo-engine-reporting/src/federatedPlugin.ts
+++ b/packages/apollo-engine-reporting/src/federatedPlugin.ts
@@ -29,8 +29,7 @@ const federatedPlugin = <TContext>(
 
       return {
         executionDidStart: () => ({
-          willResolveField(...args) {
-            const [ , , , info] = args;
+          willResolveField({ info }) {
             return treeBuilder.willResolveField(info);
           },
         }),
diff --git a/packages/apollo-engine-reporting/src/plugin.ts b/packages/apollo-engine-reporting/src/plugin.ts
index 59a483dcd0a..9c75aba803b 100644
--- a/packages/apollo-engine-reporting/src/plugin.ts
+++ b/packages/apollo-engine-reporting/src/plugin.ts
@@ -173,8 +173,7 @@ export const plugin = <TContext>(
         executionDidStart(requestContext) {
           return {
             executionDidEnd: () => didEnd(requestContext),
-            willResolveField(...args) {
-              const [, , , info] = args;
+            willResolveField({ info }) {
               return treeBuilder.willResolveField(info);
               // We could save the error into the trace during the end handler, but
               // it won't have all the information that graphql-js adds to it later,

From c73cd163dc78ebfb25e46398339782c3a6d52801 Mon Sep 17 00:00:00 2001
From: Jesse Rosenberger <git@jro.cc>
Date: Mon, 11 May 2020 08:11:00 +0000
Subject: [PATCH 27/27] nit: Add missing closing paren on comment

Ref: b0948a826cc31bec79f0a54ea46fa2db884ac95b
---
 packages/apollo-engine-reporting/src/plugin.ts | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/packages/apollo-engine-reporting/src/plugin.ts b/packages/apollo-engine-reporting/src/plugin.ts
index 9c75aba803b..84ead380e19 100644
--- a/packages/apollo-engine-reporting/src/plugin.ts
+++ b/packages/apollo-engine-reporting/src/plugin.ts
@@ -105,8 +105,8 @@ export const plugin = <TContext>(
         // because there is no operation with that name in the document), we
         // still put _that_ user-supplied `operationName` in the trace. This
         // allows the error to be better understood in Graph Manager. (We are
-        // considering changing the behavior of `operationName` in these three
-        // error cases; https://github.com/apollographql/apollo-server/pull/3465
+        // considering changing the behavior of `operationName` in these 3 error
+        // cases; https://github.com/apollographql/apollo-server/pull/3465)
         const operationName =
           requestContext.operationName ||
           requestContext.request.operationName ||