Skip to content

Commit 0c281c9

Browse files
fix: aggregate 95th percentile uses SUM instead of MAX for SIMILAR (#6843)
* fix: use aggregate_current for SIMILAR total type in percentile calc AGGREGATE_TOTAL_TYPE_SIMILAR was grouped with ALL in three locations, causing SIMILAR aggregate graphs to use aggregate_sum / aggregate_sum_peak (which sums inbound + outbound together) instead of aggregate_current / aggregate_current_peak (which computes percentiles per data source name). This made 95th percentile HRULEs on aggregate graphs display the sum of in+out instead of max(in, out), roughly doubling the reported value. Split each conditional into separate ALL and SIMILAR branches: - ALL: aggregate_sum / aggregate_sum_peak (sum all data sources) - SIMILAR: aggregate_current / aggregate_current_peak (per-source) Three locations in lib/api_aggregate.php: - text_format substitution in aggregate_graphs_insert_graph_items() - pparts replacement for COMMENT items in aggregate_handle_ptile_type() - pparts replacement for HRULE items in aggregate_handle_ptile_type() Fixes #6835 Ref #6470 Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * fix: add else branch logging and integration tests for ptile fix - Add cacti_log debug traces for unhandled total_type values - Add 18 integration tests covering full COMMENT/HRULE pparts pipeline and text_format replacement for SIMILAR vs ALL total types Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * chore: baseline pre-existing PHPStan errors Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * docs: add sync warning to integration test helpers The test helpers replicate production logic from api_aggregate.php because the production functions require DB/session state. Add a comment documenting this dependency and the lines that must stay in sync. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * chore: remove duplicate phpstan baseline (already in #6842) Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * fix: replace undefined cacti_redirect with header+exit in oauth2.php Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * revert: remove oauth2.php fix (moved to standalone PR #6886) Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> --------- Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> Co-authored-by: TheWitness <thewitness@cacti.net>
1 parent c869d9b commit 0c281c9

File tree

2 files changed

+246
-17
lines changed

2 files changed

+246
-17
lines changed

lib/api_aggregate.php

Lines changed: 17 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -409,20 +409,20 @@ function aggregate_graphs_insert_graph_items(int $_new_graph_id, int $_old_graph
409409
$prepend = false;
410410
$prepend_cnt++;
411411
} elseif (str_contains($save['text_format'], ':current:')) {
412-
if ($_total_type == AGGREGATE_TOTAL_TYPE_ALL || $_total_type == AGGREGATE_TOTAL_TYPE_SIMILAR) {
413-
// All so use sum functions
412+
if ($_total_type == AGGREGATE_TOTAL_TYPE_ALL) {
414413
$save['text_format'] = str_replace(':current:', ':aggregate_sum:', $save['text_format']);
414+
} elseif ($_total_type == AGGREGATE_TOTAL_TYPE_SIMILAR) {
415+
$save['text_format'] = str_replace(':current:', ':aggregate_current:', $save['text_format']);
415416
} else {
416-
// Similar to separate
417-
$save['text_format'] = str_replace(':current:', ':current:', $save['text_format']);
417+
cacti_log(__FUNCTION__ . ' unhandled total_type ' . $_total_type . ' for :current: in text_format', true, 'AGGREGATE', POLLER_VERBOSITY_DEBUG);
418418
}
419419
} elseif (str_contains($save['text_format'], ':max:')) {
420-
if ($_total_type == AGGREGATE_TOTAL_TYPE_ALL || $_total_type == AGGREGATE_TOTAL_TYPE_SIMILAR) {
421-
// All so use sum functions
422-
$save['text_format'] = str_replace(':max:', ':aggregate_sum:', $save['text_format']);
420+
if ($_total_type == AGGREGATE_TOTAL_TYPE_ALL) {
421+
$save['text_format'] = str_replace(':max:', ':aggregate_sum_peak:', $save['text_format']);
422+
} elseif ($_total_type == AGGREGATE_TOTAL_TYPE_SIMILAR) {
423+
$save['text_format'] = str_replace(':max:', ':aggregate_current_peak:', $save['text_format']);
423424
} else {
424-
// Similar to separate
425-
$save['text_format'] = str_replace(':max:', ':max:', $save['text_format']);
425+
cacti_log(__FUNCTION__ . ' unhandled total_type ' . $_total_type . ' for :max: in text_format', true, 'AGGREGATE', POLLER_VERBOSITY_DEBUG);
426426
}
427427
}
428428
}
@@ -1428,14 +1428,14 @@ function aggregate_handle_ptile_type(array $member_graphs, array $skipped_items,
14281428
$pparts = explode(':', $parts[1]);
14291429

14301430
if (isset($pparts[3])) {
1431-
if ($_total_type == AGGREGATE_TOTAL_TYPE_ALL || $_total_type == AGGREGATE_TOTAL_TYPE_SIMILAR) {
1432-
// All so use sum functions
1431+
if ($_total_type == AGGREGATE_TOTAL_TYPE_ALL) {
14331432
$pparts[3] = str_replace('current', 'aggregate_sum', $pparts[3]);
14341433
$pparts[3] = str_replace('max', 'aggregate_sum_peak', $pparts[3]);
1435-
} else {
1436-
// Similar to separate
1434+
} elseif ($_total_type == AGGREGATE_TOTAL_TYPE_SIMILAR) {
14371435
$pparts[3] = str_replace('current', 'aggregate_current', $pparts[3]);
14381436
$pparts[3] = str_replace('max', 'aggregate_current_peak', $pparts[3]);
1437+
} else {
1438+
cacti_log(__FUNCTION__ . ' unhandled total_type ' . $_total_type . ' for pparts[3] in text_format', true, 'AGGREGATE', POLLER_VERBOSITY_DEBUG);
14391439
}
14401440

14411441
switch($pparts[3]) {
@@ -1510,14 +1510,14 @@ function aggregate_handle_ptile_type(array $member_graphs, array $skipped_items,
15101510
$pparts = explode(':', $parts[1]);
15111511

15121512
if (isset($pparts[3])) {
1513-
if ($_total_type == AGGREGATE_TOTAL_TYPE_ALL || $_total_type == AGGREGATE_TOTAL_TYPE_SIMILAR) {
1514-
// All so use sum functions
1513+
if ($_total_type == AGGREGATE_TOTAL_TYPE_ALL) {
15151514
$pparts[3] = str_replace('current', 'aggregate_sum', $pparts[3]);
15161515
$pparts[3] = str_replace('max', 'aggregate_sum_peak', $pparts[3]);
1517-
} else {
1518-
// Similar to separate
1516+
} elseif ($_total_type == AGGREGATE_TOTAL_TYPE_SIMILAR) {
15191517
$pparts[3] = str_replace('current', 'aggregate_current', $pparts[3]);
15201518
$pparts[3] = str_replace('max', 'aggregate_current_peak', $pparts[3]);
1519+
} else {
1520+
cacti_log(__FUNCTION__ . ' unhandled total_type ' . $_total_type . ' for pparts[3] in value', true, 'AGGREGATE', POLLER_VERBOSITY_DEBUG);
15211521
}
15221522

15231523
switch($pparts[3]) {
Lines changed: 229 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,229 @@
1+
<?php
2+
/*
3+
+-------------------------------------------------------------------------+
4+
| Copyright (C) 2004-2026 The Cacti Group |
5+
| |
6+
| This program is free software; you can redistribute it and/or |
7+
| modify it under the terms of the GNU General Public License |
8+
| as published by the Free Software Foundation; either version 2 |
9+
| of the License, or (at your option) any later version. |
10+
+-------------------------------------------------------------------------+
11+
| Cacti: The Complete RRDtool-based Graphing Solution |
12+
+-------------------------------------------------------------------------+
13+
*/
14+
15+
/*
16+
* Integration tests for aggregate 95th percentile format string replacement.
17+
*
18+
* These tests simulate the full COMMENT and HRULE processing pipeline in
19+
* aggregate_handle_ptile_type(): explode the pipe-delimited string, replace
20+
* pparts[3], and rejoin. This verifies that the fix produces correct
21+
* rrdtool variable references for SIMILAR vs ALL total types.
22+
*
23+
* The bug: SIMILAR was grouped with ALL, causing aggregate graphs to use
24+
* aggregate_sum (sum of in+out) instead of aggregate_current (per-source)
25+
* for 95th percentile, roughly doubling the displayed value.
26+
*
27+
* See: https://github.com/Cacti/cacti/issues/6835
28+
*
29+
* SYNC WARNING: process_ptile_pparts() and process_text_format() below
30+
* replicate logic from lib/api_aggregate.php (lines ~412-427, ~1430-1439,
31+
* ~1512-1521). Production functions require DB/session state that prevents
32+
* direct invocation in unit tests. If the production replacement logic
33+
* changes, these helpers must be updated to match.
34+
*/
35+
36+
require_once dirname(__DIR__) . '/Helpers/CactiStubs.php';
37+
require_once dirname(__DIR__, 2) . '/include/global.php';
38+
39+
/**
40+
* Simulates the full COMMENT/HRULE pparts processing pipeline from
41+
* aggregate_handle_ptile_type(). Takes a pipe-delimited text_format or
42+
* value string, applies the total_type replacement on pparts[3], and
43+
* returns the reassembled string.
44+
*/
45+
function process_ptile_pparts(string $value, int $total_type): string {
46+
$parts = explode('|', $value);
47+
48+
if (!isset($parts[1])) {
49+
return $value;
50+
}
51+
52+
$pparts = explode(':', $parts[1]);
53+
54+
if (!isset($pparts[3])) {
55+
return $value;
56+
}
57+
58+
if ($total_type == AGGREGATE_TOTAL_TYPE_ALL) {
59+
$pparts[3] = str_replace('current', 'aggregate_sum', $pparts[3]);
60+
$pparts[3] = str_replace('max', 'aggregate_sum_peak', $pparts[3]);
61+
} elseif ($total_type == AGGREGATE_TOTAL_TYPE_SIMILAR) {
62+
$pparts[3] = str_replace('current', 'aggregate_current', $pparts[3]);
63+
$pparts[3] = str_replace('max', 'aggregate_current_peak', $pparts[3]);
64+
}
65+
66+
$parts[1] = implode(':', $pparts);
67+
68+
return implode('|', $parts);
69+
}
70+
71+
/**
72+
* Simulates the full text_format replacement pipeline from
73+
* aggregate_graphs_insert_graph_items(). Handles :current: and :max:
74+
* substitutions based on total_type.
75+
*/
76+
function process_text_format(string $text_format, int $total_type): string {
77+
if (str_contains($text_format, ':current:')) {
78+
if ($total_type == AGGREGATE_TOTAL_TYPE_ALL) {
79+
$text_format = str_replace(':current:', ':aggregate_sum:', $text_format);
80+
} elseif ($total_type == AGGREGATE_TOTAL_TYPE_SIMILAR) {
81+
$text_format = str_replace(':current:', ':aggregate_current:', $text_format);
82+
}
83+
} elseif (str_contains($text_format, ':max:')) {
84+
if ($total_type == AGGREGATE_TOTAL_TYPE_ALL) {
85+
$text_format = str_replace(':max:', ':aggregate_sum_peak:', $text_format);
86+
} elseif ($total_type == AGGREGATE_TOTAL_TYPE_SIMILAR) {
87+
$text_format = str_replace(':max:', ':aggregate_current_peak:', $text_format);
88+
}
89+
}
90+
91+
return $text_format;
92+
}
93+
94+
// --- COMMENT pipeline: SIMILAR produces per-source percentile ---
95+
96+
test('COMMENT pipeline: SIMILAR current produces aggregate_current', function () {
97+
$input = '95th Percentile|nth_percentile:95:traffic_in:current';
98+
$result = process_ptile_pparts($input, AGGREGATE_TOTAL_TYPE_SIMILAR);
99+
100+
expect($result)->toBe('95th Percentile|nth_percentile:95:traffic_in:aggregate_current');
101+
});
102+
103+
test('COMMENT pipeline: SIMILAR max produces aggregate_current_peak', function () {
104+
$input = '95th Percentile|nth_percentile:95:traffic_in:max';
105+
$result = process_ptile_pparts($input, AGGREGATE_TOTAL_TYPE_SIMILAR);
106+
107+
expect($result)->toBe('95th Percentile|nth_percentile:95:traffic_in:aggregate_current_peak');
108+
});
109+
110+
test('COMMENT pipeline: ALL current produces aggregate_sum', function () {
111+
$input = '95th Percentile|nth_percentile:95:traffic_in:current';
112+
$result = process_ptile_pparts($input, AGGREGATE_TOTAL_TYPE_ALL);
113+
114+
expect($result)->toBe('95th Percentile|nth_percentile:95:traffic_in:aggregate_sum');
115+
});
116+
117+
test('COMMENT pipeline: ALL max produces aggregate_sum_peak', function () {
118+
$input = '95th Percentile|nth_percentile:95:traffic_in:max';
119+
$result = process_ptile_pparts($input, AGGREGATE_TOTAL_TYPE_ALL);
120+
121+
expect($result)->toBe('95th Percentile|nth_percentile:95:traffic_in:aggregate_sum_peak');
122+
});
123+
124+
// --- HRULE pipeline: same replacement pattern ---
125+
126+
test('HRULE pipeline: SIMILAR current produces aggregate_current', function () {
127+
$input = '#FF5722|nth_percentile:95:traffic_out:current';
128+
$result = process_ptile_pparts($input, AGGREGATE_TOTAL_TYPE_SIMILAR);
129+
130+
expect($result)->toBe('#FF5722|nth_percentile:95:traffic_out:aggregate_current');
131+
});
132+
133+
test('HRULE pipeline: SIMILAR max produces aggregate_current_peak', function () {
134+
$input = '#FF5722|nth_percentile:95:traffic_out:max';
135+
$result = process_ptile_pparts($input, AGGREGATE_TOTAL_TYPE_SIMILAR);
136+
137+
expect($result)->toBe('#FF5722|nth_percentile:95:traffic_out:aggregate_current_peak');
138+
});
139+
140+
test('HRULE pipeline: ALL max produces aggregate_sum_peak', function () {
141+
$input = '#FF5722|nth_percentile:95:traffic_out:max';
142+
$result = process_ptile_pparts($input, AGGREGATE_TOTAL_TYPE_ALL);
143+
144+
expect($result)->toBe('#FF5722|nth_percentile:95:traffic_out:aggregate_sum_peak');
145+
});
146+
147+
// --- SIMILAR must never produce summed values ---
148+
149+
test('SIMILAR COMMENT never produces aggregate_sum for current', function () {
150+
$input = '95th Percentile|nth_percentile:95:traffic_in:current';
151+
$result = process_ptile_pparts($input, AGGREGATE_TOTAL_TYPE_SIMILAR);
152+
153+
expect($result)->not->toContain('aggregate_sum:')
154+
->and($result)->not->toContain(':aggregate_sum');
155+
});
156+
157+
test('SIMILAR HRULE never produces aggregate_sum_peak for max', function () {
158+
$input = '#FF5722|nth_percentile:95:traffic_in:max';
159+
$result = process_ptile_pparts($input, AGGREGATE_TOTAL_TYPE_SIMILAR);
160+
161+
expect($result)->not->toContain('aggregate_sum_peak');
162+
});
163+
164+
// --- text_format pipeline: full string with ISP billing context ---
165+
166+
test('text_format: SIMILAR with :current: in bandwidth label', function () {
167+
$result = process_text_format('95th In :current: bps', AGGREGATE_TOTAL_TYPE_SIMILAR);
168+
169+
expect($result)->toBe('95th In :aggregate_current: bps');
170+
});
171+
172+
test('text_format: SIMILAR with :max: in bandwidth label', function () {
173+
$result = process_text_format('95th Peak :max: bps', AGGREGATE_TOTAL_TYPE_SIMILAR);
174+
175+
expect($result)->toBe('95th Peak :aggregate_current_peak: bps');
176+
});
177+
178+
test('text_format: ALL with :current: in bandwidth label', function () {
179+
$result = process_text_format('95th Total :current: bps', AGGREGATE_TOTAL_TYPE_ALL);
180+
181+
expect($result)->toBe('95th Total :aggregate_sum: bps');
182+
});
183+
184+
test('text_format: ALL with :max: produces aggregate_sum_peak not aggregate_sum', function () {
185+
// This is the secondary fix: ALL+max was using aggregate_sum instead of
186+
// aggregate_sum_peak, mismatching the COMMENT/HRULE paths
187+
$result = process_text_format('95th Peak :max: bps', AGGREGATE_TOTAL_TYPE_ALL);
188+
189+
expect($result)->toBe('95th Peak :aggregate_sum_peak: bps')
190+
->and($result)->not->toContain(':aggregate_sum:');
191+
});
192+
193+
// --- Edge cases ---
194+
195+
test('pparts with no pipe delimiter returns unchanged', function () {
196+
$result = process_ptile_pparts('no pipes here', AGGREGATE_TOTAL_TYPE_SIMILAR);
197+
198+
expect($result)->toBe('no pipes here');
199+
});
200+
201+
test('pparts with fewer than 4 colon parts returns unchanged', function () {
202+
$result = process_ptile_pparts('label|nth_percentile:95:traffic_in', AGGREGATE_TOTAL_TYPE_SIMILAR);
203+
204+
expect($result)->toBe('label|nth_percentile:95:traffic_in');
205+
});
206+
207+
test('unknown total type leaves pparts unchanged', function () {
208+
$input = '95th Percentile|nth_percentile:95:traffic_in:current';
209+
$result = process_ptile_pparts($input, 99);
210+
211+
expect($result)->toBe($input);
212+
});
213+
214+
test('unknown total type leaves text_format unchanged', function () {
215+
$result = process_text_format('95th :current: bps', 99);
216+
217+
expect($result)->toBe('95th :current: bps');
218+
});
219+
220+
// --- Inbound vs Outbound: verify both data sources handled independently ---
221+
222+
test('SIMILAR handles traffic_in and traffic_out independently', function () {
223+
$in_result = process_ptile_pparts('In|nth_percentile:95:traffic_in:current', AGGREGATE_TOTAL_TYPE_SIMILAR);
224+
$out_result = process_ptile_pparts('Out|nth_percentile:95:traffic_out:current', AGGREGATE_TOTAL_TYPE_SIMILAR);
225+
226+
expect($in_result)->toBe('In|nth_percentile:95:traffic_in:aggregate_current')
227+
->and($out_result)->toBe('Out|nth_percentile:95:traffic_out:aggregate_current')
228+
->and($in_result)->not->toBe($out_result);
229+
});

0 commit comments

Comments
 (0)