feat(router): more intuitive subgraph traffic shaping config inheritance #2155
Conversation
endigma
requested review from
Noroth,
StarpTech,
devsergiy and
jensneuse
as code owners
WalkthroughNewTransportRequestOptions gained an optional defaults parameter and All-level TransportRequestOptions are used as defaults for per-subgraph options; Subgraphs changed from pointer to value entries and MaxConnsPerHost was introduced and propagated to subgraph options. Tests and some imports were updated accordingly. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Router image scan passed✅ No security vulnerabilities found in image: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
router/core/router.go (1)
1826-1837: Field-wise inheritance looks correct; retains explicit zero valuesThe or(...) helper preserves explicit zero values when a pointer is provided, which is important for timeouts and MaxConnsPerHost semantics (e.g., 0 meaning “unbounded”). No functional issues here.
Minor readability nit: the helper’s parameter name or shadows the function name and can be confusing when reading call sites like or(cfg.Foo, defaults.Foo). Consider renaming the parameter to fallback:
// outside the selected range (in or[T any] definition) func or[T any](maybe *T, fallback T) T { if maybe != nil { return *maybe } return fallback }router/core/router_test.go (1)
267-278: Nice: Validates subgraph overrides and multi-level fallbacks; add edge-case for explicit zero and nil entryCurrent checks cover:
- Subgraph-specific overrides (RequestTimeout, DialTimeout, including zero)
- Inheritance from All (MaxConnsPerHost)
- Fallthrough to global defaults (MaxIdleConns)
Consider adding:
- An explicit subgraph override for MaxConnsPerHost set to 0, ensuring it does not fall back to All (0 means “unbounded” and should be preserved).
- A case where a subgraph entry is nil to confirm it inherits from All without panicking.
Example additional tests (outside selected range):
func TestTrafficShapingRules_ExplicitZeroAndNilEntry(t *testing.T) { allMaxConnsPerHost := 128 rules := config.TrafficShapingRules{ All: config.GlobalSubgraphRequestRule{ MaxConnsPerHost: &allMaxConnsPerHost, }, Subgraphs: map[string]*config.GlobalSubgraphRequestRule{ "zero-override": { MaxConnsPerHost: func() *int { v := 0; return &v }() }, "nil-entry": nil, // should inherit from All }, } s := NewSubgraphTransportOptions(rules) // zero-override: preserve explicit 0 (unbounded) assert.Equal(t, 0, s.SubgraphMap["zero-override"].MaxConnsPerHost) // nil-entry: inherits from All assert.Equal(t, allMaxConnsPerHost, s.SubgraphMap["nil-entry"].MaxConnsPerHost) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
router/core/router.go(2 hunks)router/core/router_test.go(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: build-router
- GitHub Check: image_scan
- GitHub Check: image_scan (nonroot)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: integration_test (./events)
- GitHub Check: build_test
- GitHub Check: build_push_image (nonroot)
- GitHub Check: build_push_image
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (5)
router/core/router.go (1)
1819-1824: All call sites and docs are up to date
- Verified every
NewTransportRequestOptions(invocation in Go source uses two arguments (including tests inrouter/core/router_test.goand uses inrouter/core/router.go).- No references found in
.mdor.rstdocumentation that need updating.No further changes required.
router/core/router_test.go (4)
235-235: Good: Adds coverage for per-host concurrency capDefining allMaxConnsPerHost improves coverage for http.Transport.MaxConnsPerHost inheritance behavior.
243-246: All-level rule now includes MaxConnsPerHost: correct and aligned with semanticsIncluding MaxConnsPerHost at the All level reflects intended inheritance. This verifies that unspecified subgraph values inherit from All.
265-265: Assertion validates base-level MaxConnsPerHost inheritanceThis ensures the base TransportRequestOptions is constructed from All. Looks good.
292-292: Test updated to new API: correct usage with nil defaultsUsing NewTransportRequestOptions(*subgraphConfig, nil) properly validates default fallback behavior. Good coverage.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
router/core/router_test.go (1)
232-278: Consider verifying all TransportRequestOptions fields in tests (follow-up to prior feedback).Only a subset of fields are asserted (timeouts and MaxConnsPerHost). For completeness, add assertions for TLSHandshakeTimeout, ResponseHeaderTimeout, ExpectContinueTimeout, KeepAliveProbeInterval, KeepAliveIdleTimeout, MaxIdleConns, MaxIdleConnsPerHost on both the All-level and a subgraph to fully exercise inheritance semantics.
Would you like me to extend this test with those asserts?
🧹 Nitpick comments (4)
router-tests/telemetry/telemetry_test.go (1)
3642-3647: Value-type map looks correct; ensure the override is actually exercised by this testThe switch to
map[string]config.GlobalSubgraphRequestRuleand a value literal is correct and consistent with the repository-wide change away from pointers.However, the override is set for the "hobbies" subgraph while the query in this test hits the "employees" subgraph (as evidenced by spans/metrics using subgraph name "employees"). As written, the per-subgraph timeout override is likely not used, so this test won’t catch regressions in subgraph-specific inheritance.
Consider targeting a subgraph used by the query to strengthen the test:
- Subgraphs: map[string]config.GlobalSubgraphRequestRule{ - "hobbies": { - RequestTimeout: integration.ToPtr(3 * time.Second), - }, - }, + Subgraphs: map[string]config.GlobalSubgraphRequestRule{ + "employees": { + RequestTimeout: integration.ToPtr(3 * time.Second), + }, + },Alternatively, include a field that triggers the "hobbies" subgraph in the query, and assert the expected behavior.
router/core/router_test.go (1)
267-277: Nit: assert map entry presence before dereference to improve failure signals.If the map key is missing, the test will panic. Add an existence and non-nil assertion for clearer test failures.
Apply this diff:
- subgraphRequestOptions := router.subgraphTransportOptions.SubgraphMap["some-subgraph"] + subgraphRequestOptions, ok := router.subgraphTransportOptions.SubgraphMap["some-subgraph"] + require.True(t, ok, "subgraph transport options missing for key 'some-subgraph'") + require.NotNil(t, subgraphRequestOptions)router/core/router.go (2)
1819-1824: Add clarifying docstring on defaults/zero-value semantics.Make it explicit that nil pointers inherit defaults and non-nil pointers (even zero values) win. Improves readability for future maintainers.
Apply this diff:
-// NewTransportRequestOptions creates a new TransportRequestOptions instance with the given configuration and defaults. -// If defaults is nil, it uses the global default values. +// NewTransportRequestOptions creates a new TransportRequestOptions instance using hierarchical defaults. +// - If defaults is nil, DefaultTransportRequestOptions() is used. +// - For each field, a non-nil pointer in cfg overrides defaults. +// Note: zero values provided via non-nil pointers (e.g., 0s duration) are respected.
1879-1883: Circuit breaker inheritance behavior: confirm intent.Subgraph CB configs are now always set from v.CircuitBreaker without inheriting All-level fields per-field. Given CircuitBreaker uses non-pointer fields with env defaults, subgraphs cannot partially inherit All while overriding specific fields; they either omit the subgraph entirely (fall back to All) or fully override. If the product intent is per-field inheritance (like Transport options), we’ll need pointer fields or an explicit merge. If current behavior is intentional, consider documenting it in code/tests.
I can add a test that asserts:
- subgraph missing from cfg.Subgraphs inherits All CB config,
- subgraph present with only Enabled toggled uses subgraph values for all fields (no All per-field merge).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
router-tests/circuit_breaker_test.go(1 hunks)router-tests/lifecycle/shutdown_test.go(1 hunks)router-tests/prometheus_test.go(2 hunks)router-tests/telemetry/connection_metrics_test.go(1 hunks)router-tests/telemetry/telemetry_test.go(1 hunks)router-tests/timeout_test.go(3 hunks)router/core/router.go(3 hunks)router/core/router_test.go(3 hunks)router/pkg/config/config.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- router-tests/circuit_breaker_test.go
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: endigma
PR: wundergraph/cosmo#2155
File: router/core/router.go:1857-1866
Timestamp: 2025-08-20T10:08:17.801Z
Learning: router/pkg/config/config.schema.json forbids null values for traffic_shaping.subgraphs: additionalProperties references $defs.traffic_shaping_subgraph_request_rule with type "object". Therefore, in core.NewSubgraphTransportOptions, dereferencing each subgraph rule pointer is safe under schema-validated configs, and a nil-check is unnecessary.
📚 Learning: 2025-08-20T10:08:17.801Z
Learnt from: endigma
PR: wundergraph/cosmo#2155
File: router/core/router.go:1857-1866
Timestamp: 2025-08-20T10:08:17.801Z
Learning: router/pkg/config/config.schema.json forbids null values for traffic_shaping.subgraphs: additionalProperties references $defs.traffic_shaping_subgraph_request_rule with type "object". Therefore, in core.NewSubgraphTransportOptions, dereferencing each subgraph rule pointer is safe under schema-validated configs, and a nil-check is unnecessary.
Applied to files:
router-tests/timeout_test.gorouter/pkg/config/config.gorouter-tests/telemetry/connection_metrics_test.gorouter-tests/telemetry/telemetry_test.gorouter-tests/lifecycle/shutdown_test.gorouter-tests/prometheus_test.gorouter/core/router.gorouter/core/router_test.go
📚 Learning: 2025-08-20T10:08:17.800Z
Learnt from: endigma
PR: wundergraph/cosmo#2155
File: router/core/router.go:1857-1866
Timestamp: 2025-08-20T10:08:17.800Z
Learning: In the Cosmo router codebase, JSON schema validation prevents null values in TrafficShapingRules subgraph configurations, making nil checks unnecessary when dereferencing subgraph rule pointers in NewSubgraphTransportOptions.
Applied to files:
router-tests/timeout_test.gorouter/pkg/config/config.gorouter-tests/telemetry/connection_metrics_test.gorouter-tests/telemetry/telemetry_test.gorouter-tests/lifecycle/shutdown_test.gorouter-tests/prometheus_test.gorouter/core/router.gorouter/core/router_test.go
📚 Learning: 2025-07-21T14:46:34.879Z
Learnt from: SkArchon
PR: wundergraph/cosmo#2067
File: router/pkg/authentication/jwks_token_decoder.go:80-106
Timestamp: 2025-07-21T14:46:34.879Z
Learning: In the Cosmo router project, required field validation for JWKS configuration (Secret, Algorithm, KeyId) is handled at the JSON schema level in config.schema.json rather than through runtime validation in the Go code at router/pkg/authentication/jwks_token_decoder.go.
Applied to files:
router/core/router.go
📚 Learning: 2025-07-21T15:06:36.664Z
Learnt from: SkArchon
PR: wundergraph/cosmo#2067
File: router/pkg/config/config.schema.json:1637-1644
Timestamp: 2025-07-21T15:06:36.664Z
Learning: In the Cosmo router project, when extending JSON schema validation for security-sensitive fields like JWKS secrets, backwards compatibility is maintained by implementing warnings in the Go code rather than hard validation constraints in the schema. This allows existing configurations to continue working while alerting users to potential security issues.
Applied to files:
router/core/router.go
🧬 Code Graph Analysis (7)
router-tests/timeout_test.go (1)
router/pkg/config/config.go (1)
GlobalSubgraphRequestRule(190-207)
router-tests/telemetry/connection_metrics_test.go (1)
router/pkg/config/config.go (1)
GlobalSubgraphRequestRule(190-207)
router-tests/telemetry/telemetry_test.go (1)
router/pkg/config/config.go (1)
GlobalSubgraphRequestRule(190-207)
router-tests/lifecycle/shutdown_test.go (1)
router/pkg/config/config.go (1)
GlobalSubgraphRequestRule(190-207)
router-tests/prometheus_test.go (1)
router/pkg/config/config.go (1)
GlobalSubgraphRequestRule(190-207)
router/core/router.go (1)
router/pkg/config/config.go (2)
GlobalSubgraphRequestRule(190-207)CircuitBreaker(213-224)
router/core/router_test.go (2)
router/core/router.go (2)
DefaultTransportRequestOptions(1840-1854)NewTransportRequestOptions(1821-1838)router/pkg/config/config.go (2)
TrafficShapingRules(164-171)GlobalSubgraphRequestRule(190-207)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: build-router
- GitHub Check: build_test
- GitHub Check: image_scan
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: integration_test (./events)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: build_push_image (nonroot)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: image_scan (nonroot)
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (14)
router/pkg/config/config.go (1)
170-170: No remaining pointer-based Subgraphs references found
Verified that there are no legacy*GlobalSubgraphRequestRuleusages, no address-of onSubgraphsmap elements, and no functions or vars still using pointer types. The switch to a value-based map is safe and aligns with schema validation—this can be merged.router-tests/telemetry/connection_metrics_test.go (1)
164-169: LGTM: Subgraphs literal correctly updated to value-based mapThe move to
map[string]config.GlobalSubgraphRequestRulereads cleanly and matches the config type update. The per-subgraphRequestTimeoutoverride atop anAlldefault demonstrates the intended inheritance behavior.router-tests/lifecycle/shutdown_test.go (1)
30-41: LGTM: Value-based Subgraphs map in transport optionsUsing
map[string]config.GlobalSubgraphRequestRulewith pointer fields for inner values (e.g.,MaxIdleConns) looks correct and in line with the new config type. No issues spotted.router-tests/timeout_test.go (3)
51-55: LGTM: Value-based Subgraphs config literalThe updated map literal aligns with the config type change and keeps the subgraph-specific override concise.
185-192: LGTM: Per-subgraph overrides with value-based mapThe
hobbiesandtest1overrides atop the globalAlltimeout nicely exercise the new inheritance behavior.
301-305: LGTM: ResponseHeaderTimeout override using value-based SubgraphsThis test nicely covers a specific per-subgraph transport parameter override and is consistent with the new map value type.
router-tests/prometheus_test.go (2)
13-16: Import reshuffle is fine.No semantic impact. Aligns with usage below (metricdata, otel, rmetric).
4649-4654: Subgraphs map fully migrated to value typeRan a repository-wide search for any remaining
map[string]*config.GlobalSubgraphRequestRuleusages and single-argumentNewTransportRequestOptionscalls—no matches found. The shift to a value-typedSubgraphsmap is correctly applied throughout.router/core/router_test.go (5)
235-235: Good: exercising inheritance for MaxConnsPerHost.This ensures the new connection-cap inherits from All-level defaults.
243-246: All-rule now sets connection cap alongside timeouts — good coverage.Covers a representative mix of timeout and connection options for inheritance.
247-253: Value-typed Subgraphs map aligns with config change.Switching to map[string]config.GlobalSubgraphRequestRule mirrors the new config schema and avoids nil-pointer edge cases entirely.
264-266: Top-level inheritance test reads well.Asserting MaxConnsPerHost and defaulted MaxIdleConns on the base options verifies the merged defaults path.
286-293: NewTransportRequestOptions 2-arg usage LGTM.Passing nil to pick up DefaultTransportRequestOptions and verifying explicit-zero preservation covers the important cases.
router/core/router.go (1)
1857-1866: Hierarchical inheritance from All to subgraphs is correct and simplifies composition.Subgraphs now inherit from NewTransportRequestOptions(cfg.All, nil), then override with subgraph values, which matches the intended “All as defaults” behavior.
SkArchon
left a comment
SkArchon
left a comment
There was a problem hiding this comment.
PR seems ok to me, however comment on backwards compatibility
endigma
deleted the
jesse/eng-7950-more-intuitive-behaviour-for-subgraph-traffic-shaping-config
branch
Summary by CodeRabbit
New Features
Bug Fixes
Chores/Tests
Checklist