Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AER: include encodedTraces only once to prevent duplicates #2040

Merged
merged 14 commits into from
Dec 4, 2018
Merged
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +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-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
Expand Down
2 changes: 2 additions & 0 deletions packages/apollo-engine-reporting-protobuf/.gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
protobuf.d.ts
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aren't these files in src?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They're generated to be in src when apollo-engine-reporting-protobuf is npm prepared. I think it's better that the reports.proto file is the source of truth and we exclude all protobuf.d.ts. Are you suggesting that we should be excluding src/protobuf.d.ts rather than all protobuf.d.ts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think it would make sense to be more explicit about the specific file you're excluding.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious why reports.proto wouldn't move into this newly introduced src directory?

As a general principle, I find it strange that protobuf.d.ts (or anything) inside the src/ tree is Git ignored or generated. The previous implementation here didn't violate that expectation since everything was generated into dist/.

protobuf.js
7 changes: 4 additions & 3 deletions packages/apollo-engine-reporting-protobuf/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
24 changes: 24 additions & 0 deletions packages/apollo-engine-reporting-protobuf/src/index.ts
Original file line number Diff line number Diff line change
@@ -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 = protobuf;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've seen this syntax before, but you are more familiar with modern TS/JS than me.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the TypeScript way of expressing module.exports = protobuf.

9 changes: 9 additions & 0 deletions packages/apollo-engine-reporting-protobuf/tsconfig.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
{
"extends": "../../tsconfig.base",
"compilerOptions": {
"rootDir": "./src",
"outDir": "./dist"
},
"include": ["src/**/*"],
"exclude": ["src/protobuf.d.ts", "src/protobuf.js"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps this is wishful thinking on my part, but this exclude line is now a unique exception within this entire monorepo since with one other exception — which we're trying to get away from in order to align them all. I'd somewhat rather we not have exclude be a new type of exception.

}
25 changes: 2 additions & 23 deletions packages/apollo-engine-reporting/src/agent.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,27 +17,6 @@ import {
GraphQLServiceContext,
} from 'apollo-server-core/dist/requestPipelineAPI';

// Override the generated protobuf Traces.encode function so that it will look
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that there is a reference to this comment later where encodedTraces is written which should be updated to point to the other file.

// 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;
Expand Down Expand Up @@ -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,
);
Expand Down