fix: incorrect metrics for active connections - #3122
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
Claude Code Review is paused for this repository. To reconnect it, an admin of this repository's GitHub organization (or the account owner, for personal repositories) who can also manage your Claude organization's Code Review settings needs to re-link GitHub in Code Review settings. This is a one-time step.
Tip: disable this comment in your organization's Code Review settings.
Router image scan passed✅ No security vulnerabilities found in image: |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughChangesConnection metrics lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3122 +/- ##
==========================================
+ Coverage 1.58% 62.16% +60.57%
==========================================
Files 327 262 -65
Lines 45805 30978 -14827
Branches 485 0 -485
==========================================
+ Hits 728 19258 +18530
+ Misses 44763 10199 -34564
- Partials 314 1521 +1207
🚀 New features to boost your workflow:
|
![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-tests/telemetry/connection_metrics_hot_reload_test.go (1)
355-372: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFeature-flag configs alias the base config's subgraph messages.
Subgraphs: next.GetSubgraphs()hands the same[]*Subgraphpointers to every feature-flag config and to the base graph. It is read-only in this test today, but any future mutation (e.g. per-flag routing URL overrides) would silently affect all muxes. Cloning per flag, as done forEngineConfig, keeps the fixtures independent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@router-tests/telemetry/connection_metrics_hot_reload_test.go` around lines 355 - 372, Update reloadConfigPoller.configWithFeatureFlags so each feature-flag configuration receives an independently cloned Subgraphs slice and messages instead of reusing next.GetSubgraphs(). Preserve the existing base configuration and per-flag EngineConfig cloning, ensuring mutations to one flag’s subgraphs cannot affect the base graph or other feature-flag configs.router/core/graph_server.go (1)
272-277: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winRe-seed max-connections on each graph server build
router/core/graph_server.go:272-277
connectionMetricStoreis cached for the router lifetime, sostartInitMetricsonly emitsrouter.http.client.max_connectionsonce. Reloads that add subgraphs or changemax_conns_per_hostupdateConnectionPoolStats, but the gauge never gets re-emitted. Consider adding a smallSeedMaxConnectionshelper on the shared store and calling it after each graph server builds its transports.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@router/core/graph_server.go` around lines 272 - 277, The cached connection metric store does not re-emit max-connection gauges after graph server rebuilds. Add a small SeedMaxConnections helper to the shared connection metric store that refreshes gauges from current ConnectionPoolStats, then call it after connectionMetricStore succeeds in the graph server build flow before assigning s.connectionMetrics.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@router-tests/telemetry/connection_metrics_hot_reload_test.go`:
- Around line 227-263: Update activeConnections and its call from
requireActiveConnections to use the EventuallyWithT callback’s *assert.CollectT
(or another assert.TestingT) for collection and type assertions instead of the
outer *testing.T, ensuring no require.FailNow operation runs on the background
goroutine.
---
Nitpick comments:
In `@router-tests/telemetry/connection_metrics_hot_reload_test.go`:
- Around line 355-372: Update reloadConfigPoller.configWithFeatureFlags so each
feature-flag configuration receives an independently cloned Subgraphs slice and
messages instead of reusing next.GetSubgraphs(). Preserve the existing base
configuration and per-flag EngineConfig cloning, ensuring mutations to one
flag’s subgraphs cannot affect the base graph or other feature-flag configs.
In `@router/core/graph_server.go`:
- Around line 272-277: The cached connection metric store does not re-emit
max-connection gauges after graph server rebuilds. Add a small
SeedMaxConnections helper to the shared connection metric store that refreshes
gauges from current ConnectionPoolStats, then call it after
connectionMetricStore succeeds in the graph server build flow before assigning
s.connectionMetrics.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 55d6bc38-7693-451b-a981-0695545fad79
📒 Files selected for processing (7)
router-tests/telemetry/connection_metrics_hot_reload_test.gorouter/core/graph_server.gorouter/core/graph_server_test.gorouter/core/router.gorouter/pkg/metric/connection_pool_stats.gorouter/pkg/metric/oltp_connection_metric_store.gorouter/pkg/metric/prom_connection_metric_store.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@router/core/router.go`:
- Line 2654: Serialize initialization of connectionTraceDialer in newServer so
concurrent calls from buildExecutionConfigWatcher and buildManifestConfigWatcher
cannot race on the nil check. Guard the initialization with connectionStatsLock
or sync.Once, ensuring every reload path reuses the same dialer.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0117f5c1-8448-4f13-aed7-e3179618cc5c
📒 Files selected for processing (2)
router/core/graph_server_test.gorouter/core/router.go
🚧 Files skipped from review as they are similar to previous changes (1)
- router/core/graph_server_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@router/pkg/metric/connection_metric_store.go`:
- Around line 81-94: Update ConnectionMetrics.RecordMaxConnections to merge
c.baseAttributes into the per-record attributes before constructing the metric
option, while retaining the optional subgraph attribute. Ensure both OTLP and
Prometheus MeasureMaxConnections calls receive the combined base and subgraph
attributes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e1efd1ff-1738-4b4f-8851-287a2ed7d7a5
📒 Files selected for processing (6)
router/core/graph_server.gorouter/core/graph_server_test.gorouter/core/router.gorouter/pkg/metric/connection_metric_store.gorouter/pkg/metric/oltp_connection_metric_store.gorouter/pkg/metric/prom_connection_metric_store.go
💤 Files with no reviewable changes (2)
- router/pkg/metric/prom_connection_metric_store.go
- router/pkg/metric/oltp_connection_metric_store.go
🚧 Files skipped from review as they are similar to previous changes (3)
- router/core/router.go
- router/core/graph_server_test.go
- router/core/graph_server.go
…dont-count-into-active_connections
3 participants
Summary by CodeRabbit
active_connectionsmetric accuracy across router configuration hot reloads, including correct handling when subgraphs are added or removed.active_connections, plus lifetime tests for connection-metric store behavior across server swaps and shutdowns.Checklist
Open Source AI Manifesto
This project follows the principles of the Open Source AI Manifesto. Please ensure your contribution aligns with its principles.