From 38bf09b31699a41e31cb0475e2681639ae797604 Mon Sep 17 00:00:00 2001 From: Evans Hauser Date: Tue, 4 Dec 2018 02:06:08 -0800 Subject: [PATCH] (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 | 1 + .../README.md | 32 ++++++++++++++++--- .../package.json | 6 ++-- .../src/index.d.ts | 2 ++ .../src/index.js | 24 ++++++++++++++ .../{ => src}/reports.proto | 0 packages/apollo-engine-reporting/src/agent.ts | 25 ++------------- 7 files changed, 59 insertions(+), 31 deletions(-) create mode 100644 packages/apollo-engine-reporting-protobuf/src/index.d.ts create mode 100644 packages/apollo-engine-reporting-protobuf/src/index.js rename packages/apollo-engine-reporting-protobuf/{ => src}/reports.proto (100%) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7d3defa0999..8398431bbbb 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/packages/apollo-engine-reporting-protobuf/README.md b/packages/apollo-engine-reporting-protobuf/README.md index d420ece8695..c4038ba7c42 100644 --- a/packages/apollo-engine-reporting-protobuf/README.md +++ b/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. diff --git a/packages/apollo-engine-reporting-protobuf/package.json b/packages/apollo-engine-reporting-protobuf/package.json index adeb5f12443..1fba7f9f019 100644 --- a/packages/apollo-engine-reporting-protobuf/package.json +++ b/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", diff --git a/packages/apollo-engine-reporting-protobuf/src/index.d.ts b/packages/apollo-engine-reporting-protobuf/src/index.d.ts new file mode 100644 index 00000000000..627ce2e12e5 --- /dev/null +++ b/packages/apollo-engine-reporting-protobuf/src/index.d.ts @@ -0,0 +1,2 @@ +import * as protobuf from './protobuf'; +export = protobuf; diff --git a/packages/apollo-engine-reporting-protobuf/src/index.js b/packages/apollo-engine-reporting-protobuf/src/index.js new file mode 100644 index 00000000000..31fd01481f4 --- /dev/null +++ b/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; diff --git a/packages/apollo-engine-reporting-protobuf/reports.proto b/packages/apollo-engine-reporting-protobuf/src/reports.proto similarity index 100% rename from packages/apollo-engine-reporting-protobuf/reports.proto rename to packages/apollo-engine-reporting-protobuf/src/reports.proto diff --git a/packages/apollo-engine-reporting/src/agent.ts b/packages/apollo-engine-reporting/src/agent.ts index 1c82b83e820..e09f5ef3c10 100644 --- a/packages/apollo-engine-reporting/src/agent.ts +++ b/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 { 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, );