supergraph compose should call SubgraphFetchQuery only one time, not once for every subgraph

[theJC]

Description

It appears Rover supergraph compose makes a query SubgraphFetchQuery call for each subgraph, but the call gets all schemas for all subgraphs. Our supergraph in one of our environments has 70 subgraphs in it, and if I run the rover supergraph compose command with --log trace I see 70 calls with:

DEBUG rover_client::blocking::client: Request Body: {"variables":{"graph_id":"indeed-onegraph","variant":"qa"},"query":"query SubgraphFetchQuery($graph_id: ID!, $variant: String!) {\n service(id: $graph_id) {\n implementingServices(graphVariant: $variant) {\n __typename\n ... on FederatedImplementingServices {\n services {\n name\n url\n activePartialSchema {\n sdl\n }\n }\n }\n }\n }\n}\n","operationName":"SubgraphFetchQuery"}

And the command takes close to 50 seconds to complete.

Steps to reproduce

Cannot share exact supergraph yaml file being used as it contains specifics about our environment that I cannot share.

Expected result

Only one call to call the SubgraphFetchQuery should be made per execution of the rover supergraph compose command and it should take < 5 seconds to the command to complete.

Actual result

It took 50 seconds to accomplish and made 70 SubgraphFetchQuery calls

Environment

Run rover info and paste the results here
Rover Info:
Version: 0.4.1
Install Location: /Users/jchristiansen/.rover/bin/rover
OS: Mac OS 12.2.0 [64-bit]
Shell: /bin/zsh

[EverlastingBugstopper]

Hey @theJC, thanks for the report. I agree that this behavior is not ideal and there are many reasons to improve this.

Unfortunately, I don't think the Studio API currently supports fetching more than one subgraph schema with a given query so putting everything into a single query is blocked until that support comes.

We are talking about introducing a .rover "project" directory that might have some sort of "graph_modules" subdirectory akin to "node_modules" that would have copies of all of the resolved subgraphs. In order to not have to redownload that every time we'd need to store the history somehow, either with something like an etag or a better hash that we can get from the API.

Unfortunately I don't think this will be solved in the near future as we are still in design phase for this.

As a workaround for now until we have first-class support for this, you may be able to roll your own sorta hack around it if you write an "update" script that loops through the output of rover subgraph list, and then uses those subgraph names to run rover subgraph fetch and output them to a local directory. If you then refer to those local subgraph schemas in your supergraph.yml, it won't redownload the subgraph schemas, it'll read from what you have locally. You can then run your update script manually and while it'll be doing the same exact thing as rover supergraph compose that is inefficient and may take a while, it won't run on every composition which should speed up your iteration process.

Hope this helps!

[theJC]

@EverlastingBugstopper -- The response coming back a single execution of that query CONTAINS ALL of the subgraphs and their activePartialSchemas, it isn't returning just a single individual subgraph each time its called.

query SubgraphFetchQuery($graph_id: ID!, $variant: String!) {
  service(id: $graph_id) {
    implementingServices(graphVariant: $variant) {
      __typename
      ... on FederatedImplementingServices {
        services {
          name
          url
          activePartialSchema {
            sdl
          }
        }
      }
    }
  }
}

[EverlastingBugstopper]

AH! You're so right here! Ok wow this is an expensive query. It seems like the opposite of what I previously said is true, you can only fetch all subgraphs from a supergraph. for your case, these are all likely coming from the same graph, but it's feasible that you could be creating a supergraph from subgraphs that are deployed across multiple graphs or multiple variants. so, we need to query for all of those. however we could definitely go through and find all of the subgraphs we need and then batch those requests so that one request is performed per supergraph variant.

thanks for the report on this, we'll want to get this fixed up for sure.

[theJC]

@EverlastingBugstopper -- Can we get this being fixed removed from: #1000 -- it looks like it causes the top of this page show that its been fixed.

Also, can you update this ticket on status on if any progress is stalled out on this due to blockers on the Apollo studio API front? Any ticket you can point to to identify that work would be helpful.

[EverlastingBugstopper]

@theJC you're totally right - we made an improvement but not an actual fix for this. Re-opening and we'll take a look.

[theJC]

@EverlastingBugstopper -- Note, this is still showing up as closed.

[theJC]

FYI, optimizations here would provide significant developer experience lift:

Our QA env now consists of 149 subgraphs
Our PROD env now consists of 121 subgraphs

Here's our daily p95 and max for the past 60 days of our "subgraph overlay" activity in QA which consists of doing one rover subgraph list command, writing out of a yaml file based off it, and then one rover subgraph compose command:

Units are elapsed seconds:

[EverlastingBugstopper]

🤔 So, looking into this it seems like each of those queries is fetching one subgraph schema at a time and also fetching the names of all the subgraphs, like so:

query SubgraphFetchQuery($graphId: ID!, $variant: String!, $subgraphName: ID!) {
  graph(id: $graphId) {
    variant(name: $variant) {
      subgraph(name: $subgraphName) {
        name
        url
        activePartialSchema {
          sdl
        }
      }
      subgraphs {
        name
      }
    }
  }
}

Do you think the fix is as simple as removing the section that requests the names of all the subgraphs? I could imagine a world where we perform a second query after the initial one if no subgraph with that subgraphName was found in order to provide the nice "did you mean to type this subgraph" error message, but I'm not sure if that alone would fix the issue here.

One thing we could do here is to parallelize the requests being made in order to get some short term speed improvements and better CPU utilization.

[EverlastingBugstopper]

Looked into this a bit more today - removing the error handling with the list of subgraph names does not seem to meaningfully impact the time to config resolution.