chore(router): enrich logs with execution config hashes#2942
Merged
Conversation
Contributor
WalkthroughThe split-config poller now computes per-graph execution-config hashes and returns them in ChangesExecution-Config Hash Tracking and Logging
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Comment |
Router image scan passed✅ No security vulnerabilities found in image: |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2942 +/- ##
==========================================
+ Coverage 65.56% 66.33% +0.77%
==========================================
Files 327 258 -69
Lines 46918 27539 -19379
Branches 5250 0 -5250
==========================================
- Hits 30763 18269 -12494
+ Misses 16131 7818 -8313
- Partials 24 1452 +1428
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
router/core/graph_server.go (2)
369-370: ⚡ Quick winConsider defensive nil checks for
response.Hashes.Same concern as the previous comment - accessing
response.Hashes[""]without nil check.🤖 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 369 - 370, The log line assumes response.Hashes and response.Hashes[""] exist; add defensive nil/exists checks before accessing NewHash (e.g., verify response != nil, response.Hashes != nil and that the key "" exists) in the graph server code path that contains s.logger.With(...).Debug(...) so you only reference response.Hashes[""].NewHash when safe and fall back to a safe placeholder or skip the field when absent.
351-354: ⚡ Quick winConsider defensive nil checks for
response.Hashes.The code accesses
response.Hashes[""]without checking ifHashesis nil. While the currentsplitConfigPollerimplementation always initializes this map, otherConfigPollerimplementations might not. Consider adding a nil check or documenting the contract thatConfigPoller.GetRouterConfigmust always populateHashes.🛡️ Defensive coding suggestion
+ baseHash := response.Hashes[""] + if response.Hashes == nil { + baseHash = routerconfig.HashInfo{} + } s.logger.With( - zap.String("old_execution_config_hash", response.Hashes[""].OldHash), - zap.String("new_execution_config_hash", response.Hashes[""].NewHash), + zap.String("old_execution_config_hash", baseHash.OldHash), + zap.String("new_execution_config_hash", baseHash.NewHash), ).Debug("Will build a new base graph mux for new graph server")🤖 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 351 - 354, The log accesses response.Hashes[""] without guarding against a nil map; add a defensive nil-check (or zero-value fallback) before indexing response.Hashes in the graph server code path that logs the hashes (the variables referenced as response.Hashes[""].OldHash / NewHash) so you don't panic if a ConfigPoller implementation returns a nil Hashes map; alternatively enforce/document the contract on ConfigPoller.GetRouterConfig (and update splitConfigPoller if needed) to always initialize response.Hashes, but the simplest fix is to check response.Hashes != nil (and handle/mask missing keys) before reading response.Hashes[""] in the function that builds the base graph mux.
🤖 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/graph_server.go`:
- Around line 592-595: The current logger.Debug call reads
opts.hashes[featureFlagName].NewHash without verifying the key exists, which can
log empty hashes for flags skipped due to fetch failures; update the logic
around the Debug call in graph_server.go (the block that logs "Will reuse
feature flag mux for new graph server" referencing featureFlagName and
opts.hashes) to first check for the presence of opts.hashes[featureFlagName],
and if missing emit an alternate, clear message (e.g., indicate the flag was
skipped/stale due to prior fetch failure) while preserving the original debug
message when the hash exists; ensure this change handles the
SkipMissingFeatureFlags/featureFlagConfigs case so operators see distinct logs
for reused muxes with valid hashes vs skipped/stale flags.
---
Nitpick comments:
In `@router/core/graph_server.go`:
- Around line 369-370: The log line assumes response.Hashes and
response.Hashes[""] exist; add defensive nil/exists checks before accessing
NewHash (e.g., verify response != nil, response.Hashes != nil and that the key
"" exists) in the graph server code path that contains
s.logger.With(...).Debug(...) so you only reference response.Hashes[""].NewHash
when safe and fall back to a safe placeholder or skip the field when absent.
- Around line 351-354: The log accesses response.Hashes[""] without guarding
against a nil map; add a defensive nil-check (or zero-value fallback) before
indexing response.Hashes in the graph server code path that logs the hashes (the
variables referenced as response.Hashes[""].OldHash / NewHash) so you don't
panic if a ConfigPoller implementation returns a nil Hashes map; alternatively
enforce/document the contract on ConfigPoller.GetRouterConfig (and update
splitConfigPoller if needed) to always initialize response.Hashes, but the
simplest fix is to check response.Hashes != nil (and handle/mask missing keys)
before reading response.Hashes[""] in the function that builds the base graph
mux.
🪄 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: 4b63fa64-26c8-4886-93b5-72579511cc87
📒 Files selected for processing (3)
router/core/graph_server.gorouter/pkg/controlplane/configpoller/split_config_poller.gorouter/pkg/routerconfig/client.go
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
router/pkg/controlplane/configpoller/split_config_poller_test.go (1)
260-261: ⚡ Quick winUse explicit assertions for hash presence/absence.
The current assertions are ambiguous:
- Line 260:
assert.Empty(t, resp.Hashes["missing"])passes whether the key is absent or present with a zero-valueHashInfo{}. According to the contract (context snippet 1), skipped flags should not be in the map at all.- Line 261:
assert.NotEmptyis less specific than validating the actual hash value.For consistency with other tests (lines 88, 133-134, 490-491) and clearer contract verification, consider:
assert.NotContains(t, resp.Hashes, "missing", "skipped flag must not appear in Hashes") assert.Equal(t, routerconfig.HashInfo{NewHash: "hash-available"}, resp.Hashes["available"])🤖 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/pkg/controlplane/configpoller/split_config_poller_test.go` around lines 260 - 261, Replace the ambiguous assertions on resp.Hashes: instead of assert.Empty(t, resp.Hashes["missing"]) and assert.NotEmpty(t, resp.Hashes["available"]), assert that the "missing" key is not present and verify the exact HashInfo for "available"; specifically use assert.NotContains(t, resp.Hashes, "missing", "skipped flag must not appear in Hashes") and assert.Equal(t, routerconfig.HashInfo{NewHash: "hash-available"}, resp.Hashes["available"]) so the test checks absence vs exact value presence for resp.Hashes.
🤖 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.
Nitpick comments:
In `@router/pkg/controlplane/configpoller/split_config_poller_test.go`:
- Around line 260-261: Replace the ambiguous assertions on resp.Hashes: instead
of assert.Empty(t, resp.Hashes["missing"]) and assert.NotEmpty(t,
resp.Hashes["available"]), assert that the "missing" key is not present and
verify the exact HashInfo for "available"; specifically use
assert.NotContains(t, resp.Hashes, "missing", "skipped flag must not appear in
Hashes") and assert.Equal(t, routerconfig.HashInfo{NewHash: "hash-available"},
resp.Hashes["available"]) so the test checks absence vs exact value presence for
resp.Hashes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b9946685-0bf5-4e9c-9b1a-717be8ef4795
📒 Files selected for processing (3)
router/pkg/controlplane/configpoller/split_config_poller.gorouter/pkg/controlplane/configpoller/split_config_poller_test.gorouter/pkg/routerconfig/client.go
🚧 Files skipped from review as they are similar to previous changes (2)
- router/pkg/routerconfig/client.go
- router/pkg/controlplane/configpoller/split_config_poller.go
dkorittki
commented
Jun 10, 2026
Noroth
approved these changes
Jun 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
During graph server swap, enrich logs with old and new execution config hashes. Helps users identify the used router execution configs. Also fixes a bug where a failed initial fetch from CDN would result in next fetch attempts to ignore that particular engine configuration.
Summary by CodeRabbit
New Features
Bug Fixes & Improvements
Tests
Checklist
Open Source AI Manifesto
This project follows the principles of the Open Source AI Manifesto. Please ensure your contribution aligns with its principles.