[GM-833] Update Apollo Graph Manager API Key environment variable by michael-watson · Pull Request #3923 · apollographql/apollo-server

michael-watson · 2020-03-27T23:36:45Z

Since changing to Apollo Graph Manager, it is confusing to new users the reference to ENGINE_API_KEY. This PR is to change to the reference to APOLLO_KEY and add a deprecation warning for the old value

- Change ENGINE_API_KEY reference to APOLLO_KEY and add a deprecation message. - Export keys to be referenced through out packages

michael-watson · 2020-03-27T23:37:40Z

@abernix I wasn't sure how to add a test for checking the environment variables, so that might be something for us to add to this.

zionts

Not to bike-shed toooooo much, but it does strike me as a bit odd that whether this is "turned on" or not is triggered by whether the API key is in the environment, and we are just trusting the caller to not use the API key in the environment to put it into the constructor options so that they will see the deprecation message & error handling will happen. In this case, it's actually fine, but we handle options?.engine?.apikey || process.env.APOLLO_KEY || process.env.ENGINE_API_KEY several times in this repo because of it.

trevor-scheer

Going to echo myself from @zionts PR: a couple simple tests that validate the new logs and throw errors would be great. Happy to pair on that with you!

PS: this needs a changelog entry

trevor-scheer · 2020-03-30T19:39:43Z

  // the constructor is not longer shared between all integration
  constructor(options: Config) {
-    if (process.env.ENGINE_API_KEY || options.engine) {
+    if (process.env[keyEnvVar] || process.env[legacyKeyEnvVar] || options.engine) {


@zionts pointed out that we're doing this in a number of places. I have a suggestion. Let's export a function from apollo-server-env, something like getApiKeyFromEnv().

This fn can be responsible for:

Throwing when both exist

Notifying (once) that the user is depending on deprecated functionality

Caching values requested from process.env and returning them. Accessing these values isn't free, and we only need to do it once in a process's lifetime.

Returning a single apiKey, cleaning up much of the || logic we're seeing everywhere.

All right, so I followed a similar pattern to what I did here for engine.graphVariant, which is to throw it in an exported function from agent.ts in apollo-engine-reporting, which gateway relies upon. The challenge here is that it's important to have EngineReportingAgent be a class that can run on its own and perform its own validation, which means that we don't want to just calculate the "right" thing up above and pass it through. Additionally, the gateway and aer actually have different objects that they use in their construction, so there needs to be some calculation and conversion in order to satisfy the gateway.

All to say, I think having this be an exported function from aer so that aer can continue to remain a standalone package has benefits, and we can still export it and rely on it from other places, including in constructing the gateway object. That's what I've done here in response to this comment. Now we do call it twice in the case of a gateway, which would, theoretically, be a fairly simple fix, but I didn't prioritize doing so because of the added complexity that it causes and the unclear impact of the benefit (one deprecation message in your console instead of two and one more read of an environment variable at boot-up time)

These do not need to sendReportsImmediately because the reporting agent has support for handling shutdown signals to send off its final reports.

addressed

zionts · 2020-03-31T16:26:51Z

FYI @michael-watson I added some commits to your PR if you'd like to take a look :)

abernix

Thanks for moving this work forward! I've left a number of comments throughout. Some are likely duplicative of [my review](https://github.com/apollographql/apollo-server/pull/3924#pullrequestreview-385965077] on #3924 since I reviewed them several hours apart but they have overlapping concern. 😉

abernix · 2020-04-01T22:47:08Z

  requestContext: GraphQLRequestContext<TContext>,
 ) => ClientInfo;

+export function getEngineApiKey({engine, shouldWarnOnDeprecatedUsage = true}: { engine: EngineReportingOptions<any> | boolean | undefined, shouldWarnOnDeprecatedUsage?: boolean }) {


Any reasoning behind not using a module-level variable as a toggle to determine whether the deprecation warning was already shown, rather than passing it in as an option? Seems we'd only want this warning applied exactly once across... anything that uses it?

My hunch might be that it's because of the need to use the correct logger, but that's unstated.

~~I think using a module-level var works here actually — I just generally abhor mutability, though I suppose in this case it cleans things up in quite the happy way.~~ It does not work, actually, since some calls use the module function directly and some create a new instance of the class, which recreates module-level vars

Co-Authored-By: Jesse Rosenberger <git@jro.cc>

If both APOLLO_KEY and ENGINE_API_KEY are set, this will now warn and prefer APOLLO_KEY, in accordance with changes to apollo-tooling. Imp personally not very opinionated on this front, so happy to take direction from the reviewer.

stale

…i-key

Also some word-smithing to align these similar concepts. Follows-up: #3923 Follows-up: #3924

glasser · 2020-09-03T18:38:36Z

FYI, this PR (presumably accidentally) changed the semantics of setting engine: false but also setting one of the API key env vars: before this PR, that would disable usage reporting, but after this PR the engine: false is ignored.

(My work in #4453 removes engine: false and replaces it with more explicit things like ApolloServerPluginUsageReportingDisiabled() anyway.)

…apollo-server#3923) Co-authored-by: Adam Zionts <adam@meteor.com> Co-authored-by: Adam Zionts <adam@apollographql.com> Apollo-Orig-Commit-AS: apollographql/apollo-server@fa42e36

michael-watson added 2 commits March 27, 2020 15:49

Deprecate ENGINE_API_KEY

2f81a65

- Change ENGINE_API_KEY reference to APOLLO_KEY and add a deprecation message. - Export keys to be referenced through out packages

Fix keyEnvVar references

9728130

michael-watson requested review from abernix and zionts March 27, 2020 23:36

zionts changed the title ~~Update Apollo Graph Manage API Key reference~~ [GM-833] Update Apollo Graph Manager API Key reference Mar 28, 2020

zionts changed the title ~~[GM-833] Update Apollo Graph Manager API Key reference~~ [GM-833] Update Apollo Graph Manager API Key environment variable Mar 28, 2020

zionts reviewed Mar 28, 2020

View reviewed changes

Comment thread packages/apollo-engine-reporting/src/agent.ts Outdated

zionts reviewed Mar 28, 2020

View reviewed changes

This was referenced Mar 28, 2020

Support engine.graphVariant and APOLLO_GRAPH_VARIANT. #3924

Merged

[DO NOT MERGE] Support APOLLO_KEY in favor of ENGINE_API_KEY apollographql/apollo#822

Closed

trevor-scheer previously requested changes Mar 30, 2020

View reviewed changes

Adam Zionts added 3 commits March 31, 2020 01:47

Add tests

952edfb

Add CHANGELOG.md updates

521f595

Update serverless packages

135b9a2

These do not need to sendReportsImmediately because the reporting agent has support for handling shutdown signals to send off its final reports.

zionts requested a review from trevor-scheer March 31, 2020 16:26

Merge branch 'master' into aw/2020-3/update-api-key

7292c24

abernix previously requested changes Apr 2, 2020

View reviewed changes

Adam Zionts and others added 7 commits April 13, 2020 09:55

Move warn-once logic to module-level var

a7c9316

Error message clean-up

dad149d

Apply suggestions from code review

23fdb54

Co-Authored-By: Jesse Rosenberger <git@jro.cc>

Roll back changes to cloud function wrappers

83d7490

Update test

fd03542

Undo changes to apollo-server-lambda

493c4aa

Update both keys set to warn

a83a1f5

If both APOLLO_KEY and ENGINE_API_KEY are set, this will now warn and prefer APOLLO_KEY, in accordance with changes to apollo-tooling. Imp personally not very opinionated on this front, so happy to take direction from the reviewer.

zionts requested a review from abernix April 13, 2020 19:50

Merge branch 'master' into aw/2020-3/update-api-key

d3a27e3

Adam Zionts and others added 2 commits April 15, 2020 12:20

Use custom logger

cab5b0e

Merge branch 'master' into aw/2020-3/update-api-key

8326ef8

abernix self-assigned this Apr 20, 2020

zionts and others added 3 commits April 20, 2020 21:21

Merge branch 'master' into aw/2020-3/update-api-key

b91fe34

Merge branch 'master' into aw/2020-3/update-api-key

c631489

Merge remote-tracking branch 'origin/master' into aw/2020-3/update-ap…

ca85043

…i-key

abernix added this to the Release 2.13.0 milestone Apr 23, 2020

abernix merged commit fa42e36 into master Apr 23, 2020

abernix deleted the aw/2020-3/update-api-key branch April 23, 2020 15:03

abernix added a commit that referenced this pull request May 4, 2020

Change deprecation messaging to be consistently prefixed.

c6f4bc1

Also some word-smithing to align these similar concepts. Follows-up: #3923 Follows-up: #3924

github-actions Bot locked as resolved and limited conversation to collaborators Apr 21, 2023

Conversation

michael-watson commented Mar 27, 2020

Uh oh!

michael-watson commented Mar 27, 2020

Uh oh!

Uh oh!

zionts left a comment

Choose a reason for hiding this comment

Uh oh!

trevor-scheer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

trevor-scheer Mar 30, 2020

Choose a reason for hiding this comment

Uh oh!

zionts Mar 31, 2020

Choose a reason for hiding this comment

Uh oh!

zionts commented Mar 31, 2020

Uh oh!

abernix left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

abernix Apr 1, 2020

Choose a reason for hiding this comment

Uh oh!

zionts Apr 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

glasser commented Sep 3, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

zionts Apr 13, 2020 •

edited

Loading