From 172e75102f3c8a2b0b9dd9652653750cec76c5d9 Mon Sep 17 00:00:00 2001 From: raviks789 <33730024+raviks789@users.noreply.github.com> Date: Wed, 23 Aug 2023 14:54:15 +0200 Subject: [PATCH 1/3] Do not evaluate invalid performance data --- library/Icingadb/Util/PerfData.php | 97 +++++++++++++++---- library/Icingadb/Util/PerfDataFormat.php | 3 +- library/Icingadb/Util/ThresholdRange.php | 33 +++++++ .../Icingadb/Widget/Detail/PerfDataTable.php | 19 +++- public/css/widget/performance-data-table.less | 5 + 5 files changed, 134 insertions(+), 23 deletions(-) diff --git a/library/Icingadb/Util/PerfData.php b/library/Icingadb/Util/PerfData.php index 89cdad32..cc33d165 100644 --- a/library/Icingadb/Util/PerfData.php +++ b/library/Icingadb/Util/PerfData.php @@ -39,21 +39,21 @@ class PerfData /** * The value * - * @var float + * @var ?float */ protected $value; /** * The minimum value * - * @var float + * @var ?float */ protected $minValue; /** * The maximum value * - * @var float + * @var ?float */ protected $maxValue; @@ -71,6 +71,27 @@ class PerfData */ protected $criticalThreshold; + /** + * The raw value + * + * @var ?string + */ + protected $rawValue; + + /** + * The raw minimum value + * + * @var ?string + */ + protected $rawMinValue; + + /** + * The raw maximum value + * + * @var ?string + */ + protected $rawMaxValue; + /** * Create a new PerfData object based on the given performance data label and value * @@ -312,7 +333,7 @@ class PerfData */ public function isVisualizable(): bool { - return isset($this->minValue) && isset($this->maxValue) && isset($this->value); + return isset($this->minValue, $this->maxValue, $this->value) && $this->isValid(); } /** @@ -428,25 +449,39 @@ class PerfData $matches = array(); if (preg_match('@^(U|-?(?:\d+)?(?:\.\d+)?)([a-zA-TV-Z%°]{1,3})$@u', $parts[0], $matches)) { $this->unit = $matches[2]; - $this->value = $matches[1]; + $value = $matches[1]; } else { - $this->value = $parts[0]; + $value = $parts[0]; } - if (! is_numeric($this->value)) { + if (! is_numeric($value)) { + if ($value !== 'U') { + $this->rawValue = $parts[0]; + } + $this->value = null; + } else { + $this->value = floatval($value); } switch (count($parts)) { /* @noinspection PhpMissingBreakStatementInspection */ case 5: if ($parts[4] !== '') { - $this->maxValue = $parts[4]; + if (is_numeric($parts[4])) { + $this->maxValue = floatval($parts[4]); + } else { + $this->rawMaxValue = $parts[4]; + } } /* @noinspection PhpMissingBreakStatementInspection */ case 4: if ($parts[3] !== '') { - $this->minValue = $parts[3]; + if (is_numeric($parts[3])) { + $this->minValue = floatval($parts[3]); + } else { + $this->rawMinValue = $parts[3]; + } } /* @noinspection PhpMissingBreakStatementInspection */ case 3: @@ -509,6 +544,10 @@ class PerfData } if ($value instanceof ThresholdRange) { + if (! $value->isValid()) { + return (string) $value; + } + if ($value->getMin()) { return (string) $value; } @@ -578,18 +617,22 @@ class PerfData public function toArray(): array { - return array( + return [ 'label' => $this->getLabel(), - 'value' => $this->format($this->getValue()), - 'min' => isset($this->minValue) && !$this->isPercentage() - ? $this->format($this->minValue) - : '', - 'max' => isset($this->maxValue) && !$this->isPercentage() - ? $this->format($this->maxValue) - : '', - 'warn' => $this->format($this->warningThreshold), - 'crit' => $this->format($this->criticalThreshold) - ); + 'value' => isset($this->value) ? $this->format($this->value) : $this->rawValue, + 'min' => (string) ( + ! $this->isPercentage() + ? (isset($this->minValue) ? $this->format($this->minValue) : $this->rawMinValue) + : null + ), + 'max' => (string) ( + ! $this->isPercentage() + ? (isset($this->maxValue) ? $this->format($this->maxValue) : $this->rawMaxValue) + : null + ), + 'warn' => $this->format($this->warningThreshold), + 'crit' => $this->format($this->criticalThreshold) + ]; } /** @@ -643,4 +686,18 @@ class PerfData return false; } + + /** + * Returns whether the performance data can be evaluated + * + * @return bool + */ + public function isValid(): bool + { + return ! isset($this->rawValue) + && ! isset($this->rawMinValue) + && ! isset($this->rawMaxValue) + && $this->criticalThreshold->isValid() + && $this->warningThreshold->isValid(); + } } diff --git a/library/Icingadb/Util/PerfDataFormat.php b/library/Icingadb/Util/PerfDataFormat.php index 8c33ae5c..1caffff0 100644 --- a/library/Icingadb/Util/PerfDataFormat.php +++ b/library/Icingadb/Util/PerfDataFormat.php @@ -131,10 +131,9 @@ class PerfDataFormat return sprintf('%0.2f d', $value / 86400); } - protected static function formatForUnits($value, array &$units, int $base): string + protected static function formatForUnits(float $value, array &$units, int $base): string { $sign = ''; - $value = (float) $value; if ($value < 0) { $value = abs($value); $sign = '-'; diff --git a/library/Icingadb/Util/ThresholdRange.php b/library/Icingadb/Util/ThresholdRange.php index fefed75b..675697a5 100644 --- a/library/Icingadb/Util/ThresholdRange.php +++ b/library/Icingadb/Util/ThresholdRange.php @@ -37,6 +37,13 @@ class ThresholdRange */ protected $raw; + /** + * Whether the threshold range is valid + * + * @var bool + */ + protected $isValid = true; + /** * Create a new instance based on a threshold range conforming to * @@ -61,6 +68,12 @@ class ThresholdRange if (strpos($rawRange, ':') === false) { $min = 0.0; + $max = trim($rawRange); + if (! is_numeric($max)) { + $range->isValid = false; + return $range; + } + $max = floatval(trim($rawRange)); } else { list($min, $max) = explode(':', $rawRange, 2); @@ -75,9 +88,19 @@ class ThresholdRange $min = null; break; default: + if (! is_numeric($min)) { + $range->isValid = false; + return $range; + } + $min = floatval($min); } + if (! empty($max) && ! is_numeric($max)) { + $range->isValid = false; + return $range; + } + $max = empty($max) ? null : floatval($max); } @@ -168,6 +191,16 @@ class ThresholdRange )); } + /** + * Return whether the threshold range is valid + * + * @return bool + */ + public function isValid() + { + return $this->isValid; + } + /** * Return the textual representation of $this, suitable for fromString() * diff --git a/library/Icingadb/Widget/Detail/PerfDataTable.php b/library/Icingadb/Widget/Detail/PerfDataTable.php index a4417a87..957c9cac 100644 --- a/library/Icingadb/Widget/Detail/PerfDataTable.php +++ b/library/Icingadb/Widget/Detail/PerfDataTable.php @@ -12,9 +12,13 @@ use ipl\Html\HtmlElement; use ipl\Html\HtmlString; use ipl\Html\Table; use ipl\Html\Text; +use ipl\I18n\Translation; +use ipl\Web\Widget\Icon; class PerfDataTable extends Table { + use Translation; + protected $defaultAttributes = [ 'class' => 'performance-data-table collapsible', 'data-visible-rows' => 6 @@ -58,7 +62,7 @@ class PerfDataTable extends Table $containsSparkline = false; foreach ($pieChartData as $perfdata) { - if ($perfdata->isVisualizable()) { + if ($perfdata->isVisualizable() || ! $perfdata->isValid()) { $containsSparkline = true; break; } @@ -91,6 +95,19 @@ class PerfDataTable extends Table HtmlString::create($perfdata->asInlinePie($this->color)->render()), ['class' => 'sparkline-col'] ); + } elseif (! $perfdata->isValid()) { + $cols[] = Table::td( + new Icon( + 'triangle-exclamation', + [ + 'title' => $this->translate( + 'Evaluation failed. Performance data is invalid.' + ), + 'class' => ['invalid-perfdata'] + ] + ), + ['class' => 'sparkline-col'] + ); } else { $cols[] = Table::td(''); } diff --git a/public/css/widget/performance-data-table.less b/public/css/widget/performance-data-table.less index a1a7b6ee..26c18c83 100644 --- a/public/css/widget/performance-data-table.less +++ b/public/css/widget/performance-data-table.less @@ -43,6 +43,11 @@ min-width: 1.75em; width: 1.75em; padding: 2/12em 0; + .invalid-perfdata { + font-size: 1.25em; + vertical-align: text-bottom; + color: @color-warning; + } } .inline-pie > svg { From 81cf29884c7d023731beac7440fe459985e504f3 Mon Sep 17 00:00:00 2001 From: raviks789 <33730024+raviks789@users.noreply.github.com> Date: Wed, 30 Aug 2023 17:22:17 +0200 Subject: [PATCH 2/3] Add unit tests for invalid performance data evaluation --- .../library/Icingadb/Util/PerfdataSetTest.php | 2 +- .../library/Icingadb/Util/PerfdataTest.php | 209 +++++++++++++++++- .../Icingadb/Util/ThresholdRangeTest.php | 20 ++ 3 files changed, 220 insertions(+), 11 deletions(-) diff --git a/test/php/library/Icingadb/Util/PerfdataSetTest.php b/test/php/library/Icingadb/Util/PerfdataSetTest.php index 4449ab35..618c29af 100644 --- a/test/php/library/Icingadb/Util/PerfdataSetTest.php +++ b/test/php/library/Icingadb/Util/PerfdataSetTest.php @@ -93,7 +93,7 @@ class PerfdataSetTest extends TestCase 'PerfdataSet does not correctly parse invalid quoted labels' ); $this->assertSame( - '2', + 2.0, $pset->perfdata[1]->getValue() ); } diff --git a/test/php/library/Icingadb/Util/PerfdataTest.php b/test/php/library/Icingadb/Util/PerfdataTest.php index e31c86d0..5a63825b 100644 --- a/test/php/library/Icingadb/Util/PerfdataTest.php +++ b/test/php/library/Icingadb/Util/PerfdataTest.php @@ -32,7 +32,7 @@ class PerfdataTest extends TestCase 'Perfdata::fromString does not properly parse performance data labels' ); $this->assertSame( - '1234', + 1234.0, $p->getValue(), 'Perfdata::fromString does not properly parse performance data values' ); @@ -44,12 +44,12 @@ class PerfdataTest extends TestCase public function testWhetherGetValueReturnsValidValues() { $this->assertSame( - '1337', + 1337.0, Perfdata::fromString('test=1337')->getValue(), 'Perfdata::getValue does not return correct values' ); $this->assertSame( - '1337', + 1337.0, Perfdata::fromString('test=1337;;;;')->getValue(), 'Perfdata::getValue does not return correct values' ); @@ -61,12 +61,12 @@ class PerfdataTest extends TestCase public function testWhetherDecimalValuesAreCorrectlyParsed() { $this->assertSame( - '1337.5', + 1337.5, Perfdata::fromString('test=1337.5')->getValue(), 'Perfdata objects do not parse decimal values correctly' ); $this->assertSame( - '1337.5', + 1337.5, Perfdata::fromString('test=1337.5B')->getValue(), 'Perfdata objects do not parse decimal values correctly' ); @@ -250,12 +250,12 @@ class PerfdataTest extends TestCase public function testWhetherGetMinimumValueReturnsCorrectValues() { $this->assertSame( - '1337', + 1337.0, Perfdata::fromString('test=1;;;1337')->getMinimumValue(), 'Perfdata::getMinimumValue does not return correct values' ); $this->assertSame( - '1337.5', + 1337.5, Perfdata::fromString('test=1;;;1337.5')->getMinimumValue(), 'Perfdata::getMinimumValue does not return correct values' ); @@ -267,12 +267,12 @@ class PerfdataTest extends TestCase public function testWhetherGetMaximumValueReturnsCorrectValues() { $this->assertSame( - '1337', + 1337.0, Perfdata::fromString('test=1;;;;1337')->getMaximumValue(), 'Perfdata::getMaximumValue does not return correct values' ); $this->assertSame( - '1337.5', + 1337.5, Perfdata::fromString('test=1;;;;1337.5')->getMaximumValue(), 'Perfdata::getMaximumValue does not return correct values' ); @@ -373,7 +373,7 @@ class PerfdataTest extends TestCase public function testWhetherPercentagesAreHandledCorrectly() { $this->assertSame( - '66', + 66.0, Perfdata::fromString('test=66%')->getPercentage(), 'Perfdata objects do not correctly handle native percentages' ); @@ -399,4 +399,193 @@ class PerfdataTest extends TestCase 'Perfdata objects do not ignore impossible min/max combinations when returning percentages' ); } + + public function testWhetherInvalidValueInPerfDataHandledCorrectly() + { + $p1 = Perfdata::fromString('test=2,0'); + $this->assertFalse($p1->isValid()); + $this->assertNull( + $p1->getValue(), + 'Perfdata::getValue does not return null for invalid values' + ); + $this->assertSame( + '2,0', + $p1->toArray()['value'] + ); + + $p2 = Perfdata::fromString('test=i am not a value'); + $this->assertFalse($p2->isValid()); + $this->assertNull( + $p2->getValue(), + 'Perfdata::getValue does not return null for invalid values' + ); + $this->assertSame( + 'i am not a value', + $p2->toArray()['value'] + ); + + $p3 = Perfdata::fromString('test='); + $this->assertFalse($p3->isValid()); + $this->assertNull( + $p3->getValue(), + 'Perfdata::getValue does not return null for invalid values' + ); + $this->assertSame( + '', + $p3->toArray()['value'] + ); + + $p4 = Perfdata::fromString('test=-kW'); + $this->assertFalse($p4->isValid()); + $this->assertNull( + $p4->getValue(), + 'Perfdata::getValue does not return null for invalid values' + ); + $this->assertSame( + '-kW', + $p4->toArray()['value'] + ); + + $p5 = Perfdata::fromString('test=kW'); + $this->assertFalse($p5->isValid()); + $this->assertNull( + $p5->getValue(), + 'Perfdata::getValue does not return null for invalid values' + ); + $this->assertSame( + 'kW', + $p5->toArray()['value'] + ); + + $p6 = Perfdata::fromString('test=-'); + $this->assertFalse($p6->isValid()); + $this->assertNull( + $p6->getValue(), + 'Perfdata::getValue does not return null for invalid values' + ); + $this->assertSame( + '-', + $p6->toArray()['value'] + ); + } + + public function testWhetherInvalidMinInPerfDataHandledCorrectly() + { + $p1 = Perfdata::fromString('test=1;;;2,0'); + $this->assertFalse($p1->isValid()); + $this->assertNull( + $p1->getMinimumValue(), + 'Perfdata::getMinimumValue does not return null for invalid min values' + ); + $this->assertSame( + '2,0', + $p1->toArray()['min'] + ); + + $p2 = Perfdata::fromString('test=1;;;foo'); + $this->assertFalse($p2->isValid()); + $this->assertNull( + $p2->getMinimumValue(), + 'Perfdata::getMinimumValue does not return null for invalid min values' + ); + $this->assertSame( + 'foo', + $p2->toArray()['min'] + ); + } + + public function testWhetherInvalidMaxInPerfDataHandledCorrectly() + { + $p1 = Perfdata::fromString('test=1;;;;2,0'); + $this->assertFalse($p1->isValid()); + $this->assertNull( + $p1->getMaximumValue(), + 'Perfdata::getMaximumValue does not return null for invalid max values' + ); + $this->assertSame( + '2,0', + $p1->toArray()['max'] + ); + + $p2 = Perfdata::fromString('test=1;;;;foo'); + $this->assertFalse($p2->isValid()); + $this->assertNull( + $p2->getMaximumValue(), + 'Perfdata::getMaximumValue does not return null for invalid max values' + ); + $this->assertSame( + 'foo', + $p2->toArray()['max'] + ); + } + + public function testWhetherInvalidWarningThresholdInPerfDataHandledCorrectly() + { + $p1 = Perfdata::fromString('test=1;2,0:'); + $this->assertFalse($p1->getWarningThreshold()->isValid()); + $this->assertFalse($p1->isValid()); + $this->assertSame( + '2,0:', + (string) $p1->getWarningThreshold() + ); + + $p2 = Perfdata::fromString('test=1;0:4,0'); + $this->assertFalse($p2->getWarningThreshold()->isValid()); + $this->assertFalse($p2->isValid()); + $this->assertSame( + '0:4,0', + (string) $p2->getWarningThreshold() + ); + + $p3 = Perfdata::fromString('test=1;foo'); + $this->assertFalse($p2->getWarningThreshold()->isValid()); + $this->assertFalse($p3->isValid()); + $this->assertSame( + 'foo', + (string) $p3->getWarningThreshold() + ); + + $p4 = Perfdata::fromString('test=1;10@'); + $this->assertFalse($p2->getWarningThreshold()->isValid()); + $this->assertFalse($p4->isValid()); + $this->assertSame( + '10@', + (string) $p4->getWarningThreshold() + ); + } + + public function testWhetherInvalidCriticalThresholdInPerfDataHandledCorrectly() + { + $p1 = Perfdata::fromString('test=1;;2,0:'); + $this->assertFalse($p1->getCriticalThreshold()->isValid()); + $this->assertFalse($p1->isValid()); + $this->assertSame( + '2,0:', + (string) $p1->getCriticalThreshold() + ); + + $p2 = Perfdata::fromString('test=1;;0:4,0'); + $this->assertFalse($p2->getCriticalThreshold()->isValid()); + $this->assertFalse($p2->isValid()); + $this->assertSame( + '0:4,0', + (string) $p2->getCriticalThreshold() + ); + + $p3 = Perfdata::fromString('test=1;;foo'); + $this->assertFalse($p2->getCriticalThreshold()->isValid()); + $this->assertFalse($p3->isValid()); + $this->assertSame( + 'foo', + (string) $p3->getCriticalThreshold() + ); + + $p4 = Perfdata::fromString('test=1;;10@'); + $this->assertFalse($p2->getCriticalThreshold()->isValid()); + $this->assertFalse($p4->isValid()); + $this->assertSame( + '10@', + (string) $p4->getCriticalThreshold() + ); + } } diff --git a/test/php/library/Icingadb/Util/ThresholdRangeTest.php b/test/php/library/Icingadb/Util/ThresholdRangeTest.php index c0d75737..b191e880 100644 --- a/test/php/library/Icingadb/Util/ThresholdRangeTest.php +++ b/test/php/library/Icingadb/Util/ThresholdRangeTest.php @@ -319,5 +319,25 @@ class ThresholdRangeTest extends TestCase 'foo', (string) ThresholdRange::fromString('foo') ); + $this->assertSame( + '4,4:2,2', + (string) ThresholdRange::fromString('4,4:2,2') + ); + } + + public function testInvalidThresholdNotationsConsideredInValid() + { + $this->assertFalse( + ThresholdRange::fromString('10@')->isValid(), + 'Invalid threshold notation 10@ considered as valid' + ); + $this->assertFalse( + ThresholdRange::fromString('foo')->isValid(), + 'Invalid threshold notation foo considered as valid' + ); + $this->assertFalse( + ThresholdRange::fromString('4,4:2,2')->isValid(), + 'Invalid threshold notation 4,4:2,2 considered as valid' + ); } } From 3031a09629778a13800bf00e559fa23e30bbed1a Mon Sep 17 00:00:00 2001 From: raviks789 <33730024+raviks789@users.noreply.github.com> Date: Tue, 5 Sep 2023 15:53:39 +0200 Subject: [PATCH 3/3] PHPStan Baseline: Remove fixed phpstan errors --- phpstan-baseline.neon | 45 ------------------------------------------- 1 file changed, 45 deletions(-) diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon index 54694f95..f0d8b02c 100644 --- a/phpstan-baseline.neon +++ b/phpstan-baseline.neon @@ -5840,46 +5840,6 @@ parameters: count: 2 path: library/Icingadb/Util/PerfData.php - - - message: "#^Property Icinga\\\\Module\\\\Icingadb\\\\Util\\\\PerfData\\:\\:\\$maxValue \\(float\\) does not accept string\\.$#" - count: 1 - path: library/Icingadb/Util/PerfData.php - - - - message: "#^Property Icinga\\\\Module\\\\Icingadb\\\\Util\\\\PerfData\\:\\:\\$maxValue \\(float\\) in isset\\(\\) is not nullable\\.$#" - count: 2 - path: library/Icingadb/Util/PerfData.php - - - - message: "#^Property Icinga\\\\Module\\\\Icingadb\\\\Util\\\\PerfData\\:\\:\\$minValue \\(float\\) does not accept string\\.$#" - count: 1 - path: library/Icingadb/Util/PerfData.php - - - - message: "#^Property Icinga\\\\Module\\\\Icingadb\\\\Util\\\\PerfData\\:\\:\\$minValue \\(float\\) in isset\\(\\) is not nullable\\.$#" - count: 2 - path: library/Icingadb/Util/PerfData.php - - - - message: "#^Property Icinga\\\\Module\\\\Icingadb\\\\Util\\\\PerfData\\:\\:\\$value \\(float\\) does not accept null\\.$#" - count: 1 - path: library/Icingadb/Util/PerfData.php - - - - message: "#^Property Icinga\\\\Module\\\\Icingadb\\\\Util\\\\PerfData\\:\\:\\$value \\(float\\) does not accept string\\.$#" - count: 2 - path: library/Icingadb/Util/PerfData.php - - - - message: "#^Property Icinga\\\\Module\\\\Icingadb\\\\Util\\\\PerfData\\:\\:\\$value \\(float\\) in isset\\(\\) is not nullable\\.$#" - count: 1 - path: library/Icingadb/Util/PerfData.php - - - - message: "#^Result of && is always true\\.$#" - count: 2 - path: library/Icingadb/Util/PerfData.php - - message: "#^Method Icinga\\\\Module\\\\Icingadb\\\\Util\\\\PerfDataFormat\\:\\:ampereSeconds\\(\\) has parameter \\$value with no type specified\\.$#" count: 1 @@ -5905,11 +5865,6 @@ parameters: count: 1 path: library/Icingadb/Util/PerfDataFormat.php - - - message: "#^Method Icinga\\\\Module\\\\Icingadb\\\\Util\\\\PerfDataFormat\\:\\:formatForUnits\\(\\) has parameter \\$value with no type specified\\.$#" - count: 1 - path: library/Icingadb/Util/PerfDataFormat.php - - message: "#^Method Icinga\\\\Module\\\\Icingadb\\\\Util\\\\PerfDataFormat\\:\\:grams\\(\\) has parameter \\$value with no type specified\\.$#" count: 1