@apollo/gateway - Expose the current request query path to the "willSendRequest" and "didReceiveResponse" hooks #2384

talazenkot · 2023-02-08T11:37:10Z

Description:
Exposed the path to the node currently resolved by the gateway to the willSendRequest and didReceiveResponse hooks.

Solves #2353

…zenkot/federation into current_request_graph_path_exposure

netlify · 2023-02-08T11:37:13Z

👷 Deploy request for apollo-federation-docs pending review.

Visit the deploys page to approve it

Name	Link
🔨 Latest commit	`2da416c`

changeset-bot · 2023-02-08T11:37:13Z

🦋 Changeset detected

Latest commit: 2da416c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 7 packages

Name	Type
@apollo/gateway	Patch
@apollo/federation-internals	Patch
@apollo/composition	Patch
@apollo/query-planner	Patch
@apollo/query-graphs	Patch
@apollo/subgraph	Patch
apollo-federation-integration-testsuite	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

codesandbox-ci · 2023-02-08T11:38:11Z

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.

…zenkot/federation into current_request_graph_path_exposure

korinne · 2023-02-09T23:37:38Z

Thanks again for opening a PR. We have filed this PR as under review, and will provide feedback shortly.

talazenkot · 2023-02-13T10:45:18Z

Thanks again for opening a PR. We have filed this PR as under review, and will provide feedback shortly.

@korinne hey! any news regarding this? maybe there's something I can do from my side to push it forward?

pcmanus · 2023-02-20T17:40:21Z

gateway-js/src/datasources/types.ts

+  /**
+   * The path to the current processed node within the graph query
+   */
+  nodeGraphPath?: ResponsePath;


I'd move this inside the INCOMING_OPERATION part (that path makes no sense for a HEALTH_CHECK or LOADING_SCHEMA kind of request) and maybe rename it to something like pathInIncomingRequest (we call incomingRequestContext the total operation received by the gateway, and the "path" is really the path within that gateway operation at which this particular subgraph "sub-query" fits in, and that's probably how I would describe that field btw; I also wouldn't use node in the name as that's query plan specific naming and most users aren't meant to be familiar with that).

Could be nice to also document that the path may be undefined if it is unavailable, but will be empty (rather than undefined) for "top-level" fetches.

Thanks for the clarifications! I've adopted your guidelines.

pcmanus · 2023-02-20T17:55:22Z

.changeset/flat-books-sin.md

+"@apollo/gateway": patch
+---
+
+Current request graph path exposure


It would be awesome to expand this entry a bit. I'd suggest something along the lines of:

Exposes, for each subgraph request, the path in the overall gateway operation at which that subgraph request gets inserted. This path is now available as the pathInIncomingRequest field in the arguments of RemoteGraphQLDataSource.willSendRequest and RemoteGraphQLDataSource.didReceiveResponse.

Amazing. thanks!

…and changeset note, move pathInIncomingRequest to live within incoming operations context

pcmanus

A few last remarks. Sorry, I may not have been super clear in my previous ones.

pcmanus · 2023-02-21T09:59:05Z

gateway-js/src/datasources/RemoteGraphQLDataSource.ts

@@ -67,6 +68,11 @@ export class RemoteGraphQLDataSource<
    options: GraphQLDataSourceProcessOptions<TContext>,
  ): Promise<GatewayGraphQLResponse> {
    const { request, context: originalContext } = options;
+    const pathInIncomingRequest =
+      options.kind === GraphQLDataSourceRequestKind.INCOMING_OPERATION


I don't think we need that test (we can use options.pathInIncomingRequest directly): it's going to be undefined for non-INCOMING_OPERATION, but that feels almost more appropriate, and that avoids having to deal with null at all.

I changed the null assignment to undefined, however, I cannot use options.pathInIncomingRequest directly because the pathInIncomingRequest property is not guaranteed to exist in options because of the usage of typescript's union operator:

therefore, I must verify options.kind before using it, unless you can think of another approach?

Oh, correct, this is my bad.

pcmanus · 2023-02-21T10:05:10Z

gateway-js/src/datasources/types.ts

+
+      /**
+      * The path in the overall gateway operation at which that subgraph request gets inserted.
+      * Please note that this could be set to `undefined` when the path is not available, or set to `null` for top-level fetch operations.


I don't believe the "or set to null for top-level fetch operations" part is quite right: it should say "or be an empty array for top-level fetch operations".

What I mean/understand by "top-level" fetch operations is just subgraph fetches that are not entity fetches but rather query some Query/Mutation fields, and those will still be GraphQLDataSourceRequestKind.INCOMING_OPERATION but where the path in executeQueryPlan will be empty.

Your current patch sets null for queries that are not GraphQLDataSourceRequestKind.INCOMING_OPERATION, but those are queries that don't belong to the execution of a gateway query: they are instead "maintenance" queries the gateway does for either refreshing it's understanding of the schema or "health checks" of the subgraph. As said in my other comment, having the path to undefined to mean "not available" is fine for those imo.

Sorry. Misunderstood. Got you now.

talazenkot · 2023-02-21T13:01:24Z

A few last remarks. Sorry, I may not have been super clear in my previous ones.

No worries. Thanks a lot of your responsiveness and explanations. I've made a few adjustments according to your comments, and also explained why as far as I can tell I cannot use options.pathInIncomingRequest directly.

pcmanus

Lgtm. Thanks a lot for the contribution.

pcmanus · 2023-02-21T15:12:45Z

gateway-js/src/datasources/RemoteGraphQLDataSource.ts

@@ -67,6 +68,11 @@ export class RemoteGraphQLDataSource<
    options: GraphQLDataSourceProcessOptions<TContext>,
  ): Promise<GatewayGraphQLResponse> {
    const { request, context: originalContext } = options;
+    const pathInIncomingRequest =
+      options.kind === GraphQLDataSourceRequestKind.INCOMING_OPERATION


Oh, correct, this is my bad.

talazenkot added 4 commits February 8, 2023 12:49

expose the resolved node graph path via the request property

e3078e2

expose the resolved node graph path via the request property

049cb7b

Merge branch 'current_request_graph_path_exposure' of github.com:tala…

bdb1771

…zenkot/federation into current_request_graph_path_exposure

remove redundant attribute

e1db4fe

Create flat-books-sin.md

a629b6a

talazenkot changed the title ~~Current request graph path exposure~~ @apollo/gateway - Expose the current request query path to the "willSendRequest" and "didReceiveResponse" hooks Feb 8, 2023

talazenkot added 3 commits February 8, 2023 13:50

fix lint issues

255c52a

Merge branch 'current_request_graph_path_exposure' of github.com:tala…

b87d16e

…zenkot/federation into current_request_graph_path_exposure

Merge branch 'main' into current_request_graph_path_exposure

e098c73

korinne added the status/needs-review label Feb 9, 2023

Merge branch 'main' into current_request_graph_path_exposure

824e642

Merge branch 'main' into current_request_graph_path_exposure

7b94685

pcmanus suggested changes Feb 20, 2023

View reviewed changes

pcmanus reviewed Feb 20, 2023

View reviewed changes

talazenkot added 3 commits February 21, 2023 09:17

Merge branch 'main' into current_request_graph_path_exposure

aa92d13

rename nodeGraphPath to pathInIncomingRequest, improve documentation …

d829f0d

…and changeset note, move pathInIncomingRequest to live within incoming operations context

fix indentions

b4aa810

talazenkot requested a review from pcmanus February 21, 2023 09:41

pcmanus suggested changes Feb 21, 2023

View reviewed changes

remove null value & fix documentation

2da416c

talazenkot requested a review from pcmanus February 21, 2023 13:02

pcmanus assigned talazenkot Feb 21, 2023

pcmanus approved these changes Feb 21, 2023

View reviewed changes

pcmanus merged commit 1a7f484 into apollographql:main Feb 21, 2023

This was referenced Feb 21, 2023

release: on branch main #2393

Merged

release: on branch main NOUIY/federation#196

Open

release: on branch main AneethAnand/federation#1

Open

release: on branch next (alpha) #2364

Merged

release: on branch main NoobMaster-96/federation#2

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

@apollo/gateway - Expose the current request query path to the "willSendRequest" and "didReceiveResponse" hooks #2384

@apollo/gateway - Expose the current request query path to the "willSendRequest" and "didReceiveResponse" hooks #2384

talazenkot commented Feb 8, 2023 •

edited

Loading

netlify bot commented Feb 8, 2023 •

edited

Loading

changeset-bot bot commented Feb 8, 2023 •

edited

Loading

codesandbox-ci bot commented Feb 8, 2023 •

edited

Loading

korinne commented Feb 9, 2023

talazenkot commented Feb 13, 2023 •

edited

Loading

pcmanus Feb 20, 2023

talazenkot Feb 21, 2023

pcmanus Feb 20, 2023

talazenkot Feb 21, 2023

pcmanus left a comment

pcmanus Feb 21, 2023

talazenkot Feb 21, 2023

pcmanus Feb 21, 2023

pcmanus Feb 21, 2023

talazenkot Feb 21, 2023

talazenkot commented Feb 21, 2023

pcmanus left a comment

pcmanus Feb 21, 2023

@apollo/gateway - Expose the current request query path to the "willSendRequest" and "didReceiveResponse" hooks #2384

@apollo/gateway - Expose the current request query path to the "willSendRequest" and "didReceiveResponse" hooks #2384

Conversation

talazenkot commented Feb 8, 2023 • edited Loading

netlify bot commented Feb 8, 2023 • edited Loading

👷 Deploy request for apollo-federation-docs pending review.

changeset-bot bot commented Feb 8, 2023 • edited Loading

🦋 Changeset detected

codesandbox-ci bot commented Feb 8, 2023 • edited Loading

korinne commented Feb 9, 2023

talazenkot commented Feb 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pcmanus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

talazenkot commented Feb 21, 2023

pcmanus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

talazenkot commented Feb 8, 2023 •

edited

Loading

netlify bot commented Feb 8, 2023 •

edited

Loading

changeset-bot bot commented Feb 8, 2023 •

edited

Loading

codesandbox-ci bot commented Feb 8, 2023 •

edited

Loading

talazenkot commented Feb 13, 2023 •

edited

Loading