Fix inverted mongodb_mongos_sharding_chunks_is_balancer_running gauge#1295
Fix inverted mongodb_mongos_sharding_chunks_is_balancer_running gauge#1295nisarg14 wants to merge 7 commits into
Conversation
nisarg14
requested review from
JiriCtvrtka and
ademidoff
and removed request for
a team
nisarg14
changed the title
|
Please add |
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## main #1295 +/- ##
==========================================
- Coverage 70.88% 65.82% -5.07%
==========================================
Files 28 29 +1
Lines 3569 2481 -1088
==========================================
- Hits 2530 1633 -897
+ Misses 904 702 -202
- Partials 135 146 +11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Confirmed the production fix is correct. The inversion is a regression from PMM-12962 (15688e3), which renamed chunksBalanced/is_balanced ("Shards are balanced", correctly !inBalancerRound) to chunksBalancerRunning/is_balancer_running but kept the !m.InBalancerRound logic, inverting the new semantics. This restores the intended behavior (the metric is only emitted under --compatible-mode). One note on the new test below.
| assert.NoError(t, err) | ||
|
|
||
| expected := 0 | ||
| if bs.InBalancerRound { |
There was a problem hiding this comment.
This expected computation mirrors the function under test exactly (chunksBalancerRunning applies the same if inBalancerRound { 1 } mapping), so the assertion is tautological. It does guard against a re-introduced ! inversion, but on an idle test cluster inBalancerRound is effectively always false, so only the → 0 path is ever exercised — the → 1 path (the behavior this PR fixes) is never covered. Consider extracting the bool→value mapping into a pure function and unit-testing both inputs, or at minimum add a comment noting this limitation.
There was a problem hiding this comment.
Thanks for the feedback. I’ve now extracted the bool->value mapping into a pure function and added a unit test covering both true and false cases. The integration test is kept only to verify the MongoDB response->metric wiring.
There was a problem hiding this comment.
Awesome. One final fix - let's run make format, commit and push - it should make the linter happy :)
There was a problem hiding this comment.
Sure, i've ran make format and pushed the code
That group was removed, so we are good - |
| assert.NoError(t, err) | ||
|
|
||
| expected := 0 | ||
| if bs.InBalancerRound { |
There was a problem hiding this comment.
Awesome. One final fix - let's run make format, commit and push - it should make the linter happy :)
2 participants
Fix inverted mongodb_mongos_sharding_chunks_is_balancer_running gauge
Summary
Fixes chunksBalancerRunning() so mongodb_mongos_sharding_chunks_is_balancer_running is 1 when MongoDB balancerStatus.inBalancerRound is true, and 0 otherwise.
Previously the condition was negated (!m.InBalancerRound), so the gauge reported the opposite of the metric name and help text (“Shard balancer is in a balancing round”).
Renames the existing TestMongosMetrics subtest to match what it actually tests (balancer_enabled via mode).
Adds a subtest that exercises chunksBalancerRunning() against inBalancerRound from balancerStatus.