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.

Sign up for GitHub

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

Jump to bottom

Support engine.graphVariant and APOLLO_GRAPH_VARIANT. #3924

Merged
merged 20 commits into from
Apr 23, 2020
Merged

Support engine.graphVariant and APOLLO_GRAPH_VARIANT. #3924

merged 20 commits into from
Apr 23, 2020

Conversation

zionts
Copy link
Contributor

@zionts zionts commented Mar 28, 2020 •
edited
Loading

This change allows users to set engine.graphVariant in constructor options to Apollo Server as well as the APOLLO_GRAPH_VARIANT environment variable for configuration of both metrics reporting and managed configuration. Since its inception, managed federation has always supported only engine.graphVariant options as direct input, so no changes were made there.

For some time now, the name "graph variant" has been preferred over the (arguably legacy) "schema tag" — this change helps to modernize users' configuration options to create a more consistent user experience. The less-preferable options are still supported, but will output deprecation messages using console.warn when used. This PR will be paired with an update to any documentation to prefer setting the modern options added herein.

For some additional context, this change is part of a larger overhaul of all of our configuration to include more modern names in the Apollo SaaS ecosystem, in an effort to provide a consistent user experience. The hope is that the first thing that users guess is the right thing :)

I will follow this change (and @michael-watson 's #3923 ) with a docs update for both ENGINE_SCHEMA_TAG / schemaTag and ENGINE_API_KEY as a quick follow-up.
Additionally, this PR fixes an inaccurately documented ENGINE_GRAPH_VARIANT to
the (deprecated as of this PR) ENGINE_SCHEMA_TAG to correct the situation.

For associated docs update, please see apollographql/apollo#823

If interested in the larger overhaul, please take a look at:

@zionts zionts force-pushed the adam/20/3/support-graph-variant branch from 6e02abe to b4a20be Compare March 28, 2020 22:18
@trevor-scheer
Copy link
Member

A couple simple tests that validate the new thrown error and logged message would be much appreciated! This looks like a great place for them. Add a changelog entry and this LGTM!

Adam Zionts and others added 5 commits March 31, 2020 00:47
Move calculation of graphVariant into agent
e88976e 
The way it was before only would catch errors / print deprecation
messages if the user was using a gateway. Somewhat annoyingly, the
gateway and apollo server use different construction objects, so sharing
the method was cleaner (since we would, in theory, like it to be
possible to create an engine-reporting-agent as a class, it's important
that it handles its own errors itself). This does, however, mean that a
gateway Apollo Server that is using the deprecated key will get two
deprecation messages, but I think it would cause more clutter to resolve
that then the value it would provide to "fix" it.

This also adds tests.
Add mock of console.warn to test
8e4bf7a 
Additionally, since the logging is now performed by a function coming
from apollo-engine-reporting, it seemed out-of-scope to add a pluggable
logger to its feature set. If that's a blocker, please tell me!
Add changelog entry
e34789b
Add CHANGELOG entries
1eee85a 
I added these to both apollo gateway and the top-level
@zionts
Merge branch 'master' into adam/20/3/support-graph-variant
2c18ded
@abernix abernix added this to the Release 2.13.0 milestone Mar 31, 2020
@abernix abernix self-requested a review April 2, 2020 13:23
abernix
abernix previously requested changes Apr 2, 2020
View reviewed changes
Copy link
Member

@abernix abernix left a comment

Choose a reason for hiding this comment

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

Thanks for working on this! I've left a number of comments throughout, but I think at a high level, I'm curious if we should put this logic entirely in apollo-grahpql: A package that the CLI, AER, AS Core, Gateway and Federation all rely on already and that lives in the tooling repository?

(This is slated for 2.13.0 of apollo-server-* and 0.15 of @apollo/federation and @apollo/gateway.)

CHANGELOG.md Outdated Show resolved Hide resolved
docs/source/federation/metrics.md Outdated Show resolved Hide resolved
packages/apollo-engine-reporting/src/agent.ts Outdated Show resolved Hide resolved
packages/apollo-engine-reporting/src/agent.ts Outdated Show resolved Hide resolved
packages/apollo-gateway/CHANGELOG.md Outdated Show resolved Hide resolved
packages/apollo-server-core/src/__tests__/ApolloServerBase.test.ts Outdated
Comment on lines 33 to 46
it('succeeds when passed a graphVariant in construction', () => {
let serverBase;
expect(
() =>
serverBase = new ApolloServerBase({
schema: buildServiceDefinition([{ typeDefs, resolvers }]).schema,
engine: {
graphVariant: 'foo',
apiKey: 'not:real:key',
},
}),
).not.toThrow();
(serverBase as unknown as ApolloServerBase).stop();
});
Copy link
Member

Choose a reason for hiding this comment

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

I really feel this test belongs in apollo-engine-reporting since engine is implemented by that package. (I think we should continue to believe that AER is a separate package until it is not.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm down for adding another test in apollo-engine-reporting, but my main goal was to test what I care about: that Apollo Server builds.

packages/apollo-server-core/src/__tests__/ApolloServerBase.test.ts Outdated Show resolved Hide resolved
packages/apollo-server-core/src/__tests__/ApolloServerBase.test.ts Outdated Show resolved Hide resolved
packages/apollo-server-core/src/__tests__/ApolloServerBase.test.ts
apiKey: 'not:real:key',
},
}),
).not.toThrow();
Copy link
Member

Choose a reason for hiding this comment

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

I think we could be more specific here and test for what we don't want thrown, in terms of specific error message. Otherwise this test ends up failing when some other thing which intentionally fails this pattern crops up, red herring during development, etc. We can avoid that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On the flip side, it means that if someone changes the error message, then the test which is looking for it will continue to pass silently, but will be doing nothing, right?

Copy link
Member

Choose a reason for hiding this comment

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

@zionts A defensive best-of-both-worlds technique would be test for each and have the tests rely on the same error message string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But like... what error message are we looking for? I can think of a couple different error messages that are reasonable to test for.

  • defined both graphVariant and schemaTag
  • unknown key / TypeError

Will we actually keep this up to date as we add more? Will it actually be a red herring as opposed to a good test update, when we add, e.g. "throw on malformed API key" that causes this test to error?

I would really prefer to leave this as just making sure this configuration doesn't throw anything and updating the test + adding another if another behavior through a different feature causes this test to throw.

packages/apollo-server-core/src/__tests__/ApolloServerBase.test.ts Outdated Show resolved Hide resolved
zionts and others added 2 commits April 2, 2020 11:18
@zionts @abernix
Update CHANGELOG.md
0a3f799 
Co-Authored-By: Jesse Rosenberger <[email protected]>
@zionts @abernix
Apply Jesse's suggestion from code review
7d9bd3e 
Co-Authored-By: Jesse Rosenberger <[email protected]>
abernix
abernix reviewed Apr 2, 2020
View reviewed changes
packages/apollo-server-core/src/__tests__/ApolloServerBase.test.ts
apiKey: 'not:real:key',
},
}),
).not.toThrow();
Copy link
Member

Choose a reason for hiding this comment

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

@zionts A defensive best-of-both-worlds technique would be test for each and have the tests rely on the same error message string.

packages/apollo-server-core/src/__tests__/ApolloServerBase.test.ts Outdated Show resolved Hide resolved
packages/apollo-server-core/src/__tests__/ApolloServerBase.test.ts Outdated Show resolved Hide resolved
@zionts
Copy link
Contributor Author

zionts commented Apr 2, 2020

I'm curious if we should put this logic entirely in apollo-grahpql

@abernix, I'm not incredibly opinionated here, but I think that the "logic" component is rather small, and I'd prefer to have it directly in the package(s) that use it rather than imported from another library and worrying about versioning across those packages, etc. I also should admit that there's a big part of me that just doesn't think there's a lot of value gained and a fair amount of extra work and sequencing involved (read: I'm a little lazy)

@zionts zionts requested a review from abernix April 13, 2020 16:49
@abernix abernix self-assigned this Apr 20, 2020
@abernix abernix changed the title Support engine.graphVariant and APOLLO_GRAPH_VARIANT in configuration Support engine.graphVariant and APOLLO_GRAPH_VARIANT. Apr 23, 2020
@abernix abernix merged commit 275945d into master Apr 23, 2020
@abernix abernix deleted the adam/20/3/support-graph-variant branch April 23, 2020 13:53
abernix added a commit that referenced this pull request May 4, 2020
@abernix
Change deprecation messaging to be consistently prefixed.
c6f4bc1 
Also some word-smithing to align these similar concepts.

Follows-up: #3923
Follows-up: #3924
abernix pushed a commit to apollographql/federation that referenced this pull request Sep 4, 2020
@zionts
Support engine.graphVariant and APOLLO_GRAPH_VARIANT. (apollographql/…
0f614e1 
…apollo-server#3924)

Co-authored-by: Adam Zionts <[email protected]>
Apollo-Orig-Commit-AS: apollographql/apollo-server@275945d
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@trevor-scheer trevor-scheer trevor-scheer approved these changes

@abernix abernix Awaiting requested review from abernix

Assignees

@abernix abernix

Labels
None yet
Projects
None yet
Milestone
Release 2.13.0
Development

Successfully merging this pull request may close these issues.
3 participants
@zionts @trevor-scheer @abernix