Move Archive.php archive invalidation to Loader… #15616

diosmosis · 2020-02-24T06:01:44Z

… so we only invalidate when about to launch archiving.

Not sure if there are tests I can add for this just yet.

… when about to launch archiving.

sgiehl · 2020-02-24T09:44:43Z

Seems a test is failing now. Otherwise guess looks good to merge

tsteur · 2020-02-24T22:03:21Z

Here's some thoughts maybe that would likely improve our problem but not really 100% sure if it makes sense and would work @diosmosis

diff --git a/core/ArchiveProcessor/Loader.php b/core/ArchiveProcessor/Loader.php
index c99dd3d02e..d6f896fee6 100644
--- a/core/ArchiveProcessor/Loader.php
+++ b/core/ArchiveProcessor/Loader.php
@@ -79,15 +79,30 @@ class Loader
     {
         $this->params->setRequestedPlugin($pluginName);
 
+        // no valid archive exists, make sure to invalidate existing archives
+        if (($isBrowserArchiveEnabled || $isArchiveTriggered) && $this->invalidateBeforeArchiving) {
+            $this->invalidatedReportsIfNeeded(); // SHOULD INVALIDATE ALL REPORTS
+        }
+
         list($idArchive, $visits, $visitsConverted) = $this->loadExistingArchiveIdFromDb();
         if (!empty($idArchive)) {
+            // IF REPORT INCLUDES TODAY....
+                // IF ARCHIVE WAS GENERATED WITHIN LAST time_before_today_archive_considered_outdated SECONDS DO NOT RUN
+                // INVALIDATE LOGIC SINCE REPORT WOULD NOT BE REGENERATED ANYWAY
+
+                // IF ARCHIVE WAS GENERATED MORE THAN time_before_today_archive_considered_outdated SECONDS AGO,
+                // RUN INVALIDATION LOGIC AND CALL loadExistingArchiveIdFromDb() again in case it is no longer valid
+
+            // IF REPORT NOT INCLUDES TODAY
+                // IDEALLY WE INVALIDATE REPORTS ONLY THAT AFFECT THAT PERIOD TO NOT SLOW DOWN EG RANGE REQUESTS
+
             return $idArchive;
         }
 
-        // no valid archive exists, make sure to invalidate existing archives
-        if ($this->invalidateBeforeArchiving) {
-            $this->invalidatedReportsIfNeeded();
-        }
+        // NO NEED TO INVALIDATE IF THE ARCHIVE DOES NOT EXIST SINCE IT WOULDN'T MAKE A DIFFERENCE TO THE RESULT. MEANING
+        // IF WE DID INVALIDATE REPORTS, THEN THE OUTPUT BE STILL THE SAME. EG THIS WAY WE AVOID INVALIDATING REPORTS WHEN
+        // RANGE IS SELECTED AND THE REPORT DOES NOT EXIST YET. AS A RESULT OF THIS WE WOULD ALSO NOT INVALIDATE AND
+        // REARCHIVE ANY SUBPERIODS
 
         /** @var ArchivingStatus $archivingStatus */
         $archivingStatus = StaticContainer::get(ArchivingStatus::class);

Generally speaking eg on Cloud:

Browser archiving and segment archiving is disabled so only range periods are causing us trouble and need to ideally avoid invalidating reports for range dates when the archive doesn't exist or when it was created recently
The problem with invalidation of reports is that it's fairly slow, plus causes the tracker cache general to be emptied which we want to avoid whenever possible.

While many other users have browser archiving disabled, segment archiving is still enabled etc. For these users it be great to invalidate all reports only when isArchivePhpTriggered(). In all other cases to keep UI fast and not invalidate tracker caches too often we should only invalidate reports when needed as well see comments.

When browser archiving is enabled, it'll be a bit of a problem since we often have to invalidate reports in order to get accurate results unless some report was generated recently (say within 900s if default config is used). This might be still fine though. It'll just make Matomo even slower but maybe not possible to avoid it until we are in Matomo 4.

…ving.

diosmosis · 2020-02-25T06:03:40Z

@tsteur I tried making changes to handle every possible case for when archiving, can you take a look? I haven't modified tests yet, you can probably just look at the latest commit.

core/DataAccess/ArchiveSelector.php

tsteur · 2020-02-28T01:09:29Z

core/DataAccess/ArchiveSelector.php

+        if (!isset($result['idarchive'])
+            || !isset($result['nb_visits'])
+        ) {
+            return [false, false, false, true];


btw here it returned for me isAnyArchiveExists = true even though there was no archive with a done flag. Only an archive with a nb_visits. This can happen if eg the archiving fails before the done flag was written but after nb_visits was written. Not sure it's important.

core/DataAccess/ArchiveSelector.php

diosmosis · 2020-03-03T07:05:22Z

@tsteur noticed an issue w/ this change, if we invalidate only if there is something to invalidate, we may end up archiving more than once in some situations. For example:

archive for date 2012-02-02 and site 1 does not exist
tracking request comes in for that date & site, and pair is added to option list
archive for date 2012-02-02 and site 1 is requested
there is no existing archive, so nothing is invalidated
archiving is initiated for the day 2012-02-02, site 1
another archive request comes in for 2012-02-02, site 1
there is now an existing archive, and the date/site is in the list of pairs to archive, so we invalidate it
archiving is run again for 2012-02-02, site 1

We can't remove the date/site in the list in this case since it could also invalidate other archives that are not currently being requested.

The only way to do this change would be to run the invalidate query as a SELECT first then invalidate if it returns something, but that would of course be pointless.

NOTE: this only affects browser archiving, btw. Also with the 'today' change it may end up being a bit of an edge case... only other issue is that some archives may not be invalidated if they are never requested. Which would only be an issue if the options in the list are removed. We can have a daily task in that case to just run the invalidation I guess.

…e check in CronArchive.

tsteur · 2020-03-03T20:28:01Z

Which would only be an issue if the options in the list are removed. We can have a daily task in that case to just run the invalidation I guess.

Daily task for this should work. 👍

tsteur · 2020-03-03T20:28:34Z

Or maybe at the beginning of the archiving we still need to invalidate all reports like we used to for cron archiving. For browser archiving handling would need a task indeed.

diosmosis · 2020-03-03T22:18:33Z

Or maybe at the beginning of the archiving we still need to invalidate all reports like we used to for cron archiving.

We still do this, I haven't changed that (I disabled the Archive.php code for invalidation if browser archiving is enabled, so it's all done by core:archive, which I think is better IMO).

diosmosis · 2020-03-04T23:45:09Z

@tsteur tests should pass now, ready for a review. I may add some more tests in the meantime.

tsteur · 2020-03-05T22:54:52Z

plugins/CoreAdminHome/Tasks.php

@@ -81,6 +88,17 @@ public function schedule()
        $this->scheduleTrackingCodeReminderChecks();
    }

+    public function invalidateOutdatedArchives()
+    {
+        if (false && !Rules::isBrowserTriggerEnabled()) {


the false is probably debugging?

it is still there @diosmosis ?

huh, must've missed it? not sure how

tsteur · 2020-03-05T22:56:22Z

core/ArchiveProcessor/Loader.php

@@ -242,4 +271,33 @@ private function getIdSitesToArchiveWhenNoVisits()

        return $cache->fetch($cacheKey);
    }
+
+    private function invalidatedReportsIfNeeded()


@diosmosis might be good to add some tests for this maybe? Like it could return the invalidated date/siteIds which we then can test against?

added tests

tsteur · 2020-03-05T23:01:14Z

core/CronArchive.php

+        $isLast = preg_match('/^last([0-9]+)/', $date, $matches);
+
+        // don't do this check for a single period that includes today
+        if ($isTodayIncluded


@diosmosis not 100% understanding this part. today would be mostly included right? I understand that we can assume basically there is no valid archive for today because we archive each time anyway. I'm just wondering if it would still need to process below code in case

Cron Archive starts

The default date is last2

We invalidate all reports

We are about to archive a site

This logic runs

Now if previously some reports from 10 days ago are invalidated, we would need to change it to last10 basically

Not sure it's clear what I mean? Or maybe this is not needed anymore?

The idea here was if it was a single period like, period = week, day = today, then we don't check for an existing usable archive. If it is for a date in the past, then we check if there is a usable archive and if so we don't need to archive.

For lastN values, however, it is more complicated. Here, we want to check for every date except today. So we check for valid archives. If the date is last5 and for the 5th, 4th and 3rd day we have valid archives, then we change it to last2. The only difference is if we have valid archives for every day, we still do last1 (I think the min is last1 now but we can change it back to last2).

Maybe I can add some tests for this code too.

added tests, maybe you can better see what it's trying to do

fully getting it now 👍

tsteur · 2020-03-09T00:53:12Z

core/CronArchive.php

+                $newDate = 'last' . min($lastNValue, $newRangePeriod->getNumberOfSubperiods());
+            } else if ($isTodayIncluded) {
+                $isThereArchiveForAllPeriods = false;
+                $newDate = 'last1';


btw not sure but might still need to use last2 here to workaround some archiving problems where it would maybe create a range date for today and some plugins may have trouble with it. I suppose maybe shouldn't hurt to use last2?

I thought this too, but I wasn't sure... I'll change it just in case.

tsteur · 2020-03-09T01:08:17Z

core/CronArchive.php

+            } else if ($isTodayIncluded) {
+                $isThereArchiveForAllPeriods = false;
+                $newDate = 'last1';
+            }


btw... I was thinking... if it is last1 (aka today, this week, this month), then we could technically get the date when this archive was last generated, and if it was generated less than time_before_(today|$period)_archive_considered_outdated seconds ago, return $isThereArchiveForAllPeriods = true and skip the archive this time.

Only if it's easy to do though. Otherwise could also just launch the archiving. On cloud where we consider archives outdated sometimes only evert 6, 12, 18 hours (depending on config), then it could save us launching quite a few archive launches.

Not sure it's clear what I mean? It be probably similar to what we have in browser archiving.

I think this is already done, regardless of whether browser archiving is used or not. In Loader.php we always use the result of getMinTimeArchiveProcessed(). So it should always look for an archive that is out of date, correct? I could maybe add a debug log for the future...

tsteur · 2020-03-09T01:09:12Z

👍 left another comment @diosmosis few tests are still failing but I suppose you still work in it. eg https://travis-ci.org/matomo-org/matomo/jobs/659112026?utm_medium=notification&utm_source=github_status

diosmosis · 2020-03-09T10:26:10Z

@tsteur fixed some tests, there are some unrelated failures though

tsteur · 2020-03-09T23:18:55Z

plugins/Monolog/Processor/ExceptionToTextProcessor.php

@@ -85,6 +85,10 @@ private function getStackTrace($exception)

    public static function getWholeBacktrace(\Exception $exception, $shouldPrintBacktrace = true)
    {
+        if (!$shouldPrintBacktrace) {


@diosmosis should this one still be needed?

Yes, otherwise the backtrace is always printed. This was an unrelated bug I found.

* Move Archive.php archive invalidation to Loader so we only invalidate when about to launch archiving. * Attempt to handle more cases when invalidating before launching archiving. * fix possible sql error * fix possible error * fixing some tests * remove test code * Only invalidate specific archive being requested. * Do not invalidate on today in tracker and avoid existing valid archive check in CronArchive. * more test fixes * Attempt to fix more tests. * Fixing last tests. * another test fix * Invalidate in scheduled task if browser triggered archiving is enabled. * deal with TODO * Get ArchiveSelectorTest to pass. * applying review feedback including new tests * apply review feedback & fix tests * fix couple more tests Co-authored-by: Thomas Steur <[email protected]>

Move Archive.php archive invalidation to Loader so we only invalidate…

c8b6c3c

… when about to launch archiving.

diosmosis added the Needs Review PRs that need a code review label Feb 24, 2020

diosmosis added this to the 3.13.4 milestone Feb 24, 2020

Attempt to handle more cases when invalidating before launching archi…

a3fb62a

…ving.

fix possible sql error

143c9c3

tsteur reviewed Feb 28, 2020

View reviewed changes

core/DataAccess/ArchiveSelector.php Show resolved Hide resolved

fix possible error

f218967

tsteur requested changes Feb 28, 2020

View reviewed changes

diosmosis force-pushed the archive-php-invalidation-opt branch from 0117b39 to b6de30f Compare March 1, 2020 07:05

mattab added the c: Performance For when we could improve the performance / speed of Matomo. label Mar 1, 2020

diosmosis force-pushed the archive-php-invalidation-opt branch 2 times, most recently from 5deb1ac to 052d824 Compare March 1, 2020 12:42

tsteur mentioned this pull request Mar 1, 2020

Today archive is invalidated too often #15646

Closed

fixing some tests

2dd4958

diosmosis force-pushed the archive-php-invalidation-opt branch from 052d824 to 2dd4958 Compare March 2, 2020 12:33

diosmosis added 2 commits March 2, 2020 19:25

remove test code

70e5513

Only invalidate specific archive being requested.

55e2435

diosmosis added 2 commits March 3, 2020 03:41

Do not invalidate on today in tracker and avoid existing valid archiv…

f2890f9

…e check in CronArchive.

more test fixes

e45ba01

diosmosis added 3 commits March 3, 2020 20:03

Attempt to fix more tests.

9e24cb2

Fixing last tests.

7c34b24

another test fix

e9ba420

diosmosis added 3 commits March 4, 2020 21:17

Invalidate in scheduled task if browser triggered archiving is enabled.

52498fd

deal with TODO

72d886b

Get ArchiveSelectorTest to pass.

0272cc1

tsteur reviewed Mar 5, 2020

View reviewed changes

diosmosis added 2 commits March 6, 2020 03:13

applying review feedback including new tests

ff933b0

Merge branch '3.x-dev' into archive-php-invalidation-opt

a95cfd5

tsteur reviewed Mar 9, 2020

View reviewed changes

diosmosis added 2 commits March 9, 2020 00:38

apply review feedback & fix tests

c0c682e

fix couple more tests

b8e727a

tsteur reviewed Mar 9, 2020

View reviewed changes

diosmosis merged commit 22cde64 into 3.x-dev Mar 10, 2020

diosmosis deleted the archive-php-invalidation-opt branch March 10, 2020 00:21

tsteur mentioned this pull request Mar 15, 2020

Loading the dashboard with more than 2 goals in "goal overview" widget is very slow because of update query #15579

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move Archive.php archive invalidation to Loader… #15616

Move Archive.php archive invalidation to Loader… #15616

diosmosis commented Feb 24, 2020

sgiehl commented Feb 24, 2020

tsteur commented Feb 24, 2020

diosmosis commented Feb 25, 2020

tsteur Feb 28, 2020

diosmosis commented Mar 3, 2020 •

edited

Loading

tsteur commented Mar 3, 2020

tsteur commented Mar 3, 2020

diosmosis commented Mar 3, 2020

diosmosis commented Mar 4, 2020

tsteur Mar 5, 2020

diosmosis Mar 6, 2020

tsteur Mar 9, 2020

diosmosis Mar 9, 2020

tsteur Mar 5, 2020

diosmosis Mar 6, 2020

tsteur Mar 5, 2020

diosmosis Mar 6, 2020

diosmosis Mar 6, 2020

tsteur Mar 9, 2020

tsteur Mar 9, 2020

diosmosis Mar 9, 2020

tsteur Mar 9, 2020 •

edited

Loading

diosmosis Mar 9, 2020

tsteur commented Mar 9, 2020

diosmosis commented Mar 9, 2020

tsteur Mar 9, 2020

diosmosis Mar 9, 2020

Move Archive.php archive invalidation to Loader… #15616

Move Archive.php archive invalidation to Loader… #15616

Conversation

diosmosis commented Feb 24, 2020

sgiehl commented Feb 24, 2020

tsteur commented Feb 24, 2020

diosmosis commented Feb 25, 2020

Choose a reason for hiding this comment

diosmosis commented Mar 3, 2020 • edited Loading

tsteur commented Mar 3, 2020

tsteur commented Mar 3, 2020

diosmosis commented Mar 3, 2020

diosmosis commented Mar 4, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsteur Mar 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsteur commented Mar 9, 2020

diosmosis commented Mar 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diosmosis commented Mar 3, 2020 •

edited

Loading

tsteur Mar 9, 2020 •

edited

Loading