(engine-reporting) Include encodedTraces only once. (#2040)

* AER: Remove encodedTraces to prevent duplicates

When there are multiple instances of apollo-engine-reporting, the
`Trace.encode` method gets wrapped each time to add the
`encodedTraces`. In order to prevent them from being added to the
protobuf multiple times, we remove the encodedTraces after adding them
once

* Add changelog

* Move incremental Trace encoding to a-e-r-protobuf

To enable incrmental compilation of traces, we add a patch to the
Trace.encode method generated by protobujs to accept and store encoded
traces. Occassionally with multiple instances of apollo-engine-reporting
that share the same version of the protobuf, the wrapper method gets
applied more than once. In order to ensure that the wrapper only gets
applied once, we patch the Trace.encode method inside of
apollo-engine-protobuf.

tsc hangs on the processing the generated protobuf.js files, so the
tsconfig.json ignores the generated protobuf file. In order for the
typescript index.ts file to compile the generated protobuf.js file is
output to the src folder. To ensure the protobuf files are available to
the production build, `npm run compile` copies the protobuf file
manually from src to dist.

* Reexport protobuf import after modification

`export * from './protobuf'` exports the unmodified reference

* Update comment on Trace.encode to point at a-e-r-p

The override now occurs inside of apollo-engine-reporting-protobuf due
to the case of having multiple reporting challenges, so we need to
update the comments to help indicate that

* Remove typescript build step

In order to simplify the generation of this library, we move the change
the index.ts file into index.js and remove the typescript build step.
Since the type safety is created by the protobufjs generation, this
seems acceptable. In general this portion of the code should remain
relatively stable, so generating and copying the code with `prepare`
remains reasonable.

CHANGELOG.md

@@ -2,6 +2,7 @@
 
 ### vNEXT
 
+- `apollo-engine-reporting`: When multiple instances of `apollo-engine-reporting` are loaded (an uncommon edge case), ensure that `encodedTraces` are handled only once rather than once per loaded instance. [PR #2040](https://github.com/apollographql/apollo-server/pull/2040)
 - `apollo-server-micro`: Set the `Content-type` to `text/html` for GraphQL Playground. [PR #2026](https://github.com/apollographql/apollo-server/pull/2026)
 
 ### v2.2.5

packages/apollo-engine-reporting-protobuf/README.md

@@ -1,7 +1,29 @@
-# apollo-engine-reporting-protobuf
+# `apollo-engine-reporting-protobuf`
 
-This contains generated Javascript/TypeScript code for the protobuf definitions
-for the Engine reporting API.
+> **Note:** The Apollo Engine reporting API is subject to change.  We strongly
+> encourage developers to contact Apollo Engine support to discuss their use
+> case prior to building their own reporting agent using this module.
 
-The Engine reporting API is currently subject to change at any time; do not rely
-on this to build your own client.
+This module provides JavaScript/TypeScript
+[Protocol buffer](https://developers.google.com/protocol-buffers/) definitions
+for the Apollo Engine reporting API.  These definitions are generated for
+consumption from the `reports.proto` file which is defined internally within
+Apollo.
+
+## Development
+
+> **Note:** Due to a dependency on Unix tools (e.g. `bash`, `grep`, etc.), the
+> development of this module requires a Unix system.  There is no reason why
+> this can't be avoided, the time just hasn't been taken to make those changes.
+> We'd happily accept a PR which makes the appropriate changes!
+
+Currently, this package generates a majority of its code with
+[`protobufjs`](https://www.npmjs.com/package/protobufjs) based on the
+`reports.proto` file. The output is generated with the `prepare` npm script.
+
+The root of the repository provides the `devDependencies` necessary to build
+these definitions (e.g. `pbjs`, `pbts`, `protobuf`, etc.) and the the `prepare`
+npm script is invoked programmatically via the monorepo tooling (e.g. Lerna)
+thanks to _this_ module's `postinstall` script.   Therefore, when making
+changes to this module, `npx lerna run prepare` should be run from the **root**
+of this monorepo in order to update the definitions in _this_ module.

packages/apollo-engine-reporting-protobuf/package.json

@@ -5,9 +5,9 @@
   "main": "dist/index.js",
   "types": "dist/index.d.ts",
   "scripts": {
-    "prepare": "npm run pbjs && npm run pbts",
-    "pbjs": "bash -c 'mkdir -p dist && pbjs --target static-module --out dist/index.js --wrap commonjs --force-number <(grep -v \"package mdg.engine.proto\" reports.proto)'",
-    "pbts": "pbts -o dist/index.d.ts dist/index.js"
+    "prepare": "npm run pbjs && npm run pbts && cp src/* dist",
+    "pbjs": "bash -c 'mkdir -p dist && pbjs --target static-module --out dist/protobuf.js --wrap commonjs --force-number <(grep -v \"package mdg.engine.proto\" src/reports.proto)'",
+    "pbts": "pbts -o dist/protobuf.d.ts dist/protobuf.js"
   },
   "repository": {
     "type": "git",

packages/apollo-engine-reporting-protobuf/src/index.d.ts

@@ -0,0 +1,2 @@
+import * as protobuf from './protobuf';
+export = protobuf;

packages/apollo-engine-reporting-protobuf/src/index.js

@@ -0,0 +1,24 @@
+const protobuf = require('./protobuf');
+
+// Override the generated protobuf Traces.encode function so that it will look
+// for Traces that are already encoded to Buffer as well as unencoded
+// Traces. This amortizes the protobuf encoding time over each generated Trace
+// instead of bunching it all up at once at sendReport time. In load tests, this
+// change improved p99 end-to-end HTTP response times by a factor of 11 without
+// a casually noticeable effect on p50 times. This also makes it easier for us
+// to implement maxUncompressedReportSize as we know the encoded size of traces
+// as we go.
+const originalTracesEncode = protobuf.Traces.encode;
+protobuf.Traces.encode = function(message, originalWriter) {
+  const writer = originalTracesEncode(message, originalWriter);
+  const encodedTraces = message.encodedTraces;
+  if (encodedTraces != null && encodedTraces.length) {
+    for (let i = 0; i < encodedTraces.length; ++i) {
+      writer.uint32(/* id 1, wireType 2 =*/ 10);
+      writer.bytes(encodedTraces[i]);
+    }
+  }
+  return writer;
+};
+
+module.exports = protobuf;

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

@@ -17,27 +17,6 @@ import {
   GraphQLServiceContext,
 } from 'apollo-server-core/dist/requestPipelineAPI';
 
-// Override the generated protobuf Traces.encode function so that it will look
-// for Traces that are already encoded to Buffer as well as unencoded
-// Traces. This amortizes the protobuf encoding time over each generated Trace
-// instead of bunching it all up at once at sendReport time. In load tests, this
-// change improved p99 end-to-end HTTP response times by a factor of 11 without
-// a casually noticeable effect on p50 times. This also makes it easier for us
-// to implement maxUncompressedReportSize as we know the encoded size of traces
-// as we go.
-const originalTracesEncode = Traces.encode;
-Traces.encode = function(message, originalWriter) {
-  const writer = originalTracesEncode(message, originalWriter);
-  const encodedTraces = (message as any).encodedTraces;
-  if (encodedTraces != null && encodedTraces.length) {
-    for (let i = 0; i < encodedTraces.length; ++i) {
-      writer.uint32(/* id 1, wireType 2 =*/ 10);
-      writer.bytes(encodedTraces[i]);
-    }
-  }
-  return writer;
-};
-
 export interface ClientInfo {
   clientName?: string;
   clientVersion?: string;
@@ -190,8 +169,8 @@ export class EngineReportingAgent<TContext = any> {
       this.report.tracesPerQuery[statsReportKey] = new Traces();
       (this.report.tracesPerQuery[statsReportKey] as any).encodedTraces = [];
     }
-    // See comment on our override of Traces.encode to learn more about this
-    // strategy.
+    // See comment on our override of Traces.encode inside of
+    // apollo-engine-reporting-protobuf to learn more about this strategy.
     (this.report.tracesPerQuery[statsReportKey] as any).encodedTraces.push(
       encodedTrace,
     );