Fix error, tracing, and cache control propagation in schema stitching

[stubailo]

Right now, errors, tracing info, and other stuff doesn't always make it across the stitching boundaries. This means sometimes using schema stitching can cause a degradation of developer experience compared to a non-stitched schema.

We should fix that, so that we can recommend stitching without reservation as a first-class way of developing schemas. I'm going to close issues related to this and point them here as a master list.

[cgatian]

Hate to be this guy... But are there any plans for this feature?

[krisread]

checkResultAndHandleErrors is really messing up schema stitching for us

We delegate and get a perfectly nice graphql error back from a downstream service

A transformer that we can't disable calls checkResultAndHandleErrors, transforms our nice downstream graphql error into an unusable error by concatenating the body into a string?

If we want to pass on (throw) the original downstream error we have no way.

[RickyV33]

I'll go ahead and piggyback off @krisread since we're experiencing the exact same issue. It'd be great to forward the initial error to clients so that downstream services can handle errors on their own..

[flippidippi]

Is this the reason we might not see info.cachControl in resolvers we build and schema stitch together using graphql-tools? If so this is a pretty big let down because we use schema stitching in our hapi GraphQL server to organize our larger schema and to stitch in remote schemas.

[yaacovCR]

@cgatian, @krisread, @RickyV33, this should be fixed in graphql-tools-fork, I wonder if you might be able to still show a breaking example.

@flipxfx, my guess is that is a separate issue related to multiple wrapping resolvers not working well together. My next fix planned for graphql-tools-fork is to fix default fields, #1121, but if you open a separate issue for working well with Apollo server cache, that seems like another good candidate. If you can include a small code reproduction that would be very helpful.

[macrozone]

@yaacovCR i tried graphql-tools-fork (v7.2.3) ,but cache-controls in the remote schema are not passed

[yaacovCR]

@macrozone if you can put together a minimal reproduction of what you expect to be available where I would be happy to take a look.

[macrozone]

@yaacovCR will do if i find time. To give you some more information:

i have a remote schema that defines cache hints via @cacheControl(maxAge: 300) on a Query.resolver

i have another ApolloServer in front of that that does schema stitching using makeRemoteExecutableSchema. The resulting graphql server does not respect the cacheHints on the remote schema

[yaacovCR]

@macrozone, did you set mergeDirectives to true btw?

#1003

[macrozone]

@yaacovCR thx! did not know that option. However it does not solve the issue. it just throws an error: "Error: Duplicate directive name(s): include, skip, deprecated"

Edit: that comes from the apollo-engine push, so not a real error on the server side.

however the @CacheControl from the merged remote schema is not respected.

I tried with both graphql-tools and graphql-tools-fork.

[yaacovCR]

I think the duplicate directives error was from graphql-tools, should have been fixed in the fork. I do not use Apollo engine so not sure exactly the workflow you are describing. I would be happy to learn, but probably best would be a minimal reproduction.

In terms of the cacheControl directive being respected, i imagine you may have to reannotate the fields (if possible?) on the merged schema with extend.

This is a bit heavy as related to graphql/graphql-js#1343 (comment) and lengthy discussion there.

[macrozone]

In terms of the cacheControl directive being respected, i imagine you may have to reannotate the fields (if possible?) on the merged schema with extend.

that's what i want to avoid at all cost. The remote schema is huge and already defines cache hints on some Query.resolvers

For the moment, i opened a discussion on spectrum: https://spectrum.chat/apollo/apollo-server/caching-remote-schemas-result~0b0f5d90-36a1-4b39-a231-a16e5db29d9f

i think its a topic that is not covered properly yet by the apollo team. Its a bit a chaos right now with the deprecated schema-stitching and federation. So i think i will step back and no longer try and wait what the community has to say on the topic.

[yaacovCR]

I disagree a bit. We are the community, waiting has been the strategy employed so far with little progress. I am volunteering to contribute some unpaid time to this issue -- I think you stand the best shot of a working approach in the foreseeable future by adding a failing test to graphql-tools-fork or by sharing a repository with a minimal reproduction.

[yaacovCR]

@macrozone, my current understanding is that directives are lost when getting a remote schema via introspection, but that this should work if you have access to original sdl. Will try to prove that in test in fork.

[macrozone]

@yaacovCR sorry, i did not want to slam your efforts on that, i really appreciate your time! I am more concerned about the direction the apollo team is taking schema stitching... (deprecating it).

Anyway.

yes, if you have access to the original schema, you can of course have these directives. But the scenario is, that the remote service is not under control, only introspectable.

i think i also made a misstake: In the end, the stitching server makes a http call to the remote call. Here caching could already be leveraged. I just dont know if the apollo server itself honors cache headers of the remote server. If so, the stitched schema does not need to know abot the remote's cache annotation

[yaacovCR]

Ahah!

I think you can get this working using a node fetch polyfill with cache support like https://github.com/npm/make-fetch-happen as the fetch option in your schema stitching link.

You also must use GET for queries and maybe get apollo-link-persisted-queries working on server side, but looks like a great idea!

[yaacovCR]

Closing for now, please reopen as necessary, error support rolled into #1306

[yaacovCR]

I don't think there is a separate issue that discusses tracing.

[yaacovCR]

I think we can close this stale issue -- from lack of interest about tracing.

We can track caching improvements at #1380 and we should be good on errors since merging of fork.

Tracing should work just fine for the gateway schema, although additional data would be necessary to determine if any bottlenecks are from the underlying subschemas. A plugin could be developed to extract that data, but the Apollo community and those interested in that feature seems to be moving toward Federation, with interest in this issue waning.

The community can reopen a fresh new issue about tracing if they have additional concerns.