Allow plugin to decide about archiving without visits #10719

sgiehl · 2016-10-09T11:45:06Z

At the moment no plugin archiver will be executed if there are no visits for the archived period.
In some cases it would be useful for plugins to be able to archive data even if there are no visits for the archived period.
(The existing possibility to force all archivers to run for a specific site if there are no visists might be a bit to "much" in some cases.)

tsteur · 2016-10-09T19:37:05Z

FYI: refs #8820 and #8631

In #8631 (comment) I mention how nice it would be to have an Import API for this which I always wanted for this case. It would make it much simpler for developers as they won't have to take care of Archiving.getIdSitesToArchiveWhenNoVisits event and this Archiver method etc. There are probably several tweaks needed currently. I'll think a little if I find something to integrate such an API quickly

tsteur · 2016-10-09T19:43:43Z

Actually, is this method even needed or could it be solved with the event Archiving.getIdSitesToArchiveWhenNoVisits?

tsteur · 2016-10-09T19:46:06Z

and I wonder if you maybe need an event in https://github.com/piwik/piwik/blob/3.x-dev/core/CronArchive.php#L820 ?

sgiehl · 2016-10-09T22:11:55Z

Imho this doesn't replace kind of a Import API. An Import API should imho make it possible to "inject" plugin data into existing archives. So if data for a plugin is available after the archives for a date has been built, it should be possible to append the plugin data, without having to run the complete archiving for this day and period.

This extension is somehow an improvement to Archiving.getIdSitesToArchiveWhenNoVisits. That will always run the archiving for all plugins. Purpose of this new method is to make it possible to run archiving for a specific plugin if the are no visits.
For the case of importing external data in a plugin it won't always make sense to run the whole archiving.

I'll have a look at CronArchive not sure right now if something needs to be adjusted there, as well

tsteur · 2016-10-09T22:21:17Z

I understand. I wonder if we could then deprecate / remove the getIdSitesToArchiveWhenNoVisits event as we are not using it in one of our plugins anyway but there are still use cases where it is useful to have I think

sgiehl · 2016-10-09T22:23:48Z

Thought about deprecating it, as well. But there are use cases like MetaSites and stuff like that.

sgiehl · 2016-10-09T23:09:34Z

Had a short look at CronArchive. For me it seems a bit weird.
As you pointed out - if there aren't any visits the archiving for a day should somehow be skipped and archiveReportsFor isn't called in https://github.com/piwik/piwik/blob/3.x-dev/core/CronArchive.php#L844
But having a look at the archiveReportsFor method shows that there actually isn't done much as most is skipped for day (see https://github.com/piwik/piwik/blob/3.x-dev/core/CronArchive.php#L901)

Maybe it's a bit late. Will have another look tomorrow...

mattab · 2016-11-11T00:54:07Z

@sgiehl assigning tentatively to 3.0.0-b4 - maybe we'll make progress by then

sgiehl · 2016-11-20T19:32:19Z

I had another look at that one. But I'm currently not sure if the implementation is the best now.

Actually the "base" archiving for day period is running always, only archiving for segments will be executed only if there had been visits.
As we don't have any information in CronArchive if any plugin should archive even without visits I've added a check in CronArchive if any Plugin Archiver implements the shouldRunWithoutVisits method and if it returns true. I had to make some methods static for this, as otherwise I would need to fake a archive processing to get instances of plugin archivers.

Another solution that we maybe could discuss would be to always trigger archiving for everything in CronArchive and let the archiving process decide what to archive or not.

mattab · 2017-01-08T08:23:21Z

@sgiehl should we create an issue for Piwik 4 to deprecate the event getIdSitesToArchiveWhenNoVisits ? it appears unused even in Roll-Up reporting plugin, and not used in any other plugin on the marketplace.

mattab

Assuming it works (which I haven't tested), and after few comments here, looks good to me.

mattab · 2017-01-08T08:34:04Z

CHANGELOG.md

@@ -44,6 +44,7 @@ If the tracker is not initialised correctly, the browser console will display th
 * The creation of settings has slightly changed to improve performance. It is now possible to create new settings via the method `$this->makeSetting()` see `Piwik\Plugins\ExampleSettingsPlugin\SystemSettings` for an example.
 * It is no longer possible to define an introduction text for settings.
 * If requesting multipe periods for one report, the keys that define the range are no longer translated. For example before 3.0 an API response may contain: `<result date="From 2010-02-01 to 2010-02-07">` which is now `<result date="2010-02-01,2010-02-07">`.
+* The method `Piwik\Plugin\Archiver::shouldRunWithoutVisits()` has been added. It gives plugin archivers the possibility to decide whether to run if there weren't any visits by overwriting this method.


Could we maybe rename the method from shouldRunWithoutVisits to shouldRunEvenWhenNoVisit ?

minor but would write it like By overwriting this method and returning true, a plugin archiver can force the archiving to run even when there was no visit for the website/date/period/segment combination (by default, archivers are skipped when there is no visit) or so (and would use similar wording in the method documentation itself as well)

Sure, I'll change that

mattab · 2017-01-08T08:38:12Z

tests/PHPUnit/Integration/ArchiveWithNoVisitsTest.php

+    {
+        PluginsArchiver::$archivers['VisitsSummary'] = 'Piwik\Tests\Integration\ArchiveWithNoVisitsTest_MockArchiver';
+
+        ArchiveWithNoVisitsTest_MockArchiver::$runWithoutVisits = true;


let's add the similar test case with runWithoutVisits = false so the different behavior can be seen

That's what a part of the test before is doing

Ok got it now, maybe for clarity you could split the other test in two? Since when we will deprecate the event we might remove the other test without seeing it was testing another useful case. LGTM otherwise 👍

I've splitted the tests in 9681533

…Archiver

mattab · 2017-01-09T20:57:53Z

CHANGELOG.md

@@ -44,6 +44,7 @@ If the tracker is not initialised correctly, the browser console will display th
 * The creation of settings has slightly changed to improve performance. It is now possible to create new settings via the method `$this->makeSetting()` see `Piwik\Plugins\ExampleSettingsPlugin\SystemSettings` for an example.
 * It is no longer possible to define an introduction text for settings.
 * If requesting multipe periods for one report, the keys that define the range are no longer translated. For example before 3.0 an API response may contain: `<result date="From 2010-02-01 to 2010-02-07">` which is now `<result date="2010-02-01,2010-02-07">`.
+* The method `Piwik\Plugin\Archiver::shouldRunEvenWhenNoVisits()` has been added. By overwriting this method and returning true, a plugin archiver can force the archiving to run even when there was no visit for the website/date/period/segment combination (by default, archivers are skipped when there is no visit).


I'll move this to the 3.0.1 section

sgiehl added the Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. label Oct 9, 2016

sgiehl force-pushed the pluginarchivnovisits branch 2 times, most recently from e6a042a to 9541a43 Compare October 9, 2016 12:04

sgiehl added Needs Review PRs that need a code review and removed Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. labels Oct 9, 2016

sgiehl force-pushed the pluginarchivnovisits branch from 9541a43 to ac77fce Compare October 11, 2016 16:47

sgiehl force-pushed the pluginarchivnovisits branch from ac77fce to 3e8365a Compare October 31, 2016 18:29

mattab added this to the 3.0.0-b4 milestone Nov 11, 2016

sgiehl force-pushed the pluginarchivnovisits branch from 3e8365a to 2993d87 Compare November 20, 2016 19:31

sgiehl force-pushed the pluginarchivnovisits branch from 2993d87 to 44e90ee Compare November 20, 2016 20:03

mattab modified the milestones: 3.0.0-b5, 3.0.0-b4, 3.0.0 Nov 30, 2016

sgiehl force-pushed the pluginarchivnovisits branch from 44e90ee to 49ec4fc Compare December 12, 2016 21:52

mattab modified the milestones: 3.0.0-rc, 3.0.0, 3.0.1 Dec 13, 2016

sgiehl force-pushed the pluginarchivnovisits branch from 49ec4fc to f822589 Compare December 17, 2016 12:25

sgiehl force-pushed the pluginarchivnovisits branch 4 times, most recently from 7813e32 to be4b4c1 Compare December 29, 2016 23:47

sgiehl force-pushed the pluginarchivnovisits branch 2 times, most recently from 8ddc899 to 4c55659 Compare January 7, 2017 17:41

mattab reviewed Jan 8, 2017

View reviewed changes

sgiehl force-pushed the pluginarchivnovisits branch 2 times, most recently from 98bec9c to 60f495e Compare January 8, 2017 17:01

sgiehl added 3 commits January 8, 2017 18:39

allow plugin to decide to archive without visits

f2be781

Respect new shouldRunWithoutVisits method of plugin archivers in Cron…

d0dba57

…Archiver

renames method and updates doc

f4f7b76

sgiehl force-pushed the pluginarchivnovisits branch from 60f495e to f4f7b76 Compare January 8, 2017 17:39

mattab merged commit d28efb2 into 3.x-dev Jan 9, 2017

mattab deleted the pluginarchivnovisits branch January 9, 2017 08:11

sgiehl added a commit that referenced this pull request Jan 9, 2017

split tests as discussed in #10719

9681533

mattab reviewed Jan 9, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow plugin to decide about archiving without visits #10719

Allow plugin to decide about archiving without visits #10719

sgiehl commented Oct 9, 2016

tsteur commented Oct 9, 2016

tsteur commented Oct 9, 2016

tsteur commented Oct 9, 2016

sgiehl commented Oct 9, 2016 •

edited

tsteur commented Oct 9, 2016

sgiehl commented Oct 9, 2016

sgiehl commented Oct 9, 2016

mattab commented Nov 11, 2016

sgiehl commented Nov 20, 2016

mattab commented Jan 8, 2017 •

edited

mattab left a comment

mattab Jan 8, 2017

mattab Jan 8, 2017

sgiehl Jan 8, 2017

mattab Jan 8, 2017

sgiehl Jan 8, 2017

mattab Jan 9, 2017

sgiehl Jan 9, 2017

mattab Jan 9, 2017

Allow plugin to decide about archiving without visits #10719

Allow plugin to decide about archiving without visits #10719

Conversation

sgiehl commented Oct 9, 2016

tsteur commented Oct 9, 2016

tsteur commented Oct 9, 2016

tsteur commented Oct 9, 2016

sgiehl commented Oct 9, 2016 • edited

tsteur commented Oct 9, 2016

sgiehl commented Oct 9, 2016

sgiehl commented Oct 9, 2016

mattab commented Nov 11, 2016

sgiehl commented Nov 20, 2016

mattab commented Jan 8, 2017 • edited

mattab left a 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

sgiehl commented Oct 9, 2016 •

edited

mattab commented Jan 8, 2017 •

edited