Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow plugin to decide about archiving without visits #10719

Merged
merged 3 commits into from Jan 9, 2017
Merged

Conversation

@sgiehl
Copy link
Member

sgiehl commented Oct 9, 2016

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.)

@sgiehl sgiehl force-pushed the pluginarchivnovisits branch 2 times, most recently from e6a042a to 9541a43 Oct 9, 2016
@sgiehl sgiehl added Needs Review and removed Pull Request WIP labels Oct 9, 2016
@tsteur

This comment has been minimized.

Copy link
Member

tsteur commented Oct 9, 2016

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

This comment has been minimized.

Copy link
Member

tsteur commented Oct 9, 2016

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

@tsteur

This comment has been minimized.

Copy link
Member

tsteur commented Oct 9, 2016

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

@sgiehl

This comment has been minimized.

Copy link
Member Author

sgiehl commented Oct 9, 2016

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

This comment has been minimized.

Copy link
Member

tsteur commented Oct 9, 2016

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

This comment has been minimized.

Copy link
Member Author

sgiehl commented Oct 9, 2016

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

@sgiehl

This comment has been minimized.

Copy link
Member Author

sgiehl commented Oct 9, 2016

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...

@sgiehl sgiehl force-pushed the pluginarchivnovisits branch from 9541a43 to ac77fce Oct 11, 2016
@sgiehl sgiehl force-pushed the pluginarchivnovisits branch from ac77fce to 3e8365a Oct 31, 2016
@mattab

This comment has been minimized.

Copy link
Member

mattab commented Nov 11, 2016

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

@sgiehl sgiehl force-pushed the pluginarchivnovisits branch from 3e8365a to 2993d87 Nov 20, 2016
@sgiehl

This comment has been minimized.

Copy link
Member Author

sgiehl commented Nov 20, 2016

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.

@sgiehl sgiehl force-pushed the pluginarchivnovisits branch from 2993d87 to 44e90ee Nov 20, 2016
@sgiehl sgiehl force-pushed the pluginarchivnovisits branch 2 times, most recently from 49ec4fc to f822589 Dec 12, 2016
@sgiehl sgiehl force-pushed the pluginarchivnovisits branch 4 times, most recently from 7813e32 to be4b4c1 Dec 27, 2016
@sgiehl sgiehl force-pushed the pluginarchivnovisits branch 2 times, most recently from 8ddc899 to 4c55659 Jan 6, 2017
@mattab

This comment has been minimized.

Copy link
Member

mattab commented Jan 8, 2017

@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.

Copy link
Member

mattab left a comment

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

@@ -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.

This comment has been minimized.

Copy link
@mattab

mattab Jan 8, 2017

Member

Could we maybe rename the method from shouldRunWithoutVisits to shouldRunEvenWhenNoVisit ?

This comment has been minimized.

Copy link
@mattab

mattab Jan 8, 2017

Member

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)

This comment has been minimized.

Copy link
@sgiehl

sgiehl Jan 8, 2017

Author Member

Sure, I'll change that

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

ArchiveWithNoVisitsTest_MockArchiver::$runWithoutVisits = true;

This comment has been minimized.

Copy link
@mattab

mattab Jan 8, 2017

Member

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

This comment has been minimized.

Copy link
@sgiehl

sgiehl Jan 8, 2017

Author Member

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

This comment has been minimized.

Copy link
@mattab

mattab Jan 9, 2017

Member

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 👍

This comment has been minimized.

Copy link
@sgiehl

sgiehl Jan 9, 2017

Author Member

I've splitted the tests in 9681533

@sgiehl sgiehl force-pushed the pluginarchivnovisits branch 2 times, most recently from 98bec9c to 60f495e Jan 8, 2017
@sgiehl sgiehl force-pushed the pluginarchivnovisits branch from 60f495e to f4f7b76 Jan 8, 2017
@mattab mattab merged commit d28efb2 into 3.x-dev Jan 9, 2017
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@mattab mattab deleted the pluginarchivnovisits branch Jan 9, 2017
sgiehl added a commit that referenced this pull request Jan 9, 2017
@@ -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).

This comment has been minimized.

Copy link
@mattab

mattab Jan 9, 2017

Member

I'll move this to the 3.0.1 section

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.