Skip to content
19 changes: 19 additions & 0 deletions core/ArchiveProcessor/Loader.php
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,13 @@ class Loader
{
private static $archivingDepth = 0;

/**
* Tracks whether the current prepareArchive run reused an existing archive instead of processing.
*
* @var boolean
*/
private $didReuseArchive = false;

/**
* @var Parameters
*/
Expand Down Expand Up @@ -103,6 +110,11 @@ public function prepareArchive($pluginName)
return Context::changeIdSite($this->params->getSite()->getId(), function () use ($pluginName) {
try {
++self::$archivingDepth;

if (self::$archivingDepth === 1) {
$this->didReuseArchive = false;
}

return $this->prepareArchiveImpl($pluginName);
} finally {
--self::$archivingDepth;
Expand Down Expand Up @@ -135,6 +147,7 @@ private function prepareArchiveImpl($pluginName)
// load existing data from archive
$data = $this->loadArchiveData();
if (sizeof($data) == 2) {
$this->didReuseArchive = true;
return $data;
}
[$idArchives, $visits, $visitsConverted, $foundRecords] = $data;
Expand All @@ -153,6 +166,7 @@ private function prepareArchiveImpl($pluginName)
$data = $this->loadArchiveData();

if (sizeof($data) == 2) {
$this->didReuseArchive = true;
return $data;
}

Expand Down Expand Up @@ -542,6 +556,11 @@ public function canSkipThisArchiveWithReason(): array
];
}

public function didReuseArchive(): bool
{
return $this->didReuseArchive;
}

private function hasChildArchivesInPeriod($idSite, Period $period): bool
{
$cacheKey = CacheId::siteAware('Archiving.hasChildArchivesInPeriod.' . $period->getRangeString(), [$idSite]);
Expand Down
50 changes: 49 additions & 1 deletion plugins/ArchivingMetrics/ArchivingMetrics.php
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,55 @@

namespace Piwik\Plugins\ArchivingMetrics;

use Piwik\Period;
use Piwik\Segment;

class ArchivingMetrics extends \Piwik\Plugin
{
// Tracking is performed manually via Tracker from CoreAdminHome API; no events registered here.
public function registerEvents()
{
return [
'CoreAdminHome.archiveReports.start' => 'onArchiveReportsStart',
'CoreAdminHome.archiveReports.complete' => 'onArchiveReportsComplete',
];
}

public function onArchiveReportsStart(
int $idSite,
Period $period,
Segment $segment,
string $plugin,
bool $isArchivePhpTriggered,
?string $report = null
): void
{

Check failure on line 35 in plugins/ArchivingMetrics/ArchivingMetrics.php

View workflow job for this annotation

GitHub Actions / PHPCS

The closing parenthesis and the opening brace of a multi-line function declaration must be on the same line
$timer = Timer::getInstance($isArchivePhpTriggered);
$context = $this->buildContext($idSite, $period, $segment, $plugin, $report);

$timer->start($context);
}

/**
* @param int[] $idArchives
*/
public function onArchiveReportsComplete(
int $idSite,
Period $period,
Segment $segment,
string $plugin,
bool $isArchivePhpTriggered,
array $idArchives,
bool $wasCached,
?string $report = null
): void {
$timer = Timer::getInstance($isArchivePhpTriggered);
$context = $this->buildContext($idSite, $period, $segment, $plugin, $report);

$timer->complete($context, $idArchives, $wasCached);
}

private function buildContext(int $idSite, Period $period, Segment $segment, string $plugin, ?string $report = null): Context
{
return new Context($idSite, $period, $segment, $plugin, $report);
}
}
10 changes: 9 additions & 1 deletion plugins/ArchivingMetrics/Timer.php
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ final class Timer
private $runs = [];

/**
* @var Timer
* @var ?Timer
*/
private static $instance;

Expand Down Expand Up @@ -69,6 +69,14 @@ public static function getInstance(bool $isArchivePhpTriggered, ?ClockInterface
return self::$instance;
}

/**
* @internal For tests only.
*/
public static function resetInstanceForTests(): void
{
self::$instance = null;
}

public function start(Context $context): void
{
if (false === $this->isApplicableForTiming($context)) {
Expand Down
53 changes: 47 additions & 6 deletions plugins/CoreAdminHome/API.php
Original file line number Diff line number Diff line change
Expand Up @@ -299,20 +299,38 @@

$period = Factory::build($period, $date);
$site = new Site($idSite);
$segmentObj = new Segment(
$segment,
[$idSite],
$period->getDateTimeStart()->setTimezone($site->getTimezone()),
$period->getDateTimeEnd()->setTimezone($site->getTimezone())
);
$parameters = new ArchiveProcessor\Parameters(
$site,
$period,
new Segment(
$segment,
[$idSite],
$period->getDateTimeStart()->setTimezone($site->getTimezone()),
$period->getDateTimeEnd()->setTimezone($site->getTimezone())
)
$segmentObj
);
if ($report) {
$parameters->setArchiveOnlyReport($report);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it relevant for the current metric gathering if a single report was requested? For example, when a new custom report is being archived?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've decided now to skip for individual reports, i've left the condition at this level, is that going to be confusing do you think, that there is a decision here and internally in the timer.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not mean to add any skip-check here, I would actually not. The event handler should take care of not doing work on anything it receives but does not want.

Phrasing my question differently:

Would it make sense to add $report to the event signature? It is part of the archiveReports signature, and dropping it feels like a rather arbitrary decision.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is now used and removed $shouldRecordMetrics

}

/**
* Triggered before a full archiveReports run starts.
*
* Usage example:
* Piwik::addAction('CoreAdminHome.archiveReports.start', function ($idSite, $period, $segment, $plugin, $isArchivePhpTriggered) { ... });
*
* @internal
*/
Piwik::postEvent('CoreAdminHome.archiveReports.start', [
$idSite,
$period,
$segmentObj,
(string) $plugin,
$isArchivePhpTriggered,
$report

Check failure on line 331 in plugins/CoreAdminHome/API.php

View workflow job for this annotation

GitHub Actions / PHPCS

Multi-line arrays must have a trailing comma after the last element.
]);

// TODO: need to test case when there are multiple plugin archives w/ only some data each. does purging remove some that we need?
$archiveLoader = new ArchiveProcessor\Loader($parameters, $invalidateBeforeArchiving);

Expand All @@ -323,6 +341,29 @@
'nb_visits' => $result[1],
];
}

$idArchives = isset($result['idarchives']) ? (array) $result['idarchives'] : [];
$wasCached = $archiveLoader->didReuseArchive();

/**
* Triggered after a full archiveReports run completes.
*
* Usage example:
* Piwik::addAction('CoreAdminHome.archiveReports.complete', function ($idSite, $period, $segment, $plugin, $isArchivePhpTriggered, $idArchives, $wasCached, $report) { ... });
*
* @internal
*/
Piwik::postEvent('CoreAdminHome.archiveReports.complete', [
$idSite,
$period,
$segmentObj,
(string) $plugin,
$isArchivePhpTriggered,
$idArchives,
$wasCached,
$report

Check failure on line 364 in plugins/CoreAdminHome/API.php

View workflow job for this annotation

GitHub Actions / PHPCS

Multi-line arrays must have a trailing comma after the last element.
]);

return $result;
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
<?php

/**
* Matomo - free/libre analytics platform
*
* @link https://matomo.org
* @license https://www.gnu.org/licenses/gpl-3.0.html GPL v3 or later
*/

declare(strict_types=1);

namespace Piwik\Plugins\CoreAdminHome\tests\Integration;

use Piwik\Common;
use Piwik\Db;
use Piwik\Plugins\ArchivingMetrics\Timer;
use Piwik\Plugins\CoreAdminHome\API as CoreAdminHomeAPI;
use Piwik\Tests\Framework\Fixture;
use Piwik\Tests\Framework\TestCase\IntegrationTestCase;

/**
* @group CoreAdminHome
* @group CoreAdminHome_ArchiveReportsMetricsTimer
* @group Plugins
*/
class ArchiveReportsMetricsTimerTest extends IntegrationTestCase
{
public static function setUpBeforeClass(): void
{
self::$fixture->extraPluginsToLoad[] = 'ArchivingMetrics';

parent::setUpBeforeClass();
}

public function setUp(): void
{
parent::setUp();

Timer::resetInstanceForTests();
}

public function testArchiveReportsWritesMetricsOnceAndDoesNotWriteAgainWhenReusingDbArchive(): void
{
Fixture::createSuperUser(true);
$_GET['trigger'] = 'archivephp';

$idSite = Fixture::createWebsite('2024-01-01 00:00:00');

$t = Fixture::getTracker($idSite, '2024-01-01 12:00:00');
$t->setUrl('http://example.com/');
Fixture::checkResponse($t->doTrackPageView('test'));

CoreAdminHomeAPI::getInstance()->archiveReports($idSite, 'day', '2024-01-01');
$this->assertSame(1, $this->getMetricsCount(), 'Expected archiving_metrics to have 1 row after first archiveReports call.');

CoreAdminHomeAPI::getInstance()->archiveReports($idSite, 'day', '2024-01-01');
$this->assertSame(1, $this->getMetricsCount(), 'Expected archiving_metrics to still have 1 row after reusing the same DB archive.');
}

private function getMetricsCount(): int
{
return (int) Db::fetchOne('SELECT COUNT(*) FROM ' . Common::prefixTable('archiving_metrics'));
}
}
35 changes: 35 additions & 0 deletions tests/PHPUnit/Integration/ArchiveProcessor/LoaderTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,41 @@ protected static function beforeTableDataCached()
Fixture::createWebsite('2012-02-03 00:00:00');
}

public function testDidReuseArchiveFlagIsOnlySetWhenReusingArchiveFromDb()
{
$oldGet = $_GET;
Fixture::createSuperUser(true);
$_GET['trigger'] = 'archivephp';

$idSite = 1;
$dateTime = '2024-01-01 12:00:00';
$date = '2024-01-01';

$t = Fixture::getTracker($idSite, $dateTime);
$t->setUrl('http://example.com/');
Fixture::checkResponse($t->doTrackPageView('test'));

$periodObj = Factory::build('day', $date);

$params = new Parameters(new Site($idSite), $periodObj, new Segment('', [$idSite]));
$loader = new Loader($params);
$result = $loader->prepareArchive('VisitsSummary');

$this->assertFalse($loader->didReuseArchive(), 'Expected first archiving run to generate a new archive.');
$this->assertNotEmpty($result);
$this->assertNotEmpty($result[0]);

Cache::flushAll();

$params = new Parameters(new Site($idSite), $periodObj, new Segment('', [$idSite]));
$loader = new Loader($params);
$result = $loader->prepareArchive('VisitsSummary');

$this->assertTrue($loader->didReuseArchive(), 'Expected second archiving run to reuse the existing DB archive.');
$this->assertSame(1, (int) $result[0][0], 'Expected second archiving run to return the same archive ids.');
$_GET = $oldGet;
}

public function testPluginOnlyArchivingDoesNotRelaunchChildArchives()
{
$_GET['pluginOnly'] = 1;
Expand Down
Loading