Fixes #6785, #7180 add log delete command and make log deletion delete only based on time and not idvisit. #7887

diosmosis · 2015-05-12T04:56:39Z

This PR fixes #6785 and #7180. It introduces a new logs:delete command and makes all visit deletion (including the log data purger used by PrivacyManager) visit based on required criteria instead of the max idvisit.

List of changes:

New LogDeleter service that is used by logs:delete and LogDataPurger in PrivacyManager.
New method in RawLogDao that iterates over log tables based on a set of criteria. The criteria is an array describing a list of SQL conditions. I think this should be replaced eventually by a segment string, since they do the same thing, but that would require more platform infrastructure than Piwik currently has.
The new method in RawLogDao is used by the usercountry:attribute command. The looping logic was also moved to VisitorGeolocator, a late finishing touch for the command.
Removed the testing hack from LogDataPurger. Originally, to test a concurrency issue, two events were fired in LogDataPurger. Now, we use DI.
I added the ability to override DI config in IntegrationTestCase descendants.
I moved log inserting test code from VisitorGeolocatorTest to a new LogHelper class (in Piwik\Tests\Framework) and re-used for testing LogDeleter.
Moved the unused log action deletion methods to RawLogDao.

TODO:

tests for new VisitorGelocator method
tests for LogPurger service
~~[ ] tests for new RawLogDao methods~~
benchmark command + log purging on large instance
add test for Delete old visitor logs uses idvisit instead of the date #7180

mattab · 2015-05-27T02:48:10Z

@diosmosis Sorry for the delay in getting to review this. This Pull request is high value and fixes a major bug that can lead to data loss 👍

Can you rebase it (I think work done in #7964 is relevant to this PR). After rebase let me know I will review and possibly merge. thanks

diosmosis · 2015-05-27T03:52:55Z

@mattab The rebase was very complicated, tests may not pass (will have to check tomorrow). Please do a thorough review.

…rator, and add options to delete:logs command.

…e in usercountry:attribute command. usercountry:attribute tests pass.

…n will cascade. Implement LogPurger service using different strategy from LogDataPurger (which can result in error).

…ManagerTest to pass.

…ony issue w/ using mock interactive input streams. If the posix_isatty function exists, the interactive flag set by ApplicationTester will be overridden by Application::configureIO.

…er plugin, Rename LogPurger to LogDeleter, add test for VisitorGeolocator::reattributeVisits and add empty tests for LogDeleter service.

…imit.

…user desires it).

…tion step size to constructor so it can potentially be conifgured via DI, remove some unneeded TODOs.

…LogHelper and add quick class docs to LogHelper.

…gDao::insertActionsToKeep.

…nts. Also allow IntegrationTests to override DI config for tests.

…sits are tracked in the past.

…dTestCase.

mattab · 2015-06-16T03:17:13Z

tests/PHPUnit/Framework/TestCase/IntegrationTestCase.php

@@ -86,6 +88,15 @@ public function setUp()
            self::restoreDbTables(self::$tableData);
        }

+        $extraDefinitions = $this->provideContainerConfig();


Is this change expected to be in this pull request (it seems unrelated). Either way, if the code is correct, it's fine to have it here, just double checking

…isplay when ALL websites logs are deleted, reuse OPTIMIZE table core logic

… query faster.

mattab · 2015-06-16T07:23:40Z

Looks good to me: cleaning up some tech debt, adding useful new command and fixing possibly major bug all at once. Took me long time to review it and test it but my tests worked 👍

Fixes #6785, #7180 add log delete command and make log deletion delete only based on time and not idvisit.

diosmosis added the Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. label May 12, 2015

diosmosis self-assigned this May 12, 2015

diosmosis added this to the 2.14.0 milestone May 12, 2015

diosmosis force-pushed the 6785_log_delete_command branch from 4a37111 to 77c3b3f Compare May 13, 2015 07:59

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 May 15, 2015

diosmosis force-pushed the 6785_log_delete_command branch from 5dfd917 to 81204da Compare May 27, 2015 03:52

diosmosis added 21 commits June 10, 2015 23:42

Refs #7180, create initial (not working) logs:delete command.

0ae985f

Quick POC change to usercountry:attribute to use non-existant log ite…

58c7f32

…rator, and add options to delete:logs command.

Use forEach function instead of iterator to loop over logs and replac…

076ca40

…e in usercountry:attribute command. usercountry:attribute tests pass.

Remove ability to specify table in logs:delete command, visit deletio…

b8a5e03

…n will cascade. Implement LogPurger service using different strategy from LogDataPurger (which can result in error).

Use LogPurger in PrivacyManager's LogDataPurger class and get Privacy…

b48e09a

…ManagerTest to pass.

Move deleteUnusedLogAction logic to RawLogDao class.

30ac243

Remove unneeded private method from DeleteLogs.php.

349f234

Adding integration test for logs:delete command. Includes fix to symf…

5a2ac9f

…ony issue w/ using mock interactive input streams. If the posix_isatty function exists, the interactive flag set by ApplicationTester will be overridden by Application::configureIO.

Refactoring logs:delete command, move deleteVisitsFor to LogPurger.

720cb79

Remove unused use statement.

d822c3c

Move usercountry:attribute iteration logic to VisitorGeolocator plugin.

97abd79

Use LogPurger::deleteVisitsFor in LogDataPurger class in PrivacyManag…

1223d1c

…er plugin, Rename LogPurger to LogDeleter, add test for VisitorGeolocator::reattributeVisits and add empty tests for LogDeleter service.

Move log inserting logic to new test helper class, LogInserter.

98a7c11

Fill out parts of LogDeleterTest, not complete yet.

76a5a3b

Finish LogDeleterTest and get tests to pass.

148f59b

Refactor and document RawLogDao.

219046a

Document LogDeleter service and remove RawLogDao::getVisitsWithDatesL…

6b3bb8d

…imit.

Quick docs for DeleteLogs command + remove two TODOs in the class.

035db93

Allow logs:delete command to optimize tables after deleting logs (if …

22f5777

…user desires it).

Fill out some docs for LogDataPurger in PrivacyManager, add log itera…

b187a82

…tion step size to constructor so it can potentially be conifgured via DI, remove some unneeded TODOs.

Document VisitorGeolocator::reattributeVisits, rename LogInserter to …

92e1482

…LogHelper and add quick class docs to LogHelper.

diosmosis added 8 commits June 10, 2015 23:42

Remove RawLogDao::$selectSegmentSize and make it a parameter of RawLo…

b621564

…gDao::insertActionsToKeep.

Use mock RawLogDao instance in PrivacyManagerTest instead of test eve…

b0f4313

…nts. Also allow IntegrationTests to override DI config for tests.

Tweak output of optimize tables output in logs:delete command.

1bb0bfb

Move PrivacyManagerTest to PrivacyManager plugin as DataPurgingTest.php.

9cfde51

Fixing LogDeleterTest & Plugin\ManagerTest.

943fc36

Add default value for time_spent_ref_action in LogHelper test class.

e01af7d

Refs #7180, add test that checks logs are correctly purged even if vi…

794f056

…sits are tracked in the past.

Add comment describing why TestConsole class is used in ConsoleComman…

c94a8c2

…dTestCase.

diosmosis force-pushed the 6785_log_delete_command branch from 81204da to c94a8c2 Compare June 11, 2015 06:46

mattab assigned mattab and unassigned diosmosis Jun 11, 2015

mattab reviewed Jun 16, 2015
View reviewed changes

mattab added 2 commits June 16, 2015 16:19

Rename --site to --idsite, rename command to core:delete-logs-data, d…

6be8c17

…isplay when ALL websites logs are deleted, reuse OPTIMIZE table core logic

Use visit_last_action_time instead of visit_first_action_time to make…

13c4f6d

… query faster.

mattab mentioned this pull request Jun 16, 2015

How can we merge Pull Requests faster? #7953

Closed

mattab added 5 commits June 16, 2015 16:45

Merge branch 'master' into 6785_log_delete_command

1c615d3

Tests: command and parameter were renamed

85a936e

Add sanity check and assert in tests, to make trouble shooting easier

6772518

Minor: add comment, fix typo [skip-ci]

bb7ef1d

Removing un-needed code from test class

eed36ac

mattab pushed a commit that referenced this pull request Jun 16, 2015

Merge pull request #7887 from piwik/6785_log_delete_command

36529f5

Fixes #6785, #7180 add log delete command and make log deletion delete only based on time and not idvisit.

mattab merged commit 36529f5 into master Jun 16, 2015

diosmosis pushed a commit that referenced this pull request Jun 16, 2015

Refs #7887, fixing failed rebase or two.

62f170d

diosmosis deleted the 6785_log_delete_command branch June 16, 2015 07:51

mattab added the not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. label Jun 24, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #6785, #7180 add log delete command and make log deletion delete only based on time and not idvisit. #7887

Fixes #6785, #7180 add log delete command and make log deletion delete only based on time and not idvisit. #7887

diosmosis commented May 12, 2015

mattab commented May 27, 2015

diosmosis commented May 27, 2015

mattab Jun 16, 2015

mattab commented Jun 16, 2015

Fixes #6785, #7180 add log delete command and make log deletion delete only based on time and not idvisit. #7887

Fixes #6785, #7180 add log delete command and make log deletion delete only based on time and not idvisit. #7887

Conversation

diosmosis commented May 12, 2015

mattab commented May 27, 2015

diosmosis commented May 27, 2015

mattab Jun 16, 2015

Choose a reason for hiding this comment

mattab commented Jun 16, 2015