Skip to content

Commit 091edfd

Browse files
metrics: fix section_filter no-op on scalars (operator precedence) (#533)
`if collect and type(v) is float or type(v) is int:` parsed as `(collect and type(v) is float) or (type(v) is int)` due to Python's `and`/`or` precedence, so the `collect` flag -- which carries the section_filter decision -- was effectively ignored whenever the value was an int. Every int-valued INFO key leaked past the filter and into the overall map, regardless of whether the caller had whitelisted it. Parenthesize to `if collect and (type(v) is float or type(v) is int):` so the filter actually filters scalars too. Impact: callers of `collect_redis_metrics` that pass a `section_filter` now get exactly the keys they asked for. In particular, `collect_search_and_bigredis_metrics` (0.12.27) was silently pulling every scalar from bigredis / search_memory / search_disk instead of the four it lists. With this fix it returns strictly the documented keys. Adds two mock-based regression tests: - test_collect_redis_metrics_section_filter_drops_scalar_keys: the intended semantics -- only whitelisted keys survive. - test_collect_redis_metrics_no_filter_keeps_all_scalars: legacy default path (no filter) still collects every scalar. Bumps pyproject.toml 0.12.26 -> 0.12.28 (skipping .27 which is in #531's pipeline).
1 parent dcad735 commit 091edfd

3 files changed

Lines changed: 62 additions & 7 deletions

File tree

pyproject.toml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
[tool.poetry]
22
name = "redisbench-admin"
3-
version = "0.12.27"
3+
version = "0.12.28"
44
description = "Redis benchmark run helper. A wrapper around Redis and Redis Modules benchmark tools ( ftsb_redisearch, memtier_benchmark, redis-benchmark, aibench, etc... )."
55
authors = ["filipecosta90 <filipecosta.90@gmail.com>","Redis Performance Group <performance@redis.com>"]
66
readme = "README.md"

redisbench_admin/run/metrics.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -110,7 +110,7 @@ def collect_redis_metrics(
110110
if section in section_filter:
111111
if k not in section_filter[section]:
112112
collect = False
113-
if collect and type(v) is float or type(v) is int:
113+
if collect and (type(v) is float or type(v) is int):
114114
if k not in overall[section]:
115115
overall[section][k] = 0
116116
overall[section][k] += v

tests/test_metrics.py

Lines changed: 60 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -89,30 +89,85 @@ def _info(section):
8989
return conn
9090

9191

92+
def test_collect_redis_metrics_section_filter_drops_scalar_keys():
93+
"""The section_filter is meant to restrict which INFO keys make it into
94+
the overall map. Prior to this test it was silently broken for scalar
95+
(int/float) values due to Python operator precedence on
96+
`if collect and type(v) is float or type(v) is int` -- the `or` binds
97+
last, so the `collect` flag was effectively ignored whenever v was an
98+
int, and filtered-out keys leaked through. Lock in the intended
99+
semantics: only keys listed in section_filter[section] end up in the
100+
flat overall dict; the rest are dropped."""
101+
conn = _mock_conn(
102+
{
103+
"section_a": {"keep_me": 10, "drop_me": 20, "also_drop": 30.5},
104+
"section_b": {"included_b": 100, "excluded_b": 200},
105+
}
106+
)
107+
_, _, overall = collect_redis_metrics(
108+
[conn],
109+
sections=["section_a", "section_b"],
110+
section_filter={
111+
"section_a": ["keep_me"],
112+
"section_b": ["included_b"],
113+
},
114+
)
115+
assert "section_a_keep_me" in overall
116+
assert overall["section_a_keep_me"] == 10
117+
assert "section_b_included_b" in overall
118+
assert overall["section_b_included_b"] == 100
119+
# The filtered-out keys must not appear in the overall dict.
120+
assert "section_a_drop_me" not in overall
121+
assert "section_a_also_drop" not in overall
122+
assert "section_b_excluded_b" not in overall
123+
124+
125+
def test_collect_redis_metrics_no_filter_keeps_all_scalars():
126+
"""When section_filter is None, every scalar INFO key is collected --
127+
this is the legacy default and the common code path. Regression check
128+
to ensure the operator-precedence fix didn't accidentally flip the
129+
default to 'filter everything out'."""
130+
conn = _mock_conn({"section_a": {"alpha": 1, "beta": 2, "gamma": 3}})
131+
_, _, overall = collect_redis_metrics([conn], sections=["section_a"])
132+
assert overall["section_a_alpha"] == 1
133+
assert overall["section_a_beta"] == 2
134+
assert overall["section_a_gamma"] == 3
135+
136+
92137
def test_collect_search_and_bigredis_metrics_flat_keys():
93138
"""All three targeted sections present on the shard -> single flat dict
94139
keyed by `<section>_<metric>`. At minimum the four documented keys
95140
(bigredis used_ram/used_disk + search_memory/search_disk counters) must
96-
appear with the values reported by INFO."""
141+
appear with the values reported by INFO.
142+
143+
After the section_filter fix (0.12.28), only the whitelisted keys pass
144+
through, so the output is exactly the 4 documented keys -- not every
145+
scalar in the section."""
97146
conn = _mock_conn(
98147
{
99148
"bigredis": {
100149
"used_ram": 1024,
101150
"used_disk": 2048,
151+
# This extra scalar must now be dropped by section_filter.
152+
"some_other_metric": 99,
102153
},
103154
"search_memory": {
104155
"search_used_memory_indexes": 512,
156+
# Filtered out.
157+
"search_internal_thing": 7,
105158
},
106159
"search_disk": {
107160
"search_disk_usage": 4096,
108161
},
109162
}
110163
)
111164
out = collect_search_and_bigredis_metrics([conn])
112-
assert out["bigredis_used_ram"] == 1024
113-
assert out["bigredis_used_disk"] == 2048
114-
assert out["search_memory_search_used_memory_indexes"] == 512
115-
assert out["search_disk_search_disk_usage"] == 4096
165+
assert out == {
166+
"bigredis_used_ram": 1024,
167+
"bigredis_used_disk": 2048,
168+
"search_memory_search_used_memory_indexes": 512,
169+
"search_disk_search_disk_usage": 4096,
170+
}
116171

117172

118173
def test_collect_search_and_bigredis_metrics_missing_sections():

0 commit comments

Comments
 (0)