Skip to content

Commit 37ae61a

Browse files
somethingwithproofsomethingwithproof
committed
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>
1 parent f1b3825 commit 37ae61a

File tree

2 files changed

+229
-2
lines changed

2 files changed

+229
-2
lines changed

‎lib/api_aggregate.php‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -414,15 +414,15 @@ function aggregate_graphs_insert_graph_items(int $_new_graph_id, int $_old_graph
414414
} elseif ($_total_type == AGGREGATE_TOTAL_TYPE_SIMILAR) {
415415
$save['text_format'] = str_replace(':current:', ':aggregate_current:', $save['text_format']);
416416
} else {
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:')) {
420420
if ($_total_type == AGGREGATE_TOTAL_TYPE_ALL) {
421421
$save['text_format'] = str_replace(':max:', ':aggregate_sum_peak:', $save['text_format']);
422422
} elseif ($_total_type == AGGREGATE_TOTAL_TYPE_SIMILAR) {
423423
$save['text_format'] = str_replace(':max:', ':aggregate_current_peak:', $save['text_format']);
424424
} else {
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
}
@@ -1434,6 +1434,8 @@ function aggregate_handle_ptile_type(array $member_graphs, array $skipped_items,
14341434
} elseif ($_total_type == AGGREGATE_TOTAL_TYPE_SIMILAR) {
14351435
$pparts[3] = str_replace('current', 'aggregate_current', $pparts[3]);
14361436
$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);
14371439
}
14381440

14391441
switch($pparts[3]) {
@@ -1514,6 +1516,8 @@ function aggregate_handle_ptile_type(array $member_graphs, array $skipped_items,
15141516
} elseif ($_total_type == AGGREGATE_TOTAL_TYPE_SIMILAR) {
15151517
$pparts[3] = str_replace('current', 'aggregate_current', $pparts[3]);
15161518
$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);
15171521
}
15181522

15191523
switch($pparts[3]) {

‎tests/Unit/AggregatePtileFormatIntegrationTest.php‎

Lines changed: 223 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,223 @@
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+
30+
require_once dirname(__DIR__) . '/Helpers/CactiStubs.php';
31+
require_once dirname(__DIR__, 2) . '/include/global.php';
32+
33+
/**
34+
* Simulates the full COMMENT/HRULE pparts processing pipeline from
35+
* aggregate_handle_ptile_type(). Takes a pipe-delimited text_format or
36+
* value string, applies the total_type replacement on pparts[3], and
37+
* returns the reassembled string.
38+
*/
39+
function process_ptile_pparts(string $value, int $total_type): string {
40+
$parts = explode('|', $value);
41+
42+
if (!isset($parts[1])) {
43+
return $value;
44+
}
45+
46+
$pparts = explode(':', $parts[1]);
47+
48+
if (!isset($pparts[3])) {
49+
return $value;
50+
}
51+
52+
if ($total_type == AGGREGATE_TOTAL_TYPE_ALL) {
53+
$pparts[3] = str_replace('current', 'aggregate_sum', $pparts[3]);
54+
$pparts[3] = str_replace('max', 'aggregate_sum_peak', $pparts[3]);
55+
} elseif ($total_type == AGGREGATE_TOTAL_TYPE_SIMILAR) {
56+
$pparts[3] = str_replace('current', 'aggregate_current', $pparts[3]);
57+
$pparts[3] = str_replace('max', 'aggregate_current_peak', $pparts[3]);
58+
}
59+
60+
$parts[1] = implode(':', $pparts);
61+
62+
return implode('|', $parts);
63+
}
64+
65+
/**
66+
* Simulates the full text_format replacement pipeline from
67+
* aggregate_graphs_insert_graph_items(). Handles :current: and :max:
68+
* substitutions based on total_type.
69+
*/
70+
function process_text_format(string $text_format, int $total_type): string {
71+
if (str_contains($text_format, ':current:')) {
72+
if ($total_type == AGGREGATE_TOTAL_TYPE_ALL) {
73+
$text_format = str_replace(':current:', ':aggregate_sum:', $text_format);
74+
} elseif ($total_type == AGGREGATE_TOTAL_TYPE_SIMILAR) {
75+
$text_format = str_replace(':current:', ':aggregate_current:', $text_format);
76+
}
77+
} elseif (str_contains($text_format, ':max:')) {
78+
if ($total_type == AGGREGATE_TOTAL_TYPE_ALL) {
79+
$text_format = str_replace(':max:', ':aggregate_sum_peak:', $text_format);
80+
} elseif ($total_type == AGGREGATE_TOTAL_TYPE_SIMILAR) {
81+
$text_format = str_replace(':max:', ':aggregate_current_peak:', $text_format);
82+
}
83+
}
84+
85+
return $text_format;
86+
}
87+
88+
// --- COMMENT pipeline: SIMILAR produces per-source percentile ---
89+
90+
test('COMMENT pipeline: SIMILAR current produces aggregate_current', function () {
91+
$input = '95th Percentile|nth_percentile:95:traffic_in:current';
92+
$result = process_ptile_pparts($input, AGGREGATE_TOTAL_TYPE_SIMILAR);
93+
94+
expect($result)->toBe('95th Percentile|nth_percentile:95:traffic_in:aggregate_current');
95+
});
96+
97+
test('COMMENT pipeline: SIMILAR max produces aggregate_current_peak', function () {
98+
$input = '95th Percentile|nth_percentile:95:traffic_in:max';
99+
$result = process_ptile_pparts($input, AGGREGATE_TOTAL_TYPE_SIMILAR);
100+
101+
expect($result)->toBe('95th Percentile|nth_percentile:95:traffic_in:aggregate_current_peak');
102+
});
103+
104+
test('COMMENT pipeline: ALL current produces aggregate_sum', function () {
105+
$input = '95th Percentile|nth_percentile:95:traffic_in:current';
106+
$result = process_ptile_pparts($input, AGGREGATE_TOTAL_TYPE_ALL);
107+
108+
expect($result)->toBe('95th Percentile|nth_percentile:95:traffic_in:aggregate_sum');
109+
});
110+
111+
test('COMMENT pipeline: ALL max produces aggregate_sum_peak', function () {
112+
$input = '95th Percentile|nth_percentile:95:traffic_in:max';
113+
$result = process_ptile_pparts($input, AGGREGATE_TOTAL_TYPE_ALL);
114+
115+
expect($result)->toBe('95th Percentile|nth_percentile:95:traffic_in:aggregate_sum_peak');
116+
});
117+
118+
// --- HRULE pipeline: same replacement pattern ---
119+
120+
test('HRULE pipeline: SIMILAR current produces aggregate_current', function () {
121+
$input = '#FF5722|nth_percentile:95:traffic_out:current';
122+
$result = process_ptile_pparts($input, AGGREGATE_TOTAL_TYPE_SIMILAR);
123+
124+
expect($result)->toBe('#FF5722|nth_percentile:95:traffic_out:aggregate_current');
125+
});
126+
127+
test('HRULE pipeline: SIMILAR max produces aggregate_current_peak', function () {
128+
$input = '#FF5722|nth_percentile:95:traffic_out:max';
129+
$result = process_ptile_pparts($input, AGGREGATE_TOTAL_TYPE_SIMILAR);
130+
131+
expect($result)->toBe('#FF5722|nth_percentile:95:traffic_out:aggregate_current_peak');
132+
});
133+
134+
test('HRULE pipeline: ALL max produces aggregate_sum_peak', function () {
135+
$input = '#FF5722|nth_percentile:95:traffic_out:max';
136+
$result = process_ptile_pparts($input, AGGREGATE_TOTAL_TYPE_ALL);
137+
138+
expect($result)->toBe('#FF5722|nth_percentile:95:traffic_out:aggregate_sum_peak');
139+
});
140+
141+
// --- SIMILAR must never produce summed values ---
142+
143+
test('SIMILAR COMMENT never produces aggregate_sum for current', function () {
144+
$input = '95th Percentile|nth_percentile:95:traffic_in:current';
145+
$result = process_ptile_pparts($input, AGGREGATE_TOTAL_TYPE_SIMILAR);
146+
147+
expect($result)->not->toContain('aggregate_sum:')
148+
->and($result)->not->toContain(':aggregate_sum');
149+
});
150+
151+
test('SIMILAR HRULE never produces aggregate_sum_peak for max', function () {
152+
$input = '#FF5722|nth_percentile:95:traffic_in:max';
153+
$result = process_ptile_pparts($input, AGGREGATE_TOTAL_TYPE_SIMILAR);
154+
155+
expect($result)->not->toContain('aggregate_sum_peak');
156+
});
157+
158+
// --- text_format pipeline: full string with ISP billing context ---
159+
160+
test('text_format: SIMILAR with :current: in bandwidth label', function () {
161+
$result = process_text_format('95th In :current: bps', AGGREGATE_TOTAL_TYPE_SIMILAR);
162+
163+
expect($result)->toBe('95th In :aggregate_current: bps');
164+
});
165+
166+
test('text_format: SIMILAR with :max: in bandwidth label', function () {
167+
$result = process_text_format('95th Peak :max: bps', AGGREGATE_TOTAL_TYPE_SIMILAR);
168+
169+
expect($result)->toBe('95th Peak :aggregate_current_peak: bps');
170+
});
171+
172+
test('text_format: ALL with :current: in bandwidth label', function () {
173+
$result = process_text_format('95th Total :current: bps', AGGREGATE_TOTAL_TYPE_ALL);
174+
175+
expect($result)->toBe('95th Total :aggregate_sum: bps');
176+
});
177+
178+
test('text_format: ALL with :max: produces aggregate_sum_peak not aggregate_sum', function () {
179+
// This is the secondary fix: ALL+max was using aggregate_sum instead of
180+
// aggregate_sum_peak, mismatching the COMMENT/HRULE paths
181+
$result = process_text_format('95th Peak :max: bps', AGGREGATE_TOTAL_TYPE_ALL);
182+
183+
expect($result)->toBe('95th Peak :aggregate_sum_peak: bps')
184+
->and($result)->not->toContain(':aggregate_sum:');
185+
});
186+
187+
// --- Edge cases ---
188+
189+
test('pparts with no pipe delimiter returns unchanged', function () {
190+
$result = process_ptile_pparts('no pipes here', AGGREGATE_TOTAL_TYPE_SIMILAR);
191+
192+
expect($result)->toBe('no pipes here');
193+
});
194+
195+
test('pparts with fewer than 4 colon parts returns unchanged', function () {
196+
$result = process_ptile_pparts('label|nth_percentile:95:traffic_in', AGGREGATE_TOTAL_TYPE_SIMILAR);
197+
198+
expect($result)->toBe('label|nth_percentile:95:traffic_in');
199+
});
200+
201+
test('unknown total type leaves pparts unchanged', function () {
202+
$input = '95th Percentile|nth_percentile:95:traffic_in:current';
203+
$result = process_ptile_pparts($input, 99);
204+
205+
expect($result)->toBe($input);
206+
});
207+
208+
test('unknown total type leaves text_format unchanged', function () {
209+
$result = process_text_format('95th :current: bps', 99);
210+
211+
expect($result)->toBe('95th :current: bps');
212+
});
213+
214+
// --- Inbound vs Outbound: verify both data sources handled independently ---
215+
216+
test('SIMILAR handles traffic_in and traffic_out independently', function () {
217+
$in_result = process_ptile_pparts('In|nth_percentile:95:traffic_in:current', AGGREGATE_TOTAL_TYPE_SIMILAR);
218+
$out_result = process_ptile_pparts('Out|nth_percentile:95:traffic_out:current', AGGREGATE_TOTAL_TYPE_SIMILAR);
219+
220+
expect($in_result)->toBe('In|nth_percentile:95:traffic_in:aggregate_current')
221+
->and($out_result)->toBe('Out|nth_percentile:95:traffic_out:aggregate_current')
222+
->and($in_result)->not->toBe($out_result);
223+
});

0 commit comments

Comments
 (0)