Merge pull request #183 from matomo-org/PG-3685-alert-sent-on-incompl…

…ete-report Adding check for whether reports used by alert are done archiving
matomo-org · Sep 22, 2024 · 4cd6a21 · 4cd6a21
2 parents 00f7515 + 8c6a056
commit 4cd6a21
Show file tree

Hide file tree

Showing 7 changed files with 579 additions and 10 deletions.
diff --git a/CustomAlerts.php b/CustomAlerts.php
@@ -10,14 +10,26 @@
 namespace Piwik\Plugins\CustomAlerts;
 
 use Piwik\Common;
+use Piwik\Container\StaticContainer;
+use Piwik\Option;
 use Piwik\Piwik;
 use Piwik\Plugins\SitesManager\API as SitesManagerApi;
+use Piwik\Scheduler\Task;
 
 /**
  *
  */
 class CustomAlerts extends \Piwik\Plugin
 {
+    /**
+     * @var null|string
+     */
+    public static $currentlyRunningScheduledTaskName = null;
+
+    /**
+     * @var int
+     */
+    public static $currentlyRunningScheduledTaskRetryCount = 0;
 
     public function registerEvents()
     {
@@ -30,7 +42,9 @@ public function registerEvents()
             'Translate.getClientSideTranslationKeys' => 'getClientSideTranslationKeys',
             'UsersManager.deleteUser'                => 'deleteAlertsForLogin',
             'SitesManager.deleteSite.end'            => 'deleteAlertsForSite',
-            'Db.getTablesInstalled'                  => 'getTablesInstalled'
+            'Db.getTablesInstalled'                  => 'getTablesInstalled',
+            'ScheduledTasks.execute'                 => 'startingScheduledTask',
+            'ScheduledTasks.execute.end'             => 'endingScheduledTask',
         );
     }
 
@@ -183,6 +197,43 @@ public function getSiteIdsHavingAlerts()
         return array_values(array_unique($siteIdsHavingAlerts));
     }
 
+    /**
+     * If the task is for CustomAlerts, save the name of the task as static property so that we know what the currently
+     * running task is.
+     *
+     * @param Task $task
+     * @return void
+     */
+    public function startingScheduledTask(Task $task): void
+    {
+        if (strpos($taskName = $task->getName(), 'Piwik\Plugins\CustomAlerts\Tasks') === false) {
+            return;
+        }
+
+        self::$currentlyRunningScheduledTaskName = $taskName;
+
+        /** @var \Piwik\Scheduler\Timetable $timetable */
+        $timetable = StaticContainer::getContainer()->get('Piwik\Scheduler\Timetable');
+        // Look up the retry count so that we know whether this is a retry or not
+        self::$currentlyRunningScheduledTaskRetryCount = $timetable->getRetryCount($taskName);
+    }
+
+    /**
+     * If the task is for CustomAlerts, clear the property containing the name of the task that just ran.
+     *
+     * @param Task $task
+     * @return void
+     */
+    public function endingScheduledTask(Task $task): void
+    {
+        if (strpos($task->getName(), 'Piwik\Plugins\CustomAlerts\Tasks') === false) {
+            return;
+        }
+
+        self::$currentlyRunningScheduledTaskName = null;
+        self::$currentlyRunningScheduledTaskRetryCount = 0;
+    }
+
     public function getClientSideTranslationKeys(&$translations)
     {
         $translations[] = 'General_Value';

diff --git a/Model.php b/Model.php
@@ -194,6 +194,23 @@ public function getTriggeredAlerts($idSites, $login)
         return $alerts;
     }
 
+    public function getTriggeredAlertsFromPastNHours(string $period, int $idSite, int $hours)
+    {
+        $timestamp = Date::now()->addHour(-1 * $hours)->getDatetime();
+
+        $db  = $this->getDb();
+        $sql = $this->getTriggeredAlertsSelectPart()
+            . " WHERE idsite = ?"
+            . " AND  period = ?"
+            . " AND  ts_triggered > ?";
+
+        $values = [$idSite, $period, $timestamp];
+
+        $alerts = $db->fetchAll($sql, $values);
+
+        return $this->completeAlerts($alerts);
+    }
+
     public function getAllAlerts()
     {
         $sql = "SELECT * FROM " . Common::prefixTable('alert');

diff --git a/Processor.php b/Processor.php
@@ -11,10 +11,14 @@
 
 use Piwik\API\Request as ApiRequest;
 use Piwik\Common;
+use Piwik\Container\StaticContainer;
 use Piwik\Context;
 use Piwik\DataTable;
 use Piwik\Date;
+use Piwik\Option;
+use Piwik\Piwik;
 use Piwik\Plugins\API\ProcessedReport;
+use Piwik\Scheduler\RetryableException;
 use Piwik\Site;
 
 /**
@@ -30,11 +34,16 @@ class Processor
      * @var Validator
      */
     private $validator;
+    /**
+     * @var Model
+     */
+    private $model;
 
-    public function __construct(ProcessedReport $processedReport, Validator $validator)
+    public function __construct(ProcessedReport $processedReport, Validator $validator, Model $model)
     {
         $this->processedReport = $processedReport;
         $this->validator       = $validator;
+        $this->model           = $model;
     }
 
     public static function getComparablesDates()
@@ -84,15 +93,54 @@ public static function getMetricConditions()
         );
     }
 
+    /**
+     * @param $period
+     * @param $idSite
+     * @return void
+     * @throws RetryableException The exception that should be caught and handled by the Scheduler class. Only happens
+     * if there's an alert that can't be processed yet and needs to be retried.
+     */
     public function processAlerts($period, $idSite)
     {
         $alerts = $this->getAllAlerts($period);
 
+        $previouslyProcessedAlerts = $this->getPreviouslyProcessedAlerts($period, intval($idSite));
+
+        $failedAlerts = [];
         foreach ($alerts as $alert) {
-            $this->processAlert($alert, $idSite);
+            // Skip alerts that were processed previously
+            if (in_array($alert['idalert'], array_column($previouslyProcessedAlerts, 'idalert'))) {
+                continue;
+            }
+
+            try {
+                $this->processAlert($alert, $idSite);
+            } catch (RetryableException $e) {
+                $failedAlerts[] = $alert;
+            }
+        }
+
+        if (!empty($failedAlerts)) {
+            // Build up a retry exception message based of the alerts that failed to process
+            $alertsString = '';
+            foreach ($failedAlerts as $failedAlert) {
+                $alertsString .= "\nID: {$failedAlert['idalert']} Name: {$failedAlert['name']} Login: {$failedAlert['login']} Report: {$failedAlert['report']}";
+            }
+
+            if (CustomAlerts::$currentlyRunningScheduledTaskRetryCount === 3) {
+                StaticContainer::get(\Piwik\Log\LoggerInterface::class)->warning(Piwik::translate('CustomAlerts_FinalTaskRetryWarning', [$alertsString]));
+            }
+
+            // Throw an exception to let the scheduler know to retry the task
+            throw new RetryableException(Piwik::translate('CustomAlerts_TaskRetryExceptionMessage', [$alertsString]));
         }
     }
 
+    protected function getPreviouslyProcessedAlerts(string $period, int $idSite): array
+    {
+        return $this->model->getTriggeredAlertsFromPastNHours($period, $idSite, 12);
+    }
+
     private function getAllAlerts($period)
     {
         return $this->getModel()->getAllAlertsForPeriod($period);
@@ -103,6 +151,12 @@ private function getModel()
         return new Model();
     }
 
+    /**
+     * @param $alert
+     * @param $idSite
+     * @return void
+     * @throws RetryableException
+     */
     protected function processAlert($alert, $idSite)
     {
         if (!$this->shouldBeProcessed($alert, $idSite)) {
@@ -158,10 +212,11 @@ private function reportExists($idSite, $report, $metric)
 
     /**
      * @param array $alert
-     * @param int   $idSite
-     * @param int   $subPeriodN
+     * @param int $idSite
+     * @param int $subPeriodN
      *
      * @return array
+     * @throws RetryableException If the report has an archive status, and it's something other than complete
      */
     public function getValueForAlertInPast($alert, $idSite, $subPeriodN)
     {
@@ -185,6 +240,11 @@ public function getValueForAlertInPast($alert, $idSite, $subPeriodN)
             'filter_limit'           => -1
         );
 
+        // Only include the archive state param for versions of Matomo that allow it
+        if (version_compare(\Piwik\Version::VERSION, '5.1.0-b1', '>=')) {
+            $params['fetch_archive_state'] = 1;
+        }
+
         if (!empty($report['parameters'])) {
             $params = array_merge($params, $report['parameters']);
         }
@@ -193,13 +253,49 @@ public function getValueForAlertInPast($alert, $idSite, $subPeriodN)
 
         $table = ApiRequest::processRequest($report['module'] . '.' . $report['action'], $params, $default = []);
 
+        // If the response is a DataTable, check the archiving status
+        if ($table instanceof DataTable) {
+            $this->checkWhetherArchiveIsComplete($alert, $table);
+        }
+
         $value = $this->aggregateToOneValue($table, $alert['metric'], $alert['report_condition'], $alert['report_matched']);
 
         DataTable\Manager::getInstance()->deleteAll($subtableId);
 
         return $value;
     }
 
+    /**
+     * Checks whether the archive status is complete. We throw an exception if the status is something other than
+     * complete. If no status is found, we do nothing.
+     *
+     * @param array $alert Array containing all the alert information
+     * @param DataTable $table Should have the archive_state metadata set because the fetch_archive_state query param
+     * was set as part of the API request.
+     *
+     * @return void
+     * @throws RetryableException If the archive status is found and isn't complete
+     */
+    protected function checkWhetherArchiveIsComplete(array $alert, DataTable $table): void
+    {
+        // Don't bother checking older versions of Matomo since the data and constants won't be there
+        if (version_compare(\Piwik\Version::VERSION, '5.1.0-b1', '<')) {
+            return;
+        }
+
+        $archiveState = $table->getMetadata(DataTable::ARCHIVE_STATE_METADATA_NAME);
+        if (empty($archiveState)) {
+            return;
+        }
+
+        if ($archiveState === \Piwik\Archive\ArchiveState::COMPLETE) {
+            return;
+        }
+
+        // Throw an exception since the archive status was provided and isn't complete
+        throw new RetryableException('This alert is not ready to process due to incomplete archiving');
+    }
+
     private function getDateForAlertInPast($idSite, $period, $subPeriodN)
     {
         $timezone = Site::getTimezoneFor($idSite);

diff --git a/lang/en.json b/lang/en.json
@@ -71,6 +71,8 @@
         "ValuePercentageIncreasedMoreThan": "increased more than %1$s%% from %2$s to %3$s",
         "WeekComparedToPreviousWeek": "previous week",
         "When": "when",
-        "YouCanChoosePeriodFrom": "You can choose from the following options"
+        "YouCanChoosePeriodFrom": "You can choose from the following options",
+        "TaskRetryExceptionMessage": "The following alerts were unable to process because archiving is not complete for the associated reports: %1$s",
+        "FinalTaskRetryWarning": "Final retry of alerts task. Unable to process the following alerts: %1$s"
     }
 }