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

Add logging to ArchivePurger service, task + Scheduler class & modify CronArchive so scheduled Task logs are displayed. #7602

Merged
merged 18 commits into from Apr 17, 2015

Conversation

diosmosis
Copy link
Member

This pull request modifies CronArchive so scheduled task logging output displays in core:archive output, and adds logging to ArchivePurger + other classes.

We display scheduled task output in the core:archive process by going through the Request class instead of climulti:request.

Example scheduled task output:

INFO CoreConsole[2015-04-01 01:33:19] ---------------------------
INFO CoreConsole[2015-04-01 01:33:19] SCHEDULED TASKS
INFO CoreConsole[2015-04-01 01:33:19] Starting Scheduled tasks... 
INFO CoreAdminHome[2015-04-01 01:33:19] Scheduler: executing task Piwik\Plugins\CoreAdminHome\Tasks.purgeOutdatedArchives...
INFO CoreAdminHome[2015-04-01 01:33:19] Purging archives in 96 archive tables.
INFO CoreAdminHome[2015-04-01 01:33:19] No outdated archives found in archive numeric table for 2005-01-15.
INFO CoreAdminHome[2015-04-01 01:33:19] Purged 0 range archive rows from archive_numeric_2005_01 & archive_blob_2005_01.
INFO CoreAdminHome[2015-04-01 01:33:19] No outdated archives found in archive numeric table for 2006-01-15.
INFO CoreAdminHome[2015-04-01 01:33:19] Purged 0 range archive rows from archive_numeric_2006_01 & archive_blob_2006_01.
INFO CoreAdminHome[2015-04-01 01:33:19] No outdated archives found in archive numeric table for 2007-01-15.
INFO CoreAdminHome[2015-04-01 01:33:19] Scheduler: finished executing task Piwik\Plugins\CoreAdminHome\Tasks.purgeOutdatedArchives. Time elapsed: 0.114s
INFO CoreConsole[2015-04-01 01:33:19] done
INFO CoreConsole[2015-04-01 01:33:19] ---------------------------

Refs #7181

diosmosis added 2 commits March 31, 2015 18:37
…heduled tasks + Scheduler, modify CronArchive to run scheduled tasks within same process as core:archive so logs are visible in core:archive output.
@diosmosis diosmosis added the Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc. label Apr 1, 2015
@diosmosis diosmosis added this to the Piwik 2.13.0 milestone Apr 1, 2015
@diosmosis diosmosis added the Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. label Apr 1, 2015
@diosmosis
Copy link
Member Author

Remembered the CronArchive method is used within tracker, need to revise how tracker runs scheduled tasks.

@mattab
Copy link
Member

mattab commented Apr 1, 2015

the CronArchive method is used within tracker

I'm wondering, do we have a test case for this? if not, it would be useful to add one 👍

}
return $deletedCount;
Copy link
Member

Choose a reason for hiding this comment

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

Do you mind adding an integration test for this? Would be nice to have a test for this. Not only for the return value but also whether it actually deletes the archives.

Copy link
Member Author

Choose a reason for hiding this comment

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

Modified the existing ArchivePurgerTest to test for deleted row counts.

@tsteur
Copy link
Member

tsteur commented Apr 7, 2015

Nice! Do those kinda changes affect performance in any way?

diosmosis added 5 commits April 7, 2015 14:17
…cheduled tasks through tracker including test that ensures tracker won't fail if scheduled tasks fail, allow tests to override container config by setting a property in the testing environment file, and properly use TestingEnvironment in RequestCommand.
…rger doesn't log anything unless verbose output is requested.
@diosmosis
Copy link
Member Author

@tsteur Re: performance, I tested using a dump of demo2's DB (which admittedly wouldn't have too much to purge) and there was no discernible difference w/ logging added.

@diosmosis diosmosis 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 Apr 9, 2015
@diosmosis
Copy link
Member Author

Ready for another review, and then possibly merge. Now includes system test to ensure scheduled tasks are run via tracker correctly and w/o failing the tracker.

diosmosis added 2 commits April 15, 2015 19:26
Conflicts:
	core/CronArchive.php
Conflicts:
	tests/UI/expected-ui-screenshots
@mnapoli
Copy link
Contributor

mnapoli commented Apr 16, 2015

I have checked out the branch and given it a try, the amount of output when running the scheduled tasks is quite high. For example:

INFO CoreAdminHome[2015-04-16 03:56:52] Scheduler: finished executing task Piwik\Plugins\CoreAdminHome\Tasks.purgeOutdatedArchives. Time elapsed: 0.219s
INFO CoreAdminHome[2015-04-16 03:56:52] Scheduler: executing task Piwik\Plugins\ExamplePlugin\Tasks.myTaskWithParam_anystring...
INFO CoreAdminHome[2015-04-16 03:56:52] Scheduler: finished executing task Piwik\Plugins\ExamplePlugin\Tasks.myTaskWithParam_anystring. Time elapsed: 0.000s
INFO CoreAdminHome[2015-04-16 03:56:52] Scheduler: executing task Piwik\Plugins\ExamplePlugin\Tasks.myTask...
INFO CoreAdminHome[2015-04-16 03:56:52] Scheduler: finished executing task Piwik\Plugins\ExamplePlugin\Tasks.myTask. Time elapsed: 0.000s
INFO CoreAdminHome[2015-04-16 03:56:52] Scheduler: executing task Piwik\Plugins\ExamplePlugin\Tasks.myTask...
INFO CoreAdminHome[2015-04-16 03:56:52] Scheduler: finished executing task Piwik\Plugins\ExamplePlugin\Tasks.myTask. Time elapsed: 0.000s
INFO CoreAdminHome[2015-04-16 03:56:52] Scheduler: executing task Piwik\Plugins\ExamplePlugin\Tasks.myTask...
INFO CoreAdminHome[2015-04-16 03:56:52] Scheduler: finished executing task Piwik\Plugins\ExamplePlugin\Tasks.myTask. Time elapsed: 0.000s
INFO CoreAdminHome[2015-04-16 03:56:52] Scheduler: executing task Piwik\Plugins\ExamplePlugin\Tasks.myTask...
INFO CoreAdminHome[2015-04-16 03:56:52] Scheduler: finished executing task Piwik\Plugins\ExamplePlugin\Tasks.myTask. Time elapsed: 0.000s
INFO CoreAdminHome[2015-04-16 03:56:52] Scheduler: executing task Piwik\Plugins\ExamplePlugin\Tasks.myTaskWithParam_anystring...
INFO CoreAdminHome[2015-04-16 03:56:52] Scheduler: finished executing task Piwik\Plugins\ExamplePlugin\Tasks.myTaskWithParam_anystring. Time elapsed: 0.000s
[...]
INFO CoreAdminHome[2015-04-16 03:58:37] done
Piwik\Plugins\CoreAdminHome\Tasks.purgeOutdatedArchives - Time elapsed: 0.219s
Piwik\Plugins\ExamplePlugin\Tasks.myTaskWithParam_anystring - Time elapsed: 0.000s
Piwik\Plugins\ExamplePlugin\Tasks.myTask - Time elapsed: 0.000s
Piwik\Plugins\ExamplePlugin\Tasks.myTask - Time elapsed: 0.000s
Piwik\Plugins\ExamplePlugin\Tasks.myTask - Time elapsed: 0.000s
Piwik\Plugins\ExamplePlugin\Tasks.myTask - Time elapsed: 0.000s
Piwik\Plugins\ExamplePlugin\Tasks.myTaskWithParam_anystring - Time elapsed: 0.000s
Piwik\Plugins\ScheduledReports\API.sendReport_1 - Time elapsed: 103.436s
Piwik\Plugins\CustomAlerts\Tasks.runAlertsDaily_16 - Time elapsed: 0.119s
Piwik\Plugins\CustomAlerts\Tasks.runAlertsWeekly_16 - Time elapsed: 0.002s
Piwik\Plugins\CustomAlerts\Tasks.runAlertsMonthly_16 - Time elapsed: 0.003s
Piwik\Plugins\TrafficMonitor\Tasks.doCacheTotalActionsNbFromRawData - Time elapsed: 0.962s
Piwik\Plugins\CoreAdminHome\Tasks.purgeInvalidatedArchives - Time elapsed: 0.002s
Piwik\Plugins\PrivacyManager\Tasks.deleteReportData - Time elapsed: 0.004s
Piwik\Plugins\PrivacyManager\Tasks.deleteLogData - Time elapsed: 0.001s
Piwik\Plugins\CorePluginsAdmin\Tasks.clearAllCacheEntries - Time elapsed: 0.001s
Piwik\Plugins\CorePluginsAdmin\Tasks.sendNotificationIfUpdatesAvailable - Time elapsed: 0.001s
Piwik\Plugins\CoreAdminHome\Tasks.optimizeArchiveTable - Time elapsed: 0.028s
Piwik\Plugins\ExamplePlugin\Tasks.myTask - Time elapsed: 0.000s
Piwik\Plugins\UserCountry\GeoIPAutoUpdater.update - Time elapsed: 0.011s
Piwik\Plugins\CoreUpdater\Tasks.sendNotificationIfUpdateAvailable - Time elapsed: 0.001s

Would it be better to remove the finished executing ... kind of messages? E.g. only say executing task … and not say when it's finished (because it will be obvious from the output).

Or else get rid of the summary at the end (which only displays the time elapsed) and show the time elapsed when printing finished executing task ...?

Except that 👍 for merge.

$logger = null;
if ($output->getVerbosity() <= OutputInterface::VERBOSITY_NORMAL) {
$logger = new NullLogger();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this? The -q flag should automatically set the error level at "error".

Copy link
Member Author

Choose a reason for hiding this comment

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

During normal output (in addition to when -q is used), we don't want the ArchivePurger logs to output in this command, but we want it to appear in scheduled task output.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah yes I think we discussed that in slack! Could you maybe add a comment to explain it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a comment.

@diosmosis
Copy link
Member Author

Or else get rid of the summary at the end (which only displays the time elapsed) and show the time elapsed when printing finished executing task ...?

You're right, don't need the summary anymore, will remove it.

mnapoli added a commit that referenced this pull request Apr 17, 2015
Add logging to ArchivePurger service, task + Scheduler class & modify CronArchive so scheduled Task logs are displayed.
@mnapoli mnapoli merged commit b4ac2e8 into master Apr 17, 2015
@mnapoli mnapoli deleted the archive_purger_logs branch April 17, 2015 00:48
@diosmosis diosmosis removed the Needs Review PRs that need a code review label Apr 17, 2015
diosmosis pushed a commit that referenced this pull request Apr 17, 2015
@mattab
Copy link
Member

mattab commented Apr 19, 2015

only say executing task … and not say when it's finished (because it will be obvious from the output).

it looks like the output was not simplfied as much as possible so created follow up issue #7705

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants