Traffic shaping for connectors #6737

andrewmcgivery · 2025-02-06T06:04:17Z

Example config:

traffic_shaping:
  connector:
    sources:
      connector-graph.random_person_api:
        global_rate_limit:
          capacity: 20
          interval: 1s
        experimental_http2: http2only
        timeout: 1s

Checklist

Complete the checklist (and note appropriate exceptions) before the PR is marked ready-for-review.

Exceptions

Note any exceptions here

Notes

It may be appropriate to bring upcoming changes to the attention of other (impacted) groups. Please endeavour to do this before seeking PR approval. The mechanism for doing this will vary considerably, so use your judgement as to how and when to do this. ↩
Configuration is an important part of many changes. Where applicable please try to document configuration examples. ↩
Tick whichever testing boxes are applicable. If you are adding Manual Tests, please document the manual testing (extensively) in the Exceptions. ↩

svc-apollo-docs · 2025-02-06T06:04:21Z

✅ Docs preview has no changes

The preview was not built because there were no changes.

Build ID: 872f6b6d11aa1f1fd4e68499

router-perf · 2025-02-06T06:04:48Z

CI performance tests

apollo-router/src/services/supergraph/service.rs

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

…ove deduplicate query from the options

…icshaping

…s after fixing the RouterBody/String problem.

…icshaping

…ed string

lennyburdette · 2025-02-28T15:38:23Z

...uter/src/configuration/snapshots/apollo_router__configuration__tests__schema_generation.snap

+          "description": "#/definitions/Compression",
+          "nullable": true
+        },
+        "dns_resolution_strategy": {


am i correct in reading that dns_resolution_strategy and experimental_http2 aren't tested?

I did not explicitly write tests for them because the existing traffic shaping also doesn't.

My interpretation as to why is because while these settings are in traffic shaping, the actual logic/functionality for them is used elsewhere in other parts of the codebase.

lennyburdette · 2025-02-28T15:42:55Z

apollo-router/src/plugin/test/mock/connector.rs

were there things you had to test this way (using this new mock service and the tests in plugins/traffic_shaping/mod.rs) that warrant two ways of testing? i'm wondering if we can kill this in favor or tests/integration/traffic_shaping.rs?

I'm be fine with doing just the integration tests.

I did both ways mostly to match what was already being done in the existing tests.... but I'm not married to it. I tend to monkey-see-monkey-do when touching existing code. :D

lennyburdette · 2025-02-28T15:43:29Z

apollo-federation/src/sources/connect/models.rs

@@ -231,6 +231,16 @@ impl Connector {
            .map_err(|_| internal_error!("Failed to create key for connector {}", self.id.label)),
        }
    }
+
+    /// Create an identifier for this connector that can be used for configuration and service identification


can you document the behavior when source is None and why?

Added some more comment!

pubmodmatt · 2025-02-28T17:23:20Z

...uter/src/configuration/snapshots/apollo_router__configuration__tests__schema_generation.snap

@@ -1608,6 +1607,14 @@ snapshot_kind: text
          "description": "#/definitions/RouterShaping",
          "nullable": true
        },
+        "sources": {


I mentioned this in a doc comment but you might have missed it - we need an additional connector: layer here, above sources.

pubmodmatt · 2025-02-28T22:16:52Z

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

+            .sources
+            .get(&source_name)
+            .map(|connector_config| connector_config.clone().into());
+        let final_config = Self::merge_config(all_config, source_config.as_ref());


Although ConnectorShaping always sets deduplicate_query to None, this merge will cause it to be set if the all config has it set. See here

did this get addressed?

Yep! This was a problem because before the all config was the subgraph config which could contain deduplicate_query whereas now the merge will be two connector configs.

pubmodmatt · 2025-02-28T22:33:01Z

apollo-router/src/router_factory.rs

@@ -423,6 +424,31 @@ pub(crate) async fn create_http_services(
        let http_service_factory = HttpClientServiceFactory::new(http_service, plugins.clone());
        http_services.insert(name.clone(), http_service_factory);
    }
+
+    // Also create client service factories for connector sources


We'll have already redundantly created an HttpClientServiceFactory above for the expanded connector subgraph, and that one will no longer be used by the ConnectorRequestService. We should find a way to not create that.

Would creating a list of "connector subgraph names" prior to that loop and doing a comparison of the subgraph name versus the connector subgraph name suffice?

Basically... if we have a subgraph name that matches a connector subgraph name... don't add it.

Implemented what I stated above!

pubmodmatt · 2025-02-28T22:38:38Z

apollo-router/src/plugins/connectors/http_json_transport.rs

@@ -103,7 +103,7 @@ pub(crate) fn make_request(
                transport.body.as_ref().map(|body| SelectionData {
                    source: body.to_string(),
                    transformed: body.to_string(), // no transformation so this is the same
-                    result: json_body,
+                    result: json_body.clone(),


Why add this clone? It seems unnecessary.

Got left in from a previous iteration of this PR.. removed!

pubmodmatt · 2025-02-28T22:44:12Z

apollo-router/src/services/connector/request_service.rs

-                ),
-            )
-            .boxed()
+    pub(crate) fn create(&self, source_name: String) -> BoxService {


The CONNECT_REQUEST_SPAN_NAME span used to be created here - where does that happen now? I'm not finding it. It should likely move to the telemetry plugin connect_request_service method via span_factory like it is for subgraph service.

Ah crap, definitely accidentally nuked it. I'll look at adding it back!

Added it back as suggested!

pubmodmatt · 2025-03-01T00:36:38Z

apollo-router/src/plugins/connectors/handle_responses.rs

@@ -49,7 +49,7 @@ pub(crate) enum HandleResponseError {

 // --- RAW RESPONSE ------------------------------------------------------------

-enum RawResponse {
+pub(crate) enum RawResponse {


Why pub(crate)?

Got left in from a previous iteration of this PR.. removed!

…icshaping

lennyburdette · 2025-03-10T17:40:52Z

apollo-router/src/services/supergraph/service.rs

@@ -940,6 +953,17 @@ impl PluggableSupergraphServiceBuilder {
            .and_then(|plugin| (*plugin.1).as_any().downcast_ref::<Subscription>())
            .map(|p| p.config.clone());

+        let connector_sources: HashSet<String> = schema
+            .connectors


should we just make a getter on the Connectors struct for this? no need to compute it more than once

Just to clarify, this is to reduce some repetition since this exact code block is done in a couple of places?

yeah, both the set of subgraphs and set of source config keys can be calculated once at schema load time.

lennyburdette · 2025-03-10T17:45:18Z

apollo-router/src/plugins/connectors/handle_responses.rs

@@ -203,7 +203,7 @@ impl RawResponse {
 }

 // --- MAPPED RESPONSE ---------------------------------------------------------
-#[derive(Debug)]
+#[derive(Debug, Clone)]


remind me why this is clone? maybe even add a comment for future readers?

I must have accidentally left it in from a previous iteration of code 😓 removed!

…icshaping

Traffic shaping for connectors

d42233f

This comment has been minimized.

Sign in to view

lennyburdette reviewed Feb 6, 2025

View reviewed changes

apollo-router/src/services/supergraph/service.rs Outdated Show resolved Hide resolved

lennyburdette reviewed Feb 6, 2025

View reviewed changes

apollo-router/src/plugins/traffic_shaping/mod.rs Outdated Show resolved Hide resolved

andrewmcgivery added 15 commits February 6, 2025 14:58

Create new config object for connectors traffic shaping so we can rem…

108aa6c

…ove deduplicate query from the options

Remove deduplicate_query code for connectors

3876761

Remove authorization and operation_kind which are no longer in use

e6170eb

Moved construction of source names to a method on Connector

2565f49

Merge remote-tracking branch 'origin/dev' into feature/connectortraff…

4a8ec56

…icshaping

Started on tests but its a bit broken so... going to come back to thi…

7f51b27

…s after fixing the RouterBody/String problem.

Merge remote-tracking branch 'origin/dev' into feature/connectortraff…

695e12f

…icshaping

Fix test issue

f2ffa47

First passing test, fixing linting issues

e9d9eb6

Update sourceness connectors to use a synthetic name instead of a fix…

77ff8dd

…ed string

Test rate limiting on connector requests

cee38ab

Connector timeout integration test

c29c88f

Add connectors rate limiting integration test

9d54766

Removing some unused changes

454bdcd

Add changeset file

35ec535

andrewmcgivery marked this pull request as ready for review February 28, 2025 00:58

andrewmcgivery requested review from a team as code owners February 28, 2025 00:58

lennyburdette reviewed Feb 28, 2025

View reviewed changes

pubmodmatt reviewed Feb 28, 2025

View reviewed changes

pubmodmatt reviewed Mar 1, 2025

View reviewed changes

andrewmcgivery added 4 commits March 5, 2025 16:57

Add additional comment around source_config_key

3e62b5d

Remove unneeded clone

b8cf012

Removed pub(crate) that wasn't needed

dbd6ea3

Adjust configuration shape to match standard

c300bcb

andrewmcgivery requested review from a team as code owners March 6, 2025 22:23

andrewmcgivery added 7 commits March 6, 2025 17:27

Update snapshot

0c7949e

Avoid creating http factories that aren't needed for connector subgraphs

f19f317

Added CONNECT_REQUEST_SPAN_NAME back in after accidently nuking it

8b91a88

Updated snapshots

4dc9b44

Merge remote-tracking branch 'origin/dev' into feature/connectortraff…

e83b303

…icshaping

Merge remote-tracking branch 'origin/dev' into feature/connectortraff…

e5bdfa9

…icshaping

Merge remote-tracking branch 'origin/dev' into feature/connectortraff…

58e0056

…icshaping

lennyburdette reviewed Mar 10, 2025

View reviewed changes

andrewmcgivery added 5 commits March 10, 2025 13:56

Remove clone from MappedResponse

2e5a118

Merge remote-tracking branch 'origin/dev' into feature/connectortraff…

be5f67e

…icshaping

pre-compute source_config_keys as part of expand_connectors

969b7f3

Fix build issue

c35514f

Remove unneeded clones

3053da4

lennyburdette approved these changes Mar 10, 2025

View reviewed changes

andrewmcgivery merged commit 7b79bea into dev Mar 10, 2025
15 checks passed

andrewmcgivery deleted the feature/connectortrafficshaping branch March 10, 2025 20:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Traffic shaping for connectors #6737

Traffic shaping for connectors #6737

andrewmcgivery commented Feb 6, 2025 •

edited

Loading

svc-apollo-docs commented Feb 6, 2025 •

edited

Loading

This comment has been minimized.

router-perf bot commented Feb 6, 2025

lennyburdette Feb 28, 2025

andrewmcgivery Mar 5, 2025

lennyburdette Feb 28, 2025

andrewmcgivery Mar 5, 2025

lennyburdette Feb 28, 2025

andrewmcgivery Mar 5, 2025

pubmodmatt Feb 28, 2025

pubmodmatt Feb 28, 2025

lennyburdette Mar 10, 2025

andrewmcgivery Mar 10, 2025

pubmodmatt Feb 28, 2025

andrewmcgivery Mar 5, 2025

andrewmcgivery Mar 6, 2025

pubmodmatt Feb 28, 2025

andrewmcgivery Mar 5, 2025

pubmodmatt Feb 28, 2025

andrewmcgivery Mar 5, 2025

andrewmcgivery Mar 6, 2025

pubmodmatt Mar 1, 2025

andrewmcgivery Mar 5, 2025

lennyburdette Mar 10, 2025

andrewmcgivery Mar 10, 2025

lennyburdette Mar 10, 2025

lennyburdette Mar 10, 2025

andrewmcgivery Mar 10, 2025

Traffic shaping for connectors #6737

Traffic shaping for connectors #6737

Conversation

andrewmcgivery commented Feb 6, 2025 • edited Loading

Footnotes

svc-apollo-docs commented Feb 6, 2025 • edited Loading

✅ Docs preview has no changes

This comment has been minimized.

router-perf bot commented Feb 6, 2025

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

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

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

Choose a reason for hiding this comment

andrewmcgivery commented Feb 6, 2025 •

edited

Loading

svc-apollo-docs commented Feb 6, 2025 •

edited

Loading