AER: include `encodedTraces` only once to prevent duplicates #2040

evans · 2018-11-29T00:52:55Z

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.

Ran into this during Engine dogfooding 🌮:dog: 🎉

TODO:

Update CHANGELOG.md with your change (include reference to issue & this PR)
Make sure all of the significant new logic is covered by tests
Rebase your changes on master so that they can be merged easily
Make sure all tests and linter rules pass

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

glasser · 2018-11-29T20:04:10Z

Great catch!

Can you explain what you mean by multiple copies of apollo-engine-reporting? Like multiple versions loaded into the same app?

I don't think this fix is quite right. Trace.encode shouldn't mutate its receiver — you should be able to call it multiple times and get the same answer.

The fix is kind of like treating a symptom rather than a cause. The cause is that Trace.encode is getting wrapped multiple times. Let's prevent that directly instead of trying to work around the fact that it's happened. eg, we can set an "already wrapped" flag on Trace.encode and not re-wrap it if it's there. Or (depending on exactly why the issue occurs — as mentioned above I don't quite understand it) perhaps moving this hacky wrapper into apollo-engine-reporting-protobuf could help? (Or of course we could always move this functionality directly into protobufjs with a PR!)

evans · 2018-11-30T02:04:09Z

@glasser Great point! We should make this a solution to the problem rather than a treatment of symptoms.

Yes we have multiple copies loaded into the app, one at the root and another as a dependency of apollo-server-core:

This leads to a stack trace like, where the top is Trace.encode, then two different wrappers from two versions of apollo-engine-reporting, then FullTraces.encode:

I'll take a look at adding this to apollo-engine-reporting-protobuf, since adding a wrapped flag seems more brittle and a PR to protobufjs would mean a lot of upfront learning/shepherding cost

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.

glasser · 2018-11-30T21:42:14Z

This is perhaps the wrong repo for this question, but is having three copies of AER (including an ancient one in apollo?) itself an issue?

glasser · 2018-11-30T23:21:27Z

packages/apollo-engine-reporting-protobuf/.gitignore

@@ -0,0 +1,2 @@
+protobuf.d.ts


Aren't these files in src?

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?

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

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/.

glasser · 2018-11-30T23:22:54Z

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

+  return writer;
+};
+
+export * from './protobuf';


Is that really how it works? A brand new reference to ./protobuf' rather than the already imported protobuf?

😬 when I tested locally it succeeded. The code gets compiled down to something similar to the following screenshot. I like your solution, since it does not rely on this odd behavior of require

glasser · 2018-11-30T23:24:31Z

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


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

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

evans · 2018-12-03T07:35:44Z

@glasser I don't think that having multiple copies is a huge issue. I believe with npm we get a different copy of a package for each version that is present inside of the dependency tree. If we run into performance issues, then we can look at using yarn to deduplicate the mismatching versions. In general, I don't really know how much adding extra modules affects node/v8's performance.

Ideally, I'd like to remove the apollo code dependency by including all of the schema diff generation/rendering into the monorepo's codebase.

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

glasser · 2018-12-03T07:44:28Z

Re my concern about apollo, it just seems like a (separate) problem to have apollo requesting a specific old version of AER.

glasser · 2018-12-03T08:19:50Z

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

+  return writer;
+};
+
+export = protobuf;


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

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

abernix

This final approach is certainly superior to the monkey-patching we were doing before, so I'm very glad to see this getting in.

While I'll Approve this to move it along, I'm fairly curious what the temptation was to introduce TypeScript since the only TypeScript lingua being used is export = — i.e. module.exports =.

It seems that introduction may now be the reason that we now have a tsc compilation step, a generated protobuf in src (which is directly copied to dist) and other exceptions which could just have been a thin .js wrapper?

abernix · 2018-12-03T11:37:56Z

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

+  return writer;
+};
+
+export = protobuf;


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

abernix · 2018-12-03T11:38:31Z

packages/apollo-engine-reporting-protobuf/.gitignore

@@ -0,0 +1,2 @@
+protobuf.d.ts


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/.

abernix · 2018-12-03T11:54:44Z

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

+    "outDir": "./dist"
+  },
+  "include": ["src/**/*"],
+  "exclude": ["src/protobuf.d.ts", "src/protobuf.js"]


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.

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.

glasser · 2018-12-03T19:34:33Z

I'm gonna let Jesse take the reins for shepherding this one out — he's much more in tune with how we write code in this repo than me!

abernix

LGTM! Thanks for working on this!

abernix · 2018-12-13T13:53:50Z

This has been released in apollo-server-*@2.2.7.

evans requested review from abernix and zionts November 29, 2018 00:52

evans requested a review from martijnwalraven as a code owner November 29, 2018 00:52

evans changed the title ~~AER: Remove encodedTraces to prevent duplicates~~ AER: include encodedTraces only once to prevent duplicates Nov 29, 2018

Add changelog

2188b50

abernix requested a review from glasser November 29, 2018 09:19

Merge branch 'master' into evans/fix-duplicate-aer

a71f7ad

evans added 2 commits November 30, 2018 10:24

Merge branch 'master' into evans/fix-duplicate-aer

0f956b5

glasser reviewed Nov 30, 2018

View reviewed changes

Reexport protobuf import after modification

a027334

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

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

f756174

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

glasser approved these changes Dec 3, 2018

View reviewed changes

abernix approved these changes Dec 3, 2018

View reviewed changes

evans added 2 commits December 3, 2018 11:19

Add description of dev process in README

b533e45

Merge branch 'master' into evans/fix-duplicate-aer

f45c549

abernix approved these changes Dec 4, 2018

View reviewed changes

abernix added 4 commits December 4, 2018 11:48

Update CHANGELOG.md

04ba11a

Update README.md

2f590d1

Update README.md

05a2e4b

Merge branch 'master' into evans/fix-duplicate-aer

307c4f1

abernix merged commit 38bf09b into master Dec 4, 2018

abernix deleted the evans/fix-duplicate-aer branch December 4, 2018 10:06

dependencies bot mentioned this pull request Dec 13, 2018

Update apollo-server-express in / from 1.2.0 to 2.3.0 LibreTubeApp/LibreTube#277

Open

github-actions bot locked as resolved and limited conversation to collaborators Apr 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AER: include `encodedTraces` only once to prevent duplicates #2040

AER: include `encodedTraces` only once to prevent duplicates #2040

evans commented Nov 29, 2018 •

edited

Loading

glasser commented Nov 29, 2018

evans commented Nov 30, 2018 •

edited

Loading

glasser commented Nov 30, 2018

glasser Nov 30, 2018

evans Dec 3, 2018

glasser Dec 3, 2018

abernix Dec 3, 2018

glasser Nov 30, 2018

evans Dec 3, 2018

glasser Nov 30, 2018

evans commented Dec 3, 2018

glasser commented Dec 3, 2018

glasser Dec 3, 2018

abernix Dec 3, 2018

abernix left a comment

abernix Dec 3, 2018

abernix Dec 3, 2018

abernix Dec 3, 2018

glasser commented Dec 3, 2018

abernix left a comment

abernix commented Dec 13, 2018

AER: include encodedTraces only once to prevent duplicates #2040

AER: include encodedTraces only once to prevent duplicates #2040

Conversation

evans commented Nov 29, 2018 • edited Loading

glasser commented Nov 29, 2018

evans commented Nov 30, 2018 • edited Loading

glasser commented Nov 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evans commented Dec 3, 2018

glasser commented Dec 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abernix left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glasser commented Dec 3, 2018

abernix left a comment

Choose a reason for hiding this comment

abernix commented Dec 13, 2018

AER: include `encodedTraces` only once to prevent duplicates #2040

AER: include `encodedTraces` only once to prevent duplicates #2040

evans commented Nov 29, 2018 •

edited

Loading

evans commented Nov 30, 2018 •

edited

Loading