mirror of
https://github.com/SigNoz/signoz.git
synced 2026-09-09 04:50:40 +01:00
fix(ruletypes): make list sort deterministic on ties
Ties on the primary sort key kept arbitrary DB order, so rows could shuffle between page requests causing overlaps or misses. Break ties on name (case-insensitive) then id, always ascending, with the requested order applied to the primary key only. Pin the tiebreak in unit tests and paginate over state ties in the integration suite.
This commit is contained in:
@@ -95,14 +95,21 @@ var severityDisplayRank = map[string]int{
|
||||
|
||||
// SortListableRules sorts in place. Severity ranks the well-known values and
|
||||
// falls back to a lexical compare between custom ones; name compares
|
||||
// case-insensitively.
|
||||
// case-insensitively. Ties break on name then id (always ascending, so pages
|
||||
// stay stable across requests) with order applied to the primary key only.
|
||||
func SortListableRules(rules []*ListableRule, sortBy ListSort, order ListOrder) {
|
||||
direction := 1
|
||||
if order == ListOrderDesc {
|
||||
direction = -1
|
||||
}
|
||||
slices.SortStableFunc(rules, func(a, b *ListableRule) int {
|
||||
return direction * compareListableRules(a, b, sortBy)
|
||||
if c := direction * compareListableRules(a, b, sortBy); c != 0 {
|
||||
return c
|
||||
}
|
||||
if c := strings.Compare(strings.ToLower(a.AlertName), strings.ToLower(b.AlertName)); c != 0 {
|
||||
return c
|
||||
}
|
||||
return strings.Compare(a.Id, b.Id)
|
||||
})
|
||||
}
|
||||
|
||||
|
||||
@@ -89,6 +89,30 @@ func TestSortListableRules(t *testing.T) {
|
||||
order: ListOrderDesc,
|
||||
wantNames: []string{"new", "old"},
|
||||
},
|
||||
{
|
||||
name: "state desc ties break on name asc",
|
||||
rules: []*ListableRule{
|
||||
listableRule("banana", StateFiring, "", base),
|
||||
listableRule("zebra", StateDisabled, "", base),
|
||||
listableRule("Apple", StateFiring, "", base),
|
||||
listableRule("cherry", StateFiring, "", base),
|
||||
},
|
||||
sortBy: ListSortState,
|
||||
order: ListOrderDesc,
|
||||
wantNames: []string{"Apple", "banana", "cherry", "zebra"},
|
||||
},
|
||||
{
|
||||
name: "state asc flips buckets but tiebreak stays name asc",
|
||||
rules: []*ListableRule{
|
||||
listableRule("banana", StateFiring, "", base),
|
||||
listableRule("zebra", StateDisabled, "", base),
|
||||
listableRule("Apple", StateFiring, "", base),
|
||||
listableRule("cherry", StateFiring, "", base),
|
||||
},
|
||||
sortBy: ListSortState,
|
||||
order: ListOrderAsc,
|
||||
wantNames: []string{"zebra", "Apple", "banana", "cherry"},
|
||||
},
|
||||
}
|
||||
|
||||
for _, tc := range testCases {
|
||||
@@ -99,6 +123,23 @@ func TestSortListableRules(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
func TestSortListableRulesIdTiebreak(t *testing.T) {
|
||||
base := time.Date(2026, 9, 1, 0, 0, 0, 0, time.UTC)
|
||||
|
||||
for _, order := range []ListOrder{ListOrderAsc, ListOrderDesc} {
|
||||
t.Run(order.StringValue(), func(t *testing.T) {
|
||||
older := listableRule("dup", StateFiring, "", base)
|
||||
older.Id = "01aaa"
|
||||
newer := listableRule("dup", StateFiring, "", base)
|
||||
newer.Id = "01bbb"
|
||||
|
||||
rules := []*ListableRule{newer, older}
|
||||
SortListableRules(rules, ListSortState, order)
|
||||
assert.Equal(t, []string{"01aaa", "01bbb"}, []string{rules[0].Id, rules[1].Id})
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
func TestNewLabelPairsFromRawJSON(t *testing.T) {
|
||||
pairs := NewLabelPairsFromRawJSON([]string{
|
||||
`{"team":"infra","severity":"critical"}`,
|
||||
|
||||
@@ -368,7 +368,8 @@ def test_sorting(
|
||||
"prom uptime probe",
|
||||
]
|
||||
|
||||
# state display priority: inactive (rank 1) outranks disabled (rank 0)
|
||||
# state display priority: inactive (rank 1) outranks disabled (rank 0);
|
||||
# the four inactive rules tie on state and must break on name asc
|
||||
response = requests.get(
|
||||
signoz.self.host_configs["8080"].get(BASE_URL),
|
||||
params={"sort": "state", "order": "desc"},
|
||||
@@ -376,9 +377,15 @@ def test_sorting(
|
||||
timeout=5,
|
||||
)
|
||||
assert response.status_code == HTTPStatus.OK
|
||||
names = [rule["alert"] for rule in response.json()["data"]["rules"]]
|
||||
assert names[-1] == "checkout conversion drop", "disabled rule must sort last on state desc"
|
||||
assert [rule["alert"] for rule in response.json()["data"]["rules"]] == [
|
||||
"infra cpu saturation",
|
||||
"payment gateway errors",
|
||||
"payment latency high",
|
||||
"prom uptime probe",
|
||||
"checkout conversion drop",
|
||||
]
|
||||
|
||||
# asc flips the state buckets but the name tiebreak stays ascending
|
||||
response = requests.get(
|
||||
signoz.self.host_configs["8080"].get(BASE_URL),
|
||||
params={"sort": "state", "order": "asc"},
|
||||
@@ -386,10 +393,16 @@ def test_sorting(
|
||||
timeout=5,
|
||||
)
|
||||
assert response.status_code == HTTPStatus.OK
|
||||
assert response.json()["data"]["rules"][0]["alert"] == "checkout conversion drop"
|
||||
assert [rule["alert"] for rule in response.json()["data"]["rules"]] == [
|
||||
"checkout conversion drop",
|
||||
"infra cpu saturation",
|
||||
"payment gateway errors",
|
||||
"payment latency high",
|
||||
"prom uptime probe",
|
||||
]
|
||||
|
||||
# severity: known ranks first (critical > warning), then custom values
|
||||
# lexically, then rules without severity
|
||||
# lexically, then rules without severity tie and break on name asc
|
||||
response = requests.get(
|
||||
signoz.self.host_configs["8080"].get(BASE_URL),
|
||||
params={"sort": "severity", "order": "desc"},
|
||||
@@ -397,9 +410,13 @@ def test_sorting(
|
||||
timeout=5,
|
||||
)
|
||||
assert response.status_code == HTTPStatus.OK
|
||||
names = [rule["alert"] for rule in response.json()["data"]["rules"]]
|
||||
assert names[:3] == ["payment latency high", "payment gateway errors", "checkout conversion drop"]
|
||||
assert set(names[3:]) == {"infra cpu saturation", "prom uptime probe"}
|
||||
assert [rule["alert"] for rule in response.json()["data"]["rules"]] == [
|
||||
"payment latency high",
|
||||
"payment gateway errors",
|
||||
"checkout conversion drop",
|
||||
"infra cpu saturation",
|
||||
"prom uptime probe",
|
||||
]
|
||||
|
||||
for order in ("asc", "desc"):
|
||||
response = requests.get(
|
||||
@@ -443,6 +460,27 @@ def test_pagination(
|
||||
assert len(flattened) == len(set(flattened)), "pages must be disjoint"
|
||||
assert set(flattened) == {r["alert"] for r in SEED_RULES}
|
||||
|
||||
# state sort is almost all ties (four inactive rules); the name/id tiebreak
|
||||
# must keep the pages disjoint and in the same order on every request
|
||||
tie_pages = []
|
||||
for offset in (0, 2, 4):
|
||||
response = requests.get(
|
||||
signoz.self.host_configs["8080"].get(BASE_URL),
|
||||
params={"sort": "state", "order": "desc", "limit": 2, "offset": offset},
|
||||
headers={"Authorization": f"Bearer {token}"},
|
||||
timeout=5,
|
||||
)
|
||||
assert response.status_code == HTTPStatus.OK
|
||||
tie_pages.append([rule["alert"] for rule in response.json()["data"]["rules"]])
|
||||
|
||||
assert [name for page in tie_pages for name in page] == [
|
||||
"infra cpu saturation",
|
||||
"payment gateway errors",
|
||||
"payment latency high",
|
||||
"prom uptime probe",
|
||||
"checkout conversion drop",
|
||||
], "tied rows must not shuffle between page requests"
|
||||
|
||||
# a past-the-end offset returns an empty page but keeps the real total
|
||||
response = requests.get(
|
||||
signoz.self.host_configs["8080"].get(BASE_URL),
|
||||
|
||||
Reference in New Issue
Block a user