feat: generate operation names for subgraph fetches #1550

lennyburdette · 2022-03-01T00:02:59Z

Addresses #1544, #195, #146

codesandbox-ci · 2022-03-01T00:03:59Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

lennyburdette · 2022-03-01T00:04:22Z

i didn't change any the feature tests but i'm happy to do so if my approach is acceptable

pcmanus · 2022-03-01T15:06:58Z

Had a fairly quick scan, but the general approach looks good to me.

I do think the handling of unnamed operation doesn't work just right, as it ends up with operation named like undefined_accounts_0 and I believe your intend was that it would be UnnamedOperation_accounts_0.

And as an aside on that front, not sure I'm a fan of UnnamedOperation_accounts_0. Clearly a small point but I'd consider either:

just accounts_0. Not sure a somewhat random hard-coded string adds much here (and it makes query plan bigger, even if that's a fairly minor concern).
not having query names in the query plan at all (so like we do today) if the original query doesn't have one. One reason I though of that is that it would save us from having to update plenty of tests, and there's a logic to it I guess. But I suspect that some users may want operation names in query plan but without having a simple way to force "input" operations to have a names, so not certain this is a great solution.

lennyburdette · 2022-03-01T18:04:43Z

Excellent points! I talked to the customer and he thought that option 2 sounds right. I updated the code to drop operation names if the original request does not have one.

Should I start updating all the tests? Or is there anything else worth discussing/addressing first?

pcmanus · 2022-03-01T18:41:11Z

Should I start updating all the tests? Or is there anything else worth discussing/addressing first?

I'll do a final review pass on the final version, but I don't have anything else to bring. Seems like a great improvement with little downsides (especially if we use option 2 above which gives a de-factor option to "disable" the operation naming if for some reason someone wants that (can't really think of a reason but ...)). Re-tests, hopefully you don't have to change that much of them if you switch to "option 2" above since I believe most tests use unnamed operations (and so won't be impacted anymore).

Addresses apollographql#1544, apollographql#195, apollographql#146

lennyburdette · 2022-03-02T19:46:01Z

I can't add reviewers to the PR but I think this is good to go.

cpeacock · 2022-03-02T20:04:18Z

Added @pcmanus since he looked at it previously for the morning (if he can't get to it I'll look on Friday).

pcmanus

Lgtm, thanks!

abernix · 2022-03-03T08:08:33Z

gateway-js/src/__tests__/build-query-plan.feature

@@ -1213,7 +1219,8 @@ Scenario: supports multiple root mutations
            "password"
          ],
          "operationKind": "mutation",
-          "operation": "mutation($username:String!$password:String!){login(username:$username password:$password){__typename id}}"
+          "operation": "mutation LoginAndReview_accounts_0($username:String!$password:String!){login(username:$username password:$password){__typename id}}",
+          "operationName": "LoginAndReview_accounts_0"


Very thankful you chose to add operationName in here! (In a general sense, not just on this snapshot in particular!)

👍🏻 I had to in order to provide the operationName query param on the request, because that's probably what observability tools look at (instead of parsing the operation doc).

For context as to why i'm glad: it's only recently that we started adding operationKind in there, which enabled the router to make Good decisions without needing to — in the same spirit as you're describing — re-parse the operation.

abernix · 2022-03-03T17:26:35Z

I closed the related issues above referenced in the original post which weren't auto-closed. I also found #395, but I wasn't sure what the third reference was — #146 doesn't look relevant.

@pcmanus Are you keeping a running CHANGELOG for things like this?

abernix · 2022-03-03T17:54:56Z

gateway-js/src/__tests__/build-query-plan.feature

@@ -1405,7 +1418,8 @@ Scenario: multiple root mutations with correct service order
            ],
            "variableUsages": [],
            "operationKind": "query",
-            "operation": "query($representations:[_Any!]!){_entities(representations:$representations){...on User{reviews{product{__typename ...on Book{__typename isbn}...on Furniture{upc}}}}}}"
+            "operation": "query LoginAndReview_reviews_2($representations:[_Any!]!){_entities(representations:$representations){...on User{reviews{product{__typename ...on Book{__typename isbn}...on Furniture{upc}}}}}}",
+            "operationName": "LoginAndReview_reviews_2"


@pcmanus @lennyburdette Picking a random place to leave this post-merge thought — would it make sense to use double-underscores as the delimiter to create a more predictable pattern for parsing and to avoid name-conflicts across operation + subgraph names? Mostly just because it seems not completely unlikely that both an operation and a subgraph might use _ on its own and, while not forbidden, much less likely that someone would use __. We could even forbid __ as a valid graph name (and probably should or may already have a enforced character set somewhere?).

Just trying to avoid the case of two things end up being exactly equal. If my service name is movie_titles and my operation is get_movie_titles, I would have get_movie_titles_movie_titles_0. It would be nice if I could split on something that's: forbidden in the service name and therefore reliably delineates parts, and get_movie_titles__movie_titles__0 would get that.

It was similarly intentional that I'd suggested (in addition to the above) prefixing with __ in the (Slack) thread where this initially came up — just to indicate the generated-ness and avoid these name clashes, just in case people use these operations downstream in meaningful ways.

Also happy to ticket this for follow-up. Or not, depending on thoughts.

no strong feelings either way. i went with fewer characters but four extra underscores (two in the operationName query param and two in the operation document) isn't a huge deal.

I also like the idea of the double underscore separator, and if we are not already we should enforce that subgraphs have to start and end with a number or letter

pcmanus · 2022-03-07T15:07:47Z

Fyi, just noticed that we include the subgraph name here and that we enforce very little kind of limitations on subgraph names so this patch can currently very easily create invalid query names and break things. I'll write a followup patch for it. I might sneak in the double underscore change discussed above as well since it seems to be overall favored.

trevor-scheer · 2022-03-07T19:25:48Z

Should this be ported to the version-0.x branch?

lennyburdette · 2022-03-07T19:32:32Z

this patch can currently very easily create invalid query names and break things

if the original operation name name is bad, wouldn't things break in the validation phase before getting to the query planner?

Should this be ported to the version-0.x branch?

both customers i've talked to about this are fine with it being a 2.0-only improvement, but i'm sure others would be happy if you did!

pcmanus · 2022-03-08T09:02:38Z

if the original operation name name is bad, wouldn't things break in the validation phase

Yes, but it's not the original operation name the issue, it's the subgraph name, which is also included (as 2nd element) and is not otherwise validated. Anyway, #1568 has been pushed to fix it and has been merged (sorry for not waiting too long on feedback on that one but we want to do a new release very soon and this was a serious break so wanted to get it fixed asap).

chrskrchr · 2022-03-10T15:47:45Z

Should this be ported to the version-0.x branch?

both customers i've talked to about this are fine with it being a 2.0-only improvement, but i'm sure others would be happy if you did!

Any word on if/when this change will be backported to 0.x? Would love to rip out our custom operation propagation code.

…FetchNode`s (backport) (#1609) Backport of #1427 #1550 #1568 82d810f

dakshas · 2022-11-30T05:21:26Z

Hi folks! Can someone explain why the counter appended to the operation name is necessary ?
The counter makes tracking metrics slightly more complicated as now we need to use a wildcard to account for the changing counter values. Although unlikely, I'm worried the wildcard might lead to erroneous matching and there by incorrect metric calculations in cases where some operation names are substrings of other operation names.

I realize I can overwrite the operationName in my willSendRequest method, but just want to know more about what the original intention of adding the counter is.

pcmanus · 2022-11-30T13:30:38Z

why the counter appended to the operation name is necessary ?

Query plans can easily have multiple different fetches to the same subgraphs. Without the counter, each of those fetches, which can very different queries (say, they may fetch completely different entity types), would have the same name. Which might be acceptable in some use cases, but isn't in many others. Typically, if you use the operation name as an identifier for metrics, not having a counter would lump together the metrics of potentially fairly unrelated subgraph queries.

dakshas · 2022-11-30T22:10:34Z

@pcmanus thank you for the prompt reply!

lennyburdette force-pushed the fetchnode-operation-names branch from 93bb0a5 to 214cf38 Compare March 1, 2022 01:45

lennyburdette force-pushed the fetchnode-operation-names branch from 214cf38 to c7cecf7 Compare March 1, 2022 18:02

lennyburdette force-pushed the fetchnode-operation-names branch 2 times, most recently from 381a638 to d37800d Compare March 1, 2022 23:32

lennyburdette changed the title ~~DRAFT feat: generate operation names for subgraph fetches~~ feat: generate operation names for subgraph fetches Mar 1, 2022

lennyburdette marked this pull request as ready for review March 1, 2022 23:32

lennyburdette force-pushed the fetchnode-operation-names branch from d37800d to 8af25ee Compare March 2, 2022 00:32

feat: generate operation names for subgraph fetches

0ae9cd5

Addresses apollographql#1544, apollographql#195, apollographql#146

lennyburdette force-pushed the fetchnode-operation-names branch from 8af25ee to 0ae9cd5 Compare March 2, 2022 00:34

cpeacock requested a review from pcmanus March 2, 2022 20:03

pcmanus approved these changes Mar 3, 2022

View reviewed changes

pcmanus merged commit e73c15a into apollographql:main Mar 3, 2022

abernix reviewed Mar 3, 2022

View reviewed changes

This was referenced Mar 3, 2022

Provide naming of subgraph GraphQL operations instead of sending them as anonymous operations #1544

Closed

Including operation name in the query execution #195

Closed

Gateway is stripping operation names when forwarding to federated service #395

Closed

lennyburdette deleted the fetchnode-operation-names branch March 3, 2022 17:40

abernix reviewed Mar 3, 2022

View reviewed changes

pcmanus mentioned this pull request Mar 7, 2022

Fix operation names #1568

Merged

trevor-scheer mentioned this pull request Mar 15, 2022

feat(query-planner): Include operationKind and operationName in FetchNodes (backport) #1609

Merged

trevor-scheer added a commit that referenced this pull request Mar 17, 2022

feat(query-planner): Include operationKind and operationName in `…

982266e

…FetchNode`s (backport) (#1609) Backport of #1427 #1550 #1568 82d810f

smyrick mentioned this pull request Apr 8, 2022

Add generated operationName to query params apollographql/router#814

Closed

snyk-bot mentioned this pull request Jul 31, 2022

[Snyk] Security upgrade node-fetch from 2.6.7 to 3.2.10 NOUIY/federation#105

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: generate operation names for subgraph fetches #1550

feat: generate operation names for subgraph fetches #1550

lennyburdette commented Mar 1, 2022

codesandbox-ci bot commented Mar 1, 2022 •

edited

Loading

lennyburdette commented Mar 1, 2022

pcmanus commented Mar 1, 2022

lennyburdette commented Mar 1, 2022

pcmanus commented Mar 1, 2022

lennyburdette commented Mar 2, 2022

cpeacock commented Mar 2, 2022

pcmanus left a comment

abernix Mar 3, 2022 •

edited

Loading

lennyburdette Mar 3, 2022

abernix Mar 3, 2022

abernix commented Mar 3, 2022

abernix Mar 3, 2022 •

edited

Loading

lennyburdette Mar 3, 2022

smyrick Mar 4, 2022

pcmanus commented Mar 7, 2022

trevor-scheer commented Mar 7, 2022

lennyburdette commented Mar 7, 2022

pcmanus commented Mar 8, 2022

chrskrchr commented Mar 10, 2022

dakshas commented Nov 30, 2022 •

edited

Loading

pcmanus commented Nov 30, 2022

dakshas commented Nov 30, 2022

feat: generate operation names for subgraph fetches #1550

feat: generate operation names for subgraph fetches #1550

Conversation

lennyburdette commented Mar 1, 2022

codesandbox-ci bot commented Mar 1, 2022 • edited Loading

lennyburdette commented Mar 1, 2022

pcmanus commented Mar 1, 2022

lennyburdette commented Mar 1, 2022

pcmanus commented Mar 1, 2022

lennyburdette commented Mar 2, 2022

cpeacock commented Mar 2, 2022

pcmanus left a comment

Choose a reason for hiding this comment

abernix Mar 3, 2022 • edited Loading

Choose a reason for hiding this comment

lennyburdette Mar 3, 2022

Choose a reason for hiding this comment

abernix Mar 3, 2022

Choose a reason for hiding this comment

abernix commented Mar 3, 2022

abernix Mar 3, 2022 • edited Loading

Choose a reason for hiding this comment

lennyburdette Mar 3, 2022

Choose a reason for hiding this comment

smyrick Mar 4, 2022

Choose a reason for hiding this comment

pcmanus commented Mar 7, 2022

trevor-scheer commented Mar 7, 2022

lennyburdette commented Mar 7, 2022

pcmanus commented Mar 8, 2022

chrskrchr commented Mar 10, 2022

dakshas commented Nov 30, 2022 • edited Loading

pcmanus commented Nov 30, 2022

dakshas commented Nov 30, 2022

codesandbox-ci bot commented Mar 1, 2022 •

edited

Loading

abernix Mar 3, 2022 •

edited

Loading

abernix Mar 3, 2022 •

edited

Loading

dakshas commented Nov 30, 2022 •

edited

Loading