Skip to content

[AS-252] gateway: normalize root operation types in reporting #4100

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

Conversation

glasser
Copy link
Member

@glasser glasser commented May 11, 2020

The federation spec allows your backends to use any name for their root
types (not just the standard Query/Mutation/Subscription) but the composed
schema will always use the standard names.

Both Apollo-maintained implementations of federated trace
reporting (apollo-engine-reporting and federation-jvm) include the backend's
potentially nonstandard name in the federated traces sent to the gateway. Then
the gateway continues to use those names in the traces it sends to AGM.

For example, one of AGM's own implementing services uses QueryRoot, so traces
produced by the gateway in front of it have QueryRoot on the traces despite
that type not actually being part of the composed schema.

The effect is that field stats (on the schema explorer) for all root fields are
empty!

Because normalizing root operation names is part of the federation spec, it
makes sense to keep the implementing services simple and fix this just once in
the gateway.

@glasser glasser requested a review from trevor-scheer May 11, 2020 04:56
@glasser glasser force-pushed the glasser/federated-tracing-root-type-names branch from 002372b to db167ad Compare May 11, 2020 04:56
@glasser glasser requested a review from jsegaran May 11, 2020 04:56
@glasser
Copy link
Member Author

glasser commented May 11, 2020

I'd appreciate a review now but I definitely will want to add tests, CHANGELOG, etc before merging!

Requesting review from @jsegaran for his understanding of reporting and @trevor-scheer for his understanding of federation.

@glasser
Copy link
Member Author

glasser commented May 11, 2020

@trevor-scheer the second commit makes this feature be tested, but makes some other tests fail. Any chance you can look into this (with me perhaps)? I would think the other tests should pass.

@glasser glasser force-pushed the glasser/federated-tracing-root-type-names branch from 47a7836 to d2b663a Compare May 11, 2020 06:11
@glasser
Copy link
Member Author

glasser commented May 11, 2020

Oh it's because the tests call composeServices instead of its wrapper composeAndValidate, and only composeAndValidate calls normalizeTypedefs. @trevor-scheer what do you think? I changed it to have the tests call normalizeTypedefs (which I think requires exporting normalizeTypedefs from @apollo/federation purely for use in gateway tests), but maybe these tests (executeQueryPlan.test.ts and buildQueryPlan.test.ts) could just call composeAndValidate instead?

Copy link
Contributor

@trevor-scheer trevor-scheer left a comment

Choose a reason for hiding this comment

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

This LGTM, pending a changelog entry. Note, the gateway has its own changelog separate from the top level AS repo changelog.

As far as having to add normalizeTypeDefs within tests go, I think it will make sense to stop exposing composeServices to the public API and use composeAndValidate everywhere we can in tests. This in turn will remove the need to use normalizeTypeDefs in these tests. For now I don't think this PR needs to be concerned with that change.

In the same vein, I'm happy to see us testing the normalization code path within one of our common test fixtures!

@glasser glasser force-pushed the glasser/federated-tracing-root-type-names branch from d2b663a to 8b0eb04 Compare May 11, 2020 17:54
@glasser
Copy link
Member Author

glasser commented May 11, 2020

Thanks @trevor-scheer — how does this changelog look?

@trevor-scheer
Copy link
Contributor

@glasser perfect, thank you! Per your request I'll hold off on merging this until @jsegaran gets a chance to review as well.

@glasser
Copy link
Member Author

glasser commented May 11, 2020

Sure, though if you'd like to cut a pre-release off this branch I could test it in our staging infrastructure even pending @jsegaran 's review.

Copy link
Contributor

@jsegaran jsegaran left a comment

Choose a reason for hiding this comment

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

This looks good to me.

@trevor-scheer trevor-scheer changed the base branch from master to release-gateway-0.15.1@next May 11, 2020 19:04
The federation spec allows your backends to use any name for their root
types (not just the standard Query/Mutation/Subscription) but the composed
schema will always use the standard names.

Both Apollo-maintained implementations of federated trace
reporting (apollo-engine-reporting and federation-jvm) include the backend's
potentially nonstandard name in the federated traces sent to the gateway. Then
the gateway continues to use those names in the traces it sends to AGM.

For example, one of AGM's own implementing services uses QueryRoot, so traces
produced by the gateway in front of it have `QueryRoot` on the traces despite
that type not actually being part of the composed schema.

The effect is that field stats (on the schema explorer) for all root fields are
empty!

Because normalizing root operation names is part of the federation spec, it
makes sense to keep the implementing services simple and fix this just once in
the gateway.

This is tested by
packages/apollo-gateway/src/__tests__/gateway/reporting.test.ts which explicitly
contains 'Query' in the trace rather than 'RootQuery'.
@trevor-scheer trevor-scheer force-pushed the glasser/federated-tracing-root-type-names branch from 8b0eb04 to 7888057 Compare May 11, 2020 19:05
@trevor-scheer trevor-scheer merged commit 6de9ba0 into release-gateway-0.15.1@next May 11, 2020
@trevor-scheer trevor-scheer deleted the glasser/federated-tracing-root-type-names branch May 11, 2020 19:05
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants