From 5f9f86bd0b4d86b801af8d78aeb4367c26519e85 Mon Sep 17 00:00:00 2001 From: Evans Hauser Date: Wed, 28 Nov 2018 16:44:14 -0800 Subject: [PATCH 01/10] 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 --- packages/apollo-engine-reporting/src/agent.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/packages/apollo-engine-reporting/src/agent.ts b/packages/apollo-engine-reporting/src/agent.ts index 1c82b83e820..3b7e3ac0042 100644 --- a/packages/apollo-engine-reporting/src/agent.ts +++ b/packages/apollo-engine-reporting/src/agent.ts @@ -35,6 +35,10 @@ Traces.encode = function(message, originalWriter) { writer.bytes(encodedTraces[i]); } } + // To ensure that we only add the encoded Traces once, we remove them. This + // prevents multiple reporting in the case when there are multiple instances + // of a-e-r in a single project. + delete (message as any).encodedTraces; return writer; }; From 2188b50427b19026e13f3bac7e30026140fc3d3c Mon Sep 17 00:00:00 2001 From: Evans Hauser Date: Wed, 28 Nov 2018 19:04:15 -0800 Subject: [PATCH 02/10] Add changelog --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 07e1d8da44d..ee82fdeb7ae 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,8 @@ ### vNEXT +- Apollo Engine Reporting: include encodedTraces only once to prevent duplicates with multiple instances of apollo-engine-reporting [PR #2040](https://github.com/apollographql/apollo-server/pull/2040) + ### v2.2.4 - Fix GraphQL Playground documentation scrolling bug in Safari by updating to latest (rebased) fork of `graphql-playground-html`. [PR #2037](https://github.com/apollographql/apollo-server/pull/2037) From 844563ac18b5e1a1db1fd8e0a78f3b7b3de29915 Mon Sep 17 00:00:00 2001 From: Evans Hauser Date: Fri, 30 Nov 2018 12:19:12 -0800 Subject: [PATCH 03/10] 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. --- .../.gitignore | 2 ++ .../package.json | 7 +++--- .../src/index.ts | 24 ++++++++++++++++++ .../tsconfig.json | 9 +++++++ packages/apollo-engine-reporting/src/agent.ts | 25 ------------------- 5 files changed, 39 insertions(+), 28 deletions(-) create mode 100644 packages/apollo-engine-reporting-protobuf/.gitignore create mode 100644 packages/apollo-engine-reporting-protobuf/src/index.ts create mode 100644 packages/apollo-engine-reporting-protobuf/tsconfig.json diff --git a/packages/apollo-engine-reporting-protobuf/.gitignore b/packages/apollo-engine-reporting-protobuf/.gitignore new file mode 100644 index 00000000000..27001d367fb --- /dev/null +++ b/packages/apollo-engine-reporting-protobuf/.gitignore @@ -0,0 +1,2 @@ +protobuf.d.ts +protobuf.js diff --git a/packages/apollo-engine-reporting-protobuf/package.json b/packages/apollo-engine-reporting-protobuf/package.json index adeb5f12443..bf6d82d11e9 100644 --- a/packages/apollo-engine-reporting-protobuf/package.json +++ b/packages/apollo-engine-reporting-protobuf/package.json @@ -5,9 +5,10 @@ "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" + "compile": "tsc && cp src/protobuf* dist", + "prepare": "npm run pbjs && npm run pbts && npm run compile", + "pbjs": "bash -c 'pbjs --target static-module --out src/protobuf.js --wrap commonjs --force-number <(grep -v \"package mdg.engine.proto\" reports.proto)'", + "pbts": "pbts -o src/protobuf.d.ts src/protobuf.js" }, "repository": { "type": "git", diff --git a/packages/apollo-engine-reporting-protobuf/src/index.ts b/packages/apollo-engine-reporting-protobuf/src/index.ts new file mode 100644 index 00000000000..461fb2ca8e5 --- /dev/null +++ b/packages/apollo-engine-reporting-protobuf/src/index.ts @@ -0,0 +1,24 @@ +import * as protobuf from './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 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 * from './protobuf'; diff --git a/packages/apollo-engine-reporting-protobuf/tsconfig.json b/packages/apollo-engine-reporting-protobuf/tsconfig.json new file mode 100644 index 00000000000..444506ca99c --- /dev/null +++ b/packages/apollo-engine-reporting-protobuf/tsconfig.json @@ -0,0 +1,9 @@ +{ + "extends": "../../tsconfig.base", + "compilerOptions": { + "rootDir": "./src", + "outDir": "./dist" + }, + "include": ["src/**/*"], + "exclude": ["src/protobuf.d.ts", "src/protobuf.js"] +} diff --git a/packages/apollo-engine-reporting/src/agent.ts b/packages/apollo-engine-reporting/src/agent.ts index 3b7e3ac0042..cbf1c21fa23 100644 --- a/packages/apollo-engine-reporting/src/agent.ts +++ b/packages/apollo-engine-reporting/src/agent.ts @@ -17,31 +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]); - } - } - // To ensure that we only add the encoded Traces once, we remove them. This - // prevents multiple reporting in the case when there are multiple instances - // of a-e-r in a single project. - delete (message as any).encodedTraces; - return writer; -}; - export interface ClientInfo { clientName?: string; clientVersion?: string; From a027334687646ef7963fbb08a91e3815d8c88fbd Mon Sep 17 00:00:00 2001 From: Evans Hauser Date: Sun, 2 Dec 2018 23:26:56 -0800 Subject: [PATCH 04/10] Reexport protobuf import after modification `export * from './protobuf'` exports the unmodified reference --- packages/apollo-engine-reporting-protobuf/src/index.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/apollo-engine-reporting-protobuf/src/index.ts b/packages/apollo-engine-reporting-protobuf/src/index.ts index 461fb2ca8e5..20202eacd50 100644 --- a/packages/apollo-engine-reporting-protobuf/src/index.ts +++ b/packages/apollo-engine-reporting-protobuf/src/index.ts @@ -21,4 +21,4 @@ protobuf.Traces.encode = function(message, originalWriter) { return writer; }; -export * from './protobuf'; +export = protobuf; From f75617403fa7d7ba3c05fcd440f965c1177b3da3 Mon Sep 17 00:00:00 2001 From: Evans Hauser Date: Sun, 2 Dec 2018 23:37:51 -0800 Subject: [PATCH 05/10] 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 --- packages/apollo-engine-reporting/src/agent.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/apollo-engine-reporting/src/agent.ts b/packages/apollo-engine-reporting/src/agent.ts index cbf1c21fa23..e09f5ef3c10 100644 --- a/packages/apollo-engine-reporting/src/agent.ts +++ b/packages/apollo-engine-reporting/src/agent.ts @@ -169,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, ); From 36c46bc2b3c5177e54b6528c7dcd9d4963528819 Mon Sep 17 00:00:00 2001 From: Evans Hauser Date: Mon, 3 Dec 2018 11:19:36 -0800 Subject: [PATCH 06/10] 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. --- packages/apollo-engine-reporting-protobuf/.gitignore | 2 -- packages/apollo-engine-reporting-protobuf/package.json | 7 +++---- packages/apollo-engine-reporting-protobuf/src/index.d.ts | 2 ++ .../src/{index.ts => index.js} | 6 +++--- .../{ => src}/reports.proto | 0 packages/apollo-engine-reporting-protobuf/tsconfig.json | 9 --------- 6 files changed, 8 insertions(+), 18 deletions(-) delete mode 100644 packages/apollo-engine-reporting-protobuf/.gitignore create mode 100644 packages/apollo-engine-reporting-protobuf/src/index.d.ts rename packages/apollo-engine-reporting-protobuf/src/{index.ts => index.js} (89%) rename packages/apollo-engine-reporting-protobuf/{ => src}/reports.proto (100%) delete mode 100644 packages/apollo-engine-reporting-protobuf/tsconfig.json diff --git a/packages/apollo-engine-reporting-protobuf/.gitignore b/packages/apollo-engine-reporting-protobuf/.gitignore deleted file mode 100644 index 27001d367fb..00000000000 --- a/packages/apollo-engine-reporting-protobuf/.gitignore +++ /dev/null @@ -1,2 +0,0 @@ -protobuf.d.ts -protobuf.js diff --git a/packages/apollo-engine-reporting-protobuf/package.json b/packages/apollo-engine-reporting-protobuf/package.json index bf6d82d11e9..1fba7f9f019 100644 --- a/packages/apollo-engine-reporting-protobuf/package.json +++ b/packages/apollo-engine-reporting-protobuf/package.json @@ -5,10 +5,9 @@ "main": "dist/index.js", "types": "dist/index.d.ts", "scripts": { - "compile": "tsc && cp src/protobuf* dist", - "prepare": "npm run pbjs && npm run pbts && npm run compile", - "pbjs": "bash -c 'pbjs --target static-module --out src/protobuf.js --wrap commonjs --force-number <(grep -v \"package mdg.engine.proto\" reports.proto)'", - "pbts": "pbts -o src/protobuf.d.ts src/protobuf.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.ts b/packages/apollo-engine-reporting-protobuf/src/index.js similarity index 89% rename from packages/apollo-engine-reporting-protobuf/src/index.ts rename to packages/apollo-engine-reporting-protobuf/src/index.js index 20202eacd50..31fd01481f4 100644 --- a/packages/apollo-engine-reporting-protobuf/src/index.ts +++ b/packages/apollo-engine-reporting-protobuf/src/index.js @@ -1,4 +1,4 @@ -import * as protobuf from './protobuf'; +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 @@ -11,7 +11,7 @@ import * as protobuf from './protobuf'; const originalTracesEncode = protobuf.Traces.encode; protobuf.Traces.encode = function(message, originalWriter) { const writer = originalTracesEncode(message, originalWriter); - const encodedTraces = (message as any).encodedTraces; + const encodedTraces = message.encodedTraces; if (encodedTraces != null && encodedTraces.length) { for (let i = 0; i < encodedTraces.length; ++i) { writer.uint32(/* id 1, wireType 2 =*/ 10); @@ -21,4 +21,4 @@ protobuf.Traces.encode = function(message, originalWriter) { return writer; }; -export = protobuf; +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-protobuf/tsconfig.json b/packages/apollo-engine-reporting-protobuf/tsconfig.json deleted file mode 100644 index 444506ca99c..00000000000 --- a/packages/apollo-engine-reporting-protobuf/tsconfig.json +++ /dev/null @@ -1,9 +0,0 @@ -{ - "extends": "../../tsconfig.base", - "compilerOptions": { - "rootDir": "./src", - "outDir": "./dist" - }, - "include": ["src/**/*"], - "exclude": ["src/protobuf.d.ts", "src/protobuf.js"] -} From b533e45fe7cdad9e4c83299b83d89b1dfefa7889 Mon Sep 17 00:00:00 2001 From: Evans Hauser Date: Mon, 3 Dec 2018 11:31:51 -0800 Subject: [PATCH 07/10] Add description of dev process in README --- packages/apollo-engine-reporting-protobuf/README.md | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/packages/apollo-engine-reporting-protobuf/README.md b/packages/apollo-engine-reporting-protobuf/README.md index d420ece8695..fe2f119c115 100644 --- a/packages/apollo-engine-reporting-protobuf/README.md +++ b/packages/apollo-engine-reporting-protobuf/README.md @@ -5,3 +5,14 @@ for the Engine reporting API. The Engine reporting API is currently subject to change at any time; do not rely on this to build your own client. + +## Development + +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` command. +Normally `prepare` is performed by the root package.json with a `postinstall` +hook. If the code in this package needs to change, you should run `npx lerna +run prepare` in the root of this monorepo to generate the code changes. Running +the command in the root enables the `protobuf` binaries, `pbjs` and `pbts`, to +resolve correctly. From 04ba11aeb6af24b765d9fbf97631d6056e08a527 Mon Sep 17 00:00:00 2001 From: Jesse Rosenberger Date: Tue, 4 Dec 2018 11:48:52 +0200 Subject: [PATCH 08/10] Update CHANGELOG.md --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b5380dd3510..8398431bbbb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,7 +2,7 @@ ### vNEXT -- Apollo Engine Reporting: include encodedTraces only once to prevent duplicates with multiple instances of apollo-engine-reporting [PR #2040](https://github.com/apollographql/apollo-server/pull/2040) +- `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 From 2f590d166b090d8b9edbec560abf7a1b0039c8f3 Mon Sep 17 00:00:00 2001 From: Jesse Rosenberger Date: Tue, 4 Dec 2018 11:59:19 +0200 Subject: [PATCH 09/10] Update README.md --- .../README.md | 30 +++++++++++-------- 1 file changed, 18 insertions(+), 12 deletions(-) diff --git a/packages/apollo-engine-reporting-protobuf/README.md b/packages/apollo-engine-reporting-protobuf/README.md index fe2f119c115..31c5ab3863c 100644 --- a/packages/apollo-engine-reporting-protobuf/README.md +++ b/packages/apollo-engine-reporting-protobuf/README.md @@ -1,18 +1,24 @@ -# 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 -Currently this package generates a majority of its code with +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` command. -Normally `prepare` is performed by the root package.json with a `postinstall` -hook. If the code in this package needs to change, you should run `npx lerna -run prepare` in the root of this monorepo to generate the code changes. Running -the command in the root enables the `protobuf` binaries, `pbjs` and `pbts`, to -resolve correctly. +`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. From 05a2e4b6fb2db208b62a9e95f8ef7273fa1aa801 Mon Sep 17 00:00:00 2001 From: Jesse Rosenberger Date: Tue, 4 Dec 2018 12:02:25 +0200 Subject: [PATCH 10/10] Update README.md --- packages/apollo-engine-reporting-protobuf/README.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/packages/apollo-engine-reporting-protobuf/README.md b/packages/apollo-engine-reporting-protobuf/README.md index 31c5ab3863c..c4038ba7c42 100644 --- a/packages/apollo-engine-reporting-protobuf/README.md +++ b/packages/apollo-engine-reporting-protobuf/README.md @@ -12,6 +12,11 @@ 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.