persisted queries have client names #6198

glasser · 2024-10-25T23:48:02Z

(This depends on functionality of GraphOS that is not currently enabled for all users, as well as an unreleased version of Rover.)

The persisted query manifest fetched from Uplink can now contain a clientName field in each operation. Two operations with the same id but different clientName are considered to be distinct operations (and may have distinct bodies).

Router resolves the client name by taking the first of these which exists:

Reading the apollo_persisted_queries::client_name context key (which may be set by a router_service plugin)
Reading the HTTP header named by telemetry.apollo.client_name_header (which defaults to apollographql-client-name)

If a client name can be resolved for a request, Router first tries to find a persisted query with the specified ID and the resolved client name.

If there is no operation with that ID and client name, or if a client name cannot be resolved, Router tries to find a persisted query with the specified ID and no client name specified. (This means that existing PQ lists that do not contain client names will continue to work.)

This PR also improves the internal "samples" integration test suite:

Tests can be specified in YAML in addition to JSON
Unknown fields in test plans lead to test failure instead of being ignored
Test "Request" actions can have descriptions that are printed (to help find which step failed)

The existing PQ integration test is moved into the "samples" suite.

svc-apollo-docs · 2024-10-25T23:48:05Z

✅ Docs Preview Ready

Configuration

{
  "repoOverrides": {
    "apollographql/router@main": {
      "remote": {
        "owner": "apollographql",
        "repo": "router",
        "branch": "glasser/pq-client-name"
      }
    }
  }
}

Name	Link
Commit	`54e79ca`
Preview	https://www.apollographql.com/docs/deploy-preview/9f521e9e48ad13c0c2d9
Build ID	9f521e9e48ad13c0c2d9
File Changes	0 new, 9 changed, 0 removed * graphos/reference/router/telemetry/instrumentation/standard-attributes.mdx * graphos/reference/router/telemetry/instrumentation/standard-instruments.mdx * graphos/reference/router/telemetry/instrumentation/selectors.mdx * graphos/reference/router/telemetry/metrics-exporters/datadog.mdx * graphos/reference/router/telemetry/trace-exporters/datadog.mdx * graphos/reference/router/configuration.mdx * graphos/routing/observability/client-awareness.mdx * graphos/routing/security/persisted-queries.mdx * graphos/routing/configure-your-router.mdx
Pages	9

9 pages published. Build will be available for 30 days.

router-perf · 2024-10-25T23:48:33Z

CI performance tests

glasser · 2024-10-25T23:51:03Z

apollo-router/src/services/layers/persisted_queries/mod.rs

-            {
+            if let Some(persisted_query_body) = manifest_poller.get_operation_body(
+                persisted_query_id,
+                request.context.get(CLIENT_NAME).unwrap_or_default(),


This probably implies that the new feature only works if Apollo telemetry is enabled. This is probably a reasonable constraint for an initial release, but we should at least document it.

Jephuff · 2024-10-30T21:04:33Z

apollo-router/src/services/layers/persisted_queries/manifest_poller.rs

            .cloned()
+        {
+            Some(body)
+        } else if client_name.is_some() {


Do we need this else if? It looks like we would be returning the null operation for the a request for an operation with a client_name. If client_name is Some and nothing exists in state.persisted_query_manifest for it, None feels like the right return?

What this is saying is — if we don't have an operation that is specifically designated for this client name, try one that was published without a client name. (If we didn't have this, all PQs would suddenly stop working for any clients that send a client name as soon as you upgraded to this version.)

The reason it's a conditional at all instead of just an "else try client_name: None" is because if client_name is None, it would be making the same exact call twice, which seems wasteful.

I should add comments to this function though!

Ooohhhh riiight! that totally makes sense! comment will definitely help, but logic is definitely good! thanks :)

glasser · 2024-11-27T01:33:45Z

apollo-router/tests/integration_tests.rs

+        hashmap! {"accounts".to_string() => 1}
+    );
+
+    // Successfully run a persisted query with client name that has its own operation,


I'm not sure how to add an integration test that tests the "client name read from the header named by telemetry.apollo.client_name_header because I'm not sure if there's an easy way to enable the part of the telemetry code that reads the header without setting up a full reporting agent situation. (Maybe it would be OK for the PQ code to directly read this config field from the configuration instead? I had trouble getting that to work though.)

There are other integration tests that actually spin up a router, see the apollo-router/tests directory. This enables a full testing of a feature including telemetry.

That is the directory I'm in?

That said, this becomes trivial if I'm able to make the change mentioned above where the telemetry config is respected even when telemetry isn't enabled.

glasser · 2024-11-27T01:35:32Z

apollo-router/src/plugins/telemetry/mod.rs

@@ -172,7 +172,7 @@ pub(crate) mod tracing;
 pub(crate) mod utils;

 // Tracing consts
-const CLIENT_NAME: &str = "apollo_telemetry::client_name";
+pub(crate) const CLIENT_NAME: &str = "apollo_telemetry::client_name";


This PR involves various pub and pub(crate) changes; this one is to make the actual logic work, and others are to make the integration tests work. I don't know if that's appropriate, just that it makes it build.

glasser · 2024-11-27T01:35:46Z

I think this is ready for an initial review. It still needs docs and a changeset entry, but it is code complete and has tests. I'd appreciate advice on how to write one particular test as mentioned in my comment.

apollo-router/src/services/layers/persisted_queries/manifest_poller.rs

BrynCooke · 2024-11-27T13:53:28Z

apollo-router/src/services/layers/persisted_queries/mod.rs

+                        request
+                            .supergraph_request
+                            .headers()
+                            .get(client_name_header_default_str())


This won't be right if the user has configured a custom client header name.
Telemetry puts the client name into the context. So why do we need to check for a header at all here?

It seemed reasonable to have the feature not be completely broken if telemetry isn't enabled (which, eg, was the case in the integration tests). Eg if you're running your router locally with your client that's built to sent PQs. That said I feel like I'd be happier if I could get the implementation to be "honor telemetry.apollo.client_name_header even if APOLLO_KEY isn't set" but I couldn't quite figure out a comfortable way to get access to the plugin configuration from outside of it.

(I suppose another potential approach would be to refactor the telemetry plugin somehow so that it puts the client name in the context even if the actual reporting mechanism isn't enabled via API key.)

BrynCooke · 2024-11-27T13:57:33Z

apollo-router/tests/integration_tests.rs

+        hashmap! {"accounts".to_string() => 1}
+    );
+
+    // Successfully run a persisted query with client name that has its own operation,


There are other integration tests that actually spin up a router, see the apollo-router/tests directory. This enables a full testing of a feature including telemetry.

glasser · 2024-11-27T15:42:01Z

(oops, I'd been testing with cargo build and running my integration test and I missed that I broke the unit tests.)

glasser · 2024-11-27T23:41:41Z

apollo-router/tests/integration_tests.rs

-    assert_eq!(actual.data, None);
-    assert_eq!(registry.totals(), hashmap! {"accounts".to_string() => 1});
-
-    // We didn't break normal GETs.


This particular sub-test got lost because the samples files don't let me specify GET requests with checked responses.

glasser · 2024-11-27T23:43:38Z

@BrynCooke OK, this still lacks docs/changelog, but it's using a simpler form of testing that makes it more clear that the telemetry plugin is always loaded, so I was able to remove the apollographql-client-name fallback.

BrynCooke

Looks great with the new json based tests.
I still think unit testable parts should be unit tested though.

glasser

@BrynCooke OK, docs and changeset and unit tests done! I think this is ready for final consideration. Hoping it gets into v1.59.0! I'll coordinate the Rover and GraphOS-server-side changes.

glasser · 2024-12-04T01:22:44Z

docs/source/routing/security/persisted-queries.mdx

@@ -138,6 +138,20 @@ To enable safelisting, you _must_ turn off [automatic persisted queries](/router

 </Note>

+### Customization via request context


Note that there may be a second entry in this section soon depending on how ROUTER-890 is implemented.

glasser · 2024-12-05T22:06:34Z

!docs preview main

This depends on unreleased functionality of GraphOS.

glasser · 2024-12-05T22:15:04Z

!docs preview main

glasser · 2024-12-05T22:41:08Z

!docs preview main

This comment has been minimized.

Sign in to view

glasser commented Oct 25, 2024

View reviewed changes

Jephuff reviewed Oct 30, 2024

View reviewed changes

glasser force-pushed the glasser/pq-client-name branch 2 times, most recently from e20b1d2 to 7348223 Compare November 26, 2024 20:42

glasser commented Nov 27, 2024

View reviewed changes

BrynCooke requested changes Nov 27, 2024

View reviewed changes

glasser force-pushed the glasser/pq-client-name branch from 3941062 to 80f2c83 Compare November 27, 2024 20:24

glasser commented Nov 27, 2024

View reviewed changes

glasser force-pushed the glasser/pq-client-name branch from 784cc06 to f985c5e Compare November 28, 2024 05:50

BrynCooke reviewed Nov 28, 2024

View reviewed changes

abernix changed the title ~~[FOUN-575] persisted queries have client names~~ persisted queries have client names Nov 29, 2024

glasser force-pushed the glasser/pq-client-name branch from f985c5e to 03a70d1 Compare December 3, 2024 02:10

glasser commented Dec 4, 2024

View reviewed changes

glasser marked this pull request as ready for review December 4, 2024 01:23

glasser requested review from a team as code owners December 4, 2024 01:23

glasser force-pushed the glasser/pq-client-name branch from 921fe00 to bf7ba2c Compare December 4, 2024 01:24

glasser mentioned this pull request Dec 5, 2024

[ROUTER-890] Ability to skip safelisting enforcement via plugin #6403

Merged

glasser force-pushed the glasser/pq-client-name branch from bf7ba2c to 8b459b7 Compare December 5, 2024 02:47

BrynCooke approved these changes Dec 5, 2024

View reviewed changes

[FOUN-575] persisted queries have client names

c4a24d3

This depends on unreleased functionality of GraphOS.

glasser added 9 commits December 5, 2024 14:12

Get client name for more places; tests

d40678f

checkpoint

6bfdef9

remove old test

e3fb191

undo no-longer-necessary changes

09f7715

Fix unit tests

2b0befa

carg fmt

9506cd6

lint

f0d53cd

unit tests

3b4e9ca

Docs and changeset

92172c9

glasser force-pushed the glasser/pq-client-name branch 2 times, most recently from 0797d3e to 2bbe77c Compare December 5, 2024 22:14

update link in docs

54e79ca

glasser force-pushed the glasser/pq-client-name branch from 2bbe77c to 54e79ca Compare December 5, 2024 22:40

BrynCooke merged commit eb2bca6 into dev Dec 6, 2024
13 checks passed

BrynCooke deleted the glasser/pq-client-name branch December 6, 2024 16:29

BrynCooke mentioned this pull request Dec 16, 2024

prep release: v1.59.0 #6460

Merged

theJC mentioned this pull request Dec 17, 2024

Persisted Query Safelisting : incremental adoption capability #6469

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

persisted queries have client names #6198

persisted queries have client names #6198

glasser commented Oct 25, 2024 •

edited by abernix

Loading

svc-apollo-docs commented Oct 25, 2024 •

edited

Loading

This comment has been minimized.

router-perf bot commented Oct 25, 2024

glasser Oct 25, 2024

Jephuff Oct 30, 2024

glasser Oct 30, 2024

Jephuff Oct 30, 2024

glasser Nov 27, 2024

BrynCooke Nov 27, 2024

glasser Nov 27, 2024

glasser Nov 27, 2024

glasser commented Nov 27, 2024

BrynCooke Nov 27, 2024

glasser Nov 27, 2024 •

edited

Loading

glasser Nov 27, 2024

BrynCooke Nov 27, 2024

glasser commented Nov 27, 2024

glasser Nov 27, 2024

glasser commented Nov 27, 2024

BrynCooke left a comment

glasser left a comment

glasser Dec 4, 2024

glasser commented Dec 5, 2024

glasser commented Dec 5, 2024

glasser commented Dec 5, 2024

		@@ -138,6 +138,20 @@ To enable safelisting, you _must_ turn off [automatic persisted queries](/router

		</Note>

		### Customization via request context

persisted queries have client names #6198

persisted queries have client names #6198

Conversation

glasser commented Oct 25, 2024 • edited by abernix Loading

svc-apollo-docs commented Oct 25, 2024 • edited Loading

✅ Docs Preview Ready

This comment has been minimized.

router-perf bot commented Oct 25, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glasser commented Nov 27, 2024

Choose a reason for hiding this comment

glasser Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glasser commented Nov 27, 2024

Choose a reason for hiding this comment

glasser commented Nov 27, 2024

BrynCooke left a comment

Choose a reason for hiding this comment

glasser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glasser commented Dec 5, 2024

glasser commented Dec 5, 2024

glasser commented Dec 5, 2024

glasser commented Oct 25, 2024 •

edited by abernix

Loading

svc-apollo-docs commented Oct 25, 2024 •

edited

Loading

glasser Nov 27, 2024 •

edited

Loading