Skip to content

Commit b85ac41

Browse files
Tjitse-ETjitse-E
authored
Merge pull request #20 from Vendic/bugfix/PIPE-444-caching-mechanism-rework
Fixed fail->warning transition causing unintended value switch
2 parents 5c3db6c + 0d07a3a commit b85ac41

15 files changed

+395
-281
lines changed

‎.github/workflows/integration.yml‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,9 @@ jobs:
77
matrix:
88
include:
99
- PHP_VERSION: php83-fpm
10-
MAGENTO_VERSION: 2.4.7
10+
MAGENTO_VERSION: 2.4.7-p6
11+
- PHP_VERSION: php83-fpm
12+
MAGENTO_VERSION: 2.4.8-p1
1113
runs-on: ubuntu-latest
1214
steps:
1315
- uses: actions/checkout@v4

‎.github/workflows/phpcs.yml‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,9 @@ jobs:
77
matrix:
88
include:
99
- PHP_VERSION: php83-fpm
10-
MAGENTO_VERSION: 2.4.7
10+
MAGENTO_VERSION: 2.4.7-p6
11+
- PHP_VERSION: php83-fpm
12+
MAGENTO_VERSION: 2.4.8-p1
1113
runs-on: ubuntu-latest
1214
steps:
1315
- uses: actions/checkout@v4

‎.github/workflows/phpstan.yml‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,9 @@ jobs:
77
matrix:
88
include:
99
- PHP_VERSION: php83-fpm
10-
MAGENTO_VERSION: 2.4.7
10+
MAGENTO_VERSION: 2.4.7-p6
11+
- PHP_VERSION: php83-fpm
12+
MAGENTO_VERSION: 2.4.8-p1
1113
runs-on: ubuntu-latest
1214
steps:
1315
- uses: actions/checkout@v4

‎.github/workflows/setup-di-compile.yml‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,9 @@ jobs:
77
matrix:
88
include:
99
- PHP_VERSION: php83-fpm
10-
MAGENTO_VERSION: 2.4.7
10+
MAGENTO_VERSION: 2.4.7-p6
11+
- PHP_VERSION: php83-fpm
12+
MAGENTO_VERSION: 2.4.8-p1
1113
runs-on: ubuntu-latest
1214
steps:
1315
- uses: actions/checkout@v4

‎Model/CachedStatusResolver.php‎

Lines changed: 25 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -75,17 +75,28 @@ public function updateCacheCheck(
7575
$checkResult->getLabel()
7676
)
7777
);
78-
$this->cacheService->removeCheckData($checkResult->getName());
78+
$this->cacheService->saveCheckData($checkResult->getName(), $status->value, (string)$this->getTime());
7979

8080
return $checkResult;
8181
}
8282

8383
// Status changed, reset time for selected status
8484
if (($checkCache['status'] ?? null) !== $status->value) {
8585
$cachedStatus = $checkCache['status'] ?? CheckStatus::STATUS_OK->value;
86-
$this->cacheService->updateCheckData($checkResult->getName(), $status->value, (string)time());
86+
$cachedFallbackStatus = $checkCache['fallback_status'] ?? CheckStatus::STATUS_OK->value;
87+
$persistFallback = $this->exceedsThreshold((int)($checkCache['fallback_data']['data'] ?? 0));
88+
89+
$this->cacheService->saveCheckData(
90+
$checkResult->getName(),
91+
$status->value,
92+
(string)$this->getTime(),
93+
$persistFallback
94+
);
8795

88-
$checkResult->setStatus($this->getFlappingStatus($cachedStatus, $status->value));
96+
$checkResult->setStatus($this->getFlappingStatus(
97+
$persistFallback ? $cachedFallbackStatus : $cachedStatus,
98+
$status->value
99+
));
89100
$checkResult->setShortSummary(
90101
sprintf($this->getMessagesByStatus()[self::STATUS_CHANGE]['summary'], $checkResult->getLabel())
91102
);
@@ -102,7 +113,7 @@ public function updateCacheCheck(
102113
}
103114

104115
// Status does not exceed the threshold, keep as it is
105-
if ((int)$checkCache['data'] > (time() - $this->getStatusTimeThreshold() * 60)) {
116+
if ($this->exceedsThreshold((int)$checkCache['data'])) {
106117
$checkResult->setStatus($this->getFlappingStatus(
107118
$checkCache['fallback_status'],
108119
$status->value
@@ -173,6 +184,11 @@ public function getFlappingStatus(string $oldStatus, string $newStatus): CheckSt
173184
return constant("\Vendic\OhDear\Api\Data\CheckStatus::{$statusToSet}");
174185
}
175186

187+
private function exceedsThreshold(int $time): bool
188+
{
189+
return $time > ($this->getTime() - $this->getStatusTimeThreshold() * 60);
190+
}
191+
176192
private function getStatusTimeThreshold(): int
177193
{
178194
return $this->statusTimeThreshold;
@@ -183,4 +199,9 @@ public function setStatusTimeThreshold(int $statusTimeThreshold): self
183199
$this->statusTimeThreshold = $statusTimeThreshold;
184200
return $this;
185201
}
202+
203+
public function getTime(): int
204+
{
205+
return time();
206+
}
186207
}

‎Service/CacheService.php‎

Lines changed: 50 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -6,89 +6,99 @@
66

77
declare(strict_types=1);
88

9+
// phpcs:disable PSR2.ControlStructures.ControlStructureSpacing.SpacingAfterOpenBrace
10+
911
namespace Vendic\OhDear\Service;
1012

1113
use Magento\Framework\App\CacheInterface;
14+
use Magento\Framework\Serialize\Serializer\Json;
1215
use Vendic\OhDear\Api\Data\CheckStatus;
1316

1417
class CacheService
1518
{
1619
private const OD_CACHE_PREFIX = 'oh_dear';
1720

18-
private const DATA_SEPARATOR = '==>>';
19-
2021
public function __construct(
2122
private readonly CacheInterface $cache,
23+
private readonly Json $serializer
2224
) {
2325
}
2426

2527
/**
2628
* @return array{
2729
* status: string,
2830
* fallback_status: string,
29-
* data: string
31+
* data: string|array,
32+
* fallback_data: array
3033
* }|null
3134
*/
3235
public function getDataForCheck(string $checkKey): ?array
3336
{
34-
$result = [
35-
'status' => null,
36-
'data' => null
37-
];
38-
3937
$identifier = self::OD_CACHE_PREFIX . '_' . $checkKey;
40-
$fallbackRecordIdentifier = self::OD_CACHE_PREFIX . '_fallback_' . $checkKey;
4138

4239
if (!$cacheData = $this->cache->load($identifier)) {
4340
return null;
4441
}
4542

46-
$fallbackData = $this->cache->load($fallbackRecordIdentifier) ?: null;
47-
48-
[$severity, $data] = explode(self::DATA_SEPARATOR, $cacheData, 2);
49-
50-
$fallbackSeverity = null;
51-
if ($fallbackData) {
52-
[$fallbackSeverity] = explode(self::DATA_SEPARATOR, $fallbackData);
53-
}
54-
55-
$result['data'] = $data;
56-
$result['status'] = $severity;
57-
$result['fallback_status'] = $fallbackSeverity ?? CheckStatus::STATUS_OK->value;
43+
$result = $this->serializer->unserialize($cacheData);
44+
$result['fallback_status'] = $result['fallback_data']['status'] ?? CheckStatus::STATUS_OK->value;
5845

5946
return $result['data'] ? $result : null;
6047
}
6148

62-
public function removeCheckData(string $checkKey, bool $removeFallback = false): bool
49+
public function removeCheckData(string $checkKey, bool $removeFallback = true): bool
6350
{
64-
$result = false;
6551
$identifier = self::OD_CACHE_PREFIX . '_' . $checkKey;
66-
$fallbackIdentifier = self::OD_CACHE_PREFIX . '_fallback_' . $checkKey;
6752

68-
if (($cachedData = $this->cache->load($identifier)) && !$removeFallback) {
69-
$this->cache->save($cachedData, $fallbackIdentifier);
70-
}
71-
72-
if ($removeFallback) {
73-
$this->cache->remove($fallbackIdentifier);
53+
if (
54+
($cacheRecord = $this->cache->load($identifier))
55+
&& ($cachedData = $this->serializer->unserialize($cacheRecord))
56+
&& isset($cachedData['fallback_data'])
57+
&& !$removeFallback
58+
) {
59+
return $this->cache->save(
60+
$this->serializer->serialize($cachedData['fallback_data']),
61+
$identifier
62+
);
7463
}
7564

7665
return $this->cache->remove($identifier);
7766
}
7867

79-
public function saveCheckData(string $checkKey, string $status, string $data): bool
80-
{
68+
public function saveCheckData(
69+
string $checkKey,
70+
string $status,
71+
string|array $data,
72+
bool $persistFallback = false
73+
): bool {
8174
$identifier = self::OD_CACHE_PREFIX . '_' . $checkKey;
82-
$fallbackRecordIdentifier = self::OD_CACHE_PREFIX . '_fallback_' . $checkKey;
8375

84-
if ($currentData = $this->cache->load($identifier)) {
85-
$this->cache->save($currentData, $fallbackRecordIdentifier);
76+
$payload = [
77+
"status" => $status,
78+
"data" => $data,
79+
];
80+
81+
try {
82+
if (
83+
($currentData = $this->cache->load($identifier))
84+
&& $currentData = $this->serializer->unserialize($currentData)
85+
) {
86+
$payload['fallback_data'] = $currentData;
87+
}
88+
89+
if (
90+
isset($currentData['fallback_data'])
91+
&& $persistFallback
92+
) {
93+
$payload['fallback_data'] = $currentData['fallback_data'];
94+
}
95+
96+
unset($payload['fallback_data']['fallback_data']);
97+
// phpcs:ignore
98+
} catch (\Exception $exception) {
99+
// Literally do nothing, that simply means fallback data cannot be extracted
86100
}
87-
return $this->cache->save($status . self::DATA_SEPARATOR . $data, $identifier);
88-
}
89101

90-
public function updateCheckData(string $checkKey, string $status, string $data): bool
91-
{
92-
return $this->saveCheckData($checkKey, $status, $data);
102+
return $this->cache->save($this->serializer->serialize($payload), $identifier);
93103
}
94104
}

‎Test/Integration/Checks/CpuLoadTest.php‎

Lines changed: 59 additions & 63 deletions
Original file line numberDiff line numberDiff line change
@@ -21,43 +21,67 @@
2121
class CpuLoadTest extends TestCase
2222
{
2323

24-
/**
25-
* @dataProvider cpuLoadDataProvider
26-
*/
27-
public function testCpuLoadsAndCheckStatus(
28-
float $loadLastMinute,
29-
float $loadLastFiveMinutes,
30-
float $loadLastFifteenMinutes,
31-
CheckStatus $expectedStatus,
32-
string $message = ''
33-
): void {
24+
public function testCpuLoadsAndCheckStatus(): void
25+
{
3426
/** @var ObjectManager $objectManager */
3527
$objectManager = Bootstrap::getObjectManager();
36-
$cpuLoadUtilsMock = $this->getCpuLoadUtilsMock(
37-
$loadLastMinute,
38-
$loadLastFiveMinutes,
39-
$loadLastFifteenMinutes
40-
);
41-
42-
$objectManager->addSharedInstance($cpuLoadUtilsMock, CpuLoadUtils::class);
28+
29+
// Test data for consecutive calls
30+
$testCases = [
31+
['loads' => [1, 1, 1], 'expectedStatus' => CheckStatus::STATUS_OK, 'message' => 'All loads are below 1: OK'],
32+
['loads' => [20, 1, 1], 'expectedStatus' => CheckStatus::STATUS_WARNING, 'message' => 'Load last minute is 20: warning'],
33+
['loads' => [1, 20, 1], 'expectedStatus' => CheckStatus::STATUS_WARNING, 'message' => 'Load last five minutes is 20: warning'],
34+
['loads' => [1, 1, 20], 'expectedStatus' => CheckStatus::STATUS_WARNING, 'message' => 'Load last fifteen minutes is 20: warning'],
35+
['loads' => [20, 20, 20], 'expectedStatus' => CheckStatus::STATUS_WARNING, 'message' => 'All loads are above 20: warning'],
36+
];
37+
38+
// Create five separate CpuLoadResults mocks - one for each test case
39+
/** @var CpuLoadResults & MockObject $result1Mock */
40+
$result1Mock = $this->createMock(CpuLoadResults::class);
41+
$result1Mock->method('getLoadLastMinute')->willReturn(1);
42+
$result1Mock->method('getLoadLastFiveMinutes')->willReturn(1);
43+
$result1Mock->method('getLoadLastFifteenMinutes')->willReturn(1);
44+
45+
/** @var CpuLoadResults & MockObject $result2Mock */
46+
$result2Mock = $this->createMock(CpuLoadResults::class);
47+
$result2Mock->method('getLoadLastMinute')->willReturn(20);
48+
$result2Mock->method('getLoadLastFiveMinutes')->willReturn(1);
49+
$result2Mock->method('getLoadLastFifteenMinutes')->willReturn(1);
50+
51+
/** @var CpuLoadResults & MockObject $result3Mock */
52+
$result3Mock = $this->createMock(CpuLoadResults::class);
53+
$result3Mock->method('getLoadLastMinute')->willReturn(1);
54+
$result3Mock->method('getLoadLastFiveMinutes')->willReturn(20);
55+
$result3Mock->method('getLoadLastFifteenMinutes')->willReturn(1);
56+
57+
/** @var CpuLoadResults & MockObject $result4Mock */
58+
$result4Mock = $this->createMock(CpuLoadResults::class);
59+
$result4Mock->method('getLoadLastMinute')->willReturn(1);
60+
$result4Mock->method('getLoadLastFiveMinutes')->willReturn(1);
61+
$result4Mock->method('getLoadLastFifteenMinutes')->willReturn(20);
62+
63+
/** @var CpuLoadResults & MockObject $result5Mock */
64+
$result5Mock = $this->createMock(CpuLoadResults::class);
65+
$result5Mock->method('getLoadLastMinute')->willReturn(20);
66+
$result5Mock->method('getLoadLastFiveMinutes')->willReturn(20);
67+
$result5Mock->method('getLoadLastFifteenMinutes')->willReturn(20);
68+
69+
/** @var CpuLoadUtils & MockObject $cpuLoadUtilsMock */
70+
$cpuLoadUtilsMock = $this->createMock(CpuLoadUtils::class);
71+
$cpuLoadUtilsMock->method('measure')
72+
->willReturnOnConsecutiveCalls($result1Mock, $result2Mock, $result3Mock, $result4Mock, $result5Mock);
4373

44-
/** @var CpuLoad $cpuLoadCheck */
45-
$cpuLoadCheck = $objectManager->get(CpuLoad::class);
46-
$checkResult = $cpuLoadCheck->run();
74+
$objectManager->addSharedInstance($cpuLoadUtilsMock, CpuLoadUtils::class, true);
4775

48-
$this->assertEquals('cpu_load', $checkResult->getName());
49-
$this->assertEquals($expectedStatus, $checkResult->getStatus(), $message);
50-
}
76+
// Run each test case
77+
foreach ($testCases as $index => $testCase) {
78+
/** @var CpuLoad $cpuLoadCheck */
79+
$cpuLoadCheck = $objectManager->create(CpuLoad::class);
80+
$checkResult = $cpuLoadCheck->run();
5181

52-
public function cpuLoadDataProvider(): array
53-
{
54-
return [
55-
[1, 1, 1, CheckStatus::STATUS_OK, 'All loads are below 1: OK'],
56-
[20, 1, 1, CheckStatus::STATUS_WARNING, 'Load last minute is 20: warning '],
57-
[1, 20, 1, CheckStatus::STATUS_WARNING, 'Load last five minutes is 20: warning'],
58-
[1, 1, 20, CheckStatus::STATUS_WARNING, 'Load last fifteen minutes is 20: warning'],
59-
[20, 20, 20, CheckStatus::STATUS_WARNING, 'All loads are above 20: warning'],
60-
];
82+
$this->assertEquals('cpu_load', $checkResult->getName(), "Test case {$index}: {$testCase['message']}");
83+
$this->assertEquals($testCase['expectedStatus'], $checkResult->getStatus(), "Test case {$index}: {$testCase['message']}");
84+
}
6185
}
6286

6387
public function testCannotGetCpuLoad(): void
@@ -66,47 +90,19 @@ public function testCannotGetCpuLoad(): void
6690
$objectManager = Bootstrap::getObjectManager();
6791

6892
/** @var CpuLoadUtils & MockObject $cpuLoadUtilsMock */
69-
$cpuLoadUtilsMock = $this->getMockBuilder(CpuLoadUtils::class)
70-
->disableOriginalConstructor()
71-
->onlyMethods(['measure'])
72-
->getMock();
93+
$cpuLoadUtilsMock = $this->createMock(CpuLoadUtils::class);
7394
$cpuLoadUtilsMock->method('measure')
7495
->willThrowException(new LocalizedException(__('Cannot get CPU load')));
7596

76-
$objectManager->addSharedInstance($cpuLoadUtilsMock, CpuLoadUtils::class);
97+
$objectManager->addSharedInstance($cpuLoadUtilsMock, CpuLoadUtils::class, true);
7798

7899
/** @var CpuLoad $cpuLoadCheck */
79-
$cpuLoadCheck = $objectManager->get(CpuLoad::class);
100+
$cpuLoadCheck = $objectManager->create(CpuLoad::class);
80101
$checkResult = $cpuLoadCheck->run();
81102

82103
$this->assertEquals('cpu_load', $checkResult->getName());
83104
$this->assertEquals('Could not get CPU load', $checkResult->getShortSummary());
84105
$this->assertEquals(CheckStatus::STATUS_CRASHED, $checkResult->getStatus());
85106
}
86107

87-
/**
88-
* @return CpuLoadUtils&MockObject
89-
*/
90-
private function getCpuLoadUtilsMock(
91-
float $loadLastMinute,
92-
float $loadLastFiveMinutes,
93-
float $loadLastFifteenMinutes
94-
): CpuLoadUtils&MockObject {
95-
/** @var CpuLoadResults & MockObject $cpuLoadResultMock */
96-
$cpuLoadResultMock = $this->getMockBuilder(\Vendic\OhDear\Utils\CpuLoad\CpuLoadResults::class)
97-
->disableOriginalConstructor()
98-
->onlyMethods(['getLoadLastMinute', 'getLoadLastFiveMinutes', 'getLoadLastFifteenMinutes'])
99-
->getMock();
100-
$cpuLoadResultMock->method('getLoadLastMinute')->willReturn($loadLastMinute);
101-
$cpuLoadResultMock->method('getLoadLastFiveMinutes')->willReturn($loadLastFiveMinutes);
102-
$cpuLoadResultMock->method('getLoadLastFifteenMinutes')->willReturn($loadLastFifteenMinutes);
103-
104-
/** @var CpuLoadUtils & MockObject $cpuLoadUtilsMock */
105-
$cpuLoadUtilsMock = $this->getMockBuilder(CpuLoadUtils::class)
106-
->disableOriginalConstructor()
107-
->onlyMethods(['measure'])
108-
->getMock();
109-
$cpuLoadUtilsMock->method('measure')->willReturn($cpuLoadResultMock);
110-
return $cpuLoadUtilsMock;
111-
}
112108
}

0 commit comments

Comments
 (0)