From d022377578b84aebbcab0714ab80b6d0fbb9cab8 Mon Sep 17 00:00:00 2001 From: rarohde Date: Tue, 31 Mar 2015 21:51:16 -0700 Subject: [PATCH] Merge global profiling keys The last step of the profiling overhaul. See T53294 for the original description by Dragons flight. Note: Here I'm adding a FixMe for a problem which already exists in the code and the child patch will fix it. Bug: T53294 Depends-On: I2d8c8f8278073a9420e3eb373fb89a655925618a Change-Id: Ib12e072a245fcad93c6c6bd452041f3441f68bb7 --- i18n/en.json | 2 +- includes/AbuseFilter.php | 23 ++---- includes/AbuseFilterRunner.php | 77 ++++++++++++------- includes/Views/AbuseFilterViewList.php | 32 ++++---- tests/phpunit/AbuseFilterConsequencesTest.php | 12 +-- 5 files changed, 83 insertions(+), 63 deletions(-) diff --git a/i18n/en.json b/i18n/en.json index 17d9c3443..abed1e015 100644 --- a/i18n/en.json +++ b/i18n/en.json @@ -164,7 +164,7 @@ "abusefilter-reautoconfirm-none": "That user has not had {{GENDER:$1|his|her|their}} autoconfirmed status suspended.", "abusefilter-reautoconfirm-notallowed": "You are not allowed to restore autoconfirmed status.", "abusefilter-reautoconfirm-done": "Account's autoconfirmed status has been restored", - "abusefilter-status": "Of the last $1 {{PLURAL:$1|action|actions}}, $2 ($3%) {{PLURAL:$2|has|have}} reached the condition limit of $4, and $5 ($6%) {{PLURAL:$5|has|have}} matched one of the filters currently enabled.", + "abusefilter-status": "Of the last $1 {{PLURAL:$1|action|actions}}, $2 ($3%) {{PLURAL:$2|has|have}} reached the condition limit of $4, and $5 ($6%) {{PLURAL:$5|has|have}} matched at least one of the filters currently enabled.", "abusefilter-edit": "Editing abuse filter", "abusefilter-edit-subtitle": "Editing filter $1", "abusefilter-edit-subtitle-new": "Creating filter", diff --git a/includes/AbuseFilter.php b/includes/AbuseFilter.php index 0808d77e5..41a0eda29 100644 --- a/includes/AbuseFilter.php +++ b/includes/AbuseFilter.php @@ -995,13 +995,6 @@ class AbuseFilter { return $value[$group] ?? $value['default']; } - /** - * @return string - */ - public static function filterLimitReachedKey() { - return wfMemcKey( 'abusefilter', 'stats', 'overflow' ); - } - /** * Get the memcache access key used to store per-filter profiling data. * @@ -1014,18 +1007,14 @@ class AbuseFilter { } /** - * @param string $group The filter's group (as defined in $wgAbuseFilterValidGroups) + * Memcache access key used to store overall profiling data for rule groups + * + * @param string $group * @return string */ - public static function filterUsedKey( $group ) { - return wfMemcKey( 'abusefilter', 'stats', 'total', $group ); - } - - /** - * @return string - */ - public static function filterMatchesKey() { - return wfMemcKey( 'abusefilter', 'stats', 'matches', null ); + public static function filterProfileGroupKey( $group ) { + $cache = MediaWikiServices::getInstance()->getMainWANObjectCache(); + return $cache->makeKey( 'abusefilter-profile', 'group', $group ); } /** diff --git a/includes/AbuseFilterRunner.php b/includes/AbuseFilterRunner.php index a0d56f1bb..5029d803f 100644 --- a/includes/AbuseFilterRunner.php +++ b/includes/AbuseFilterRunner.php @@ -156,7 +156,7 @@ class AbuseFilterRunner { $result['runtime'] ); $this->recordPerFilterProfiling( $result['profiling'] ); - $this->recordStats( $result['condCount'] ); + $this->recordStats( $result['condCount'], $result['runtime'], (bool)$matchedFilters ); if ( count( $matchedFilters ) === 0 ) { return Status::newGood(); @@ -348,6 +348,7 @@ class AbuseFilterRunner { } } + // Tag the action if the condition limit was hit if ( $this->parser->getCondCount() > $wgAbuseFilterConditionLimit ) { $actionID = $this->getTaggingID(); AbuseFilter::bufferTagsToSetByAction( [ $actionID => [ 'abusefilter-condition-limit' ] ] ); @@ -477,34 +478,62 @@ class AbuseFilterRunner { * Update global statistics * * @param int $condsUsed The amount of used conditions + * @param float $totalTime Time taken, in milliseconds + * @param bool $anyMatch Whether at least one filter matched the action */ - protected function recordStats( $condsUsed ) { - global $wgAbuseFilterConditionLimit, $wgAbuseFilterProfileActionsCap; - + protected function recordStats( $condsUsed, $totalTime, $anyMatch ) { + $profileKey = AbuseFilter::filterProfileGroupKey( $this->group ); $stash = MediaWikiServices::getInstance()->getMainObjectStash(); - $overflowKey = AbuseFilter::filterLimitReachedKey(); - $totalKey = AbuseFilter::filterUsedKey( $this->group ); + // Note: All related data is stored in a single memcache entry and updated via merge() + // to avoid race conditions where partial updates on competing instances corrupt the data. + $stash->merge( + $profileKey, + function ( $cache, $key, $profile ) use ( $condsUsed, $totalTime, $anyMatch ) { + global $wgAbuseFilterConditionLimit, $wgAbuseFilterProfileActionsCap; - $total = $stash->get( $totalKey ); + if ( $profile === false || $profile['total'] > $wgAbuseFilterProfileActionsCap ) { + // This is for if the total doesn't exist, or has gone past $wgAbuseFilterProfileActionsCap. + // Recreate all the keys at the same time, so they expire together. - $storagePeriod = AbuseFilter::$statsStoragePeriod; + $profile = [ + // Total number of actions observed + 'total' => 0, + // Number of actions ending by exceeding condition limit + 'overflow' => 0, + // Total time of execution of all observed actions + 'total-time' => 0, + // Total number of conditions from all observed actions + 'total-cond' => 0, + // Total number of filters matched + 'matches' => 0 + ]; - if ( !$total || $total > $wgAbuseFilterProfileActionsCap ) { - // This is for if the total doesn't exist, or has gone past the limit. - // Recreate all the keys at the same time, so they expire together. - $stash->set( $totalKey, 0, $storagePeriod ); - $stash->set( $overflowKey, 0, $storagePeriod ); + // @fixme We should also call resetFilterProfile, but this isn't the right place: + // it should probably be done before updating any profiling data, for instance + // before calling recordRuntimeProfilingResult. Note that resetting + // it for filters passed in here is enough, as profiling for other (=disabled) filters + // will be reset upon re-enabling them. + } - $stash->set( AbuseFilter::filterMatchesKey(), 0, $storagePeriod ); - } + $profile['total']++; + $profile['total-time'] += $totalTime; + $profile['total-cond'] += $condsUsed; - $stash->incr( $totalKey ); + // Increment overflow counter, if our condition limit overflowed + if ( $condsUsed > $wgAbuseFilterConditionLimit ) { + $profile['overflow']++; + } - // Increment overflow counter, if our condition limit overflowed - if ( $condsUsed > $wgAbuseFilterConditionLimit ) { - $stash->incr( $overflowKey ); - } + // Increment counter by 1 if there was at least one match + if ( $anyMatch ) { + $profile['matches']++; + } + + return $profile; + }, + AbuseFilter::$statsStoragePeriod + ); } /** @@ -1105,11 +1134,6 @@ class AbuseFilterRunner { // To distinguish from stuff stored directly $varDump = "stored-text:$varDump"; - $stash = MediaWikiServices::getInstance()->getMainObjectStash(); - - // Increment trigger counter - $stash->incr( AbuseFilter::filterMatchesKey() ); - $localLogIDs = []; global $wgAbuseFilterNotifications, $wgAbuseFilterNotificationsPrivate; foreach ( $logRows as $data ) { @@ -1237,7 +1261,8 @@ class AbuseFilterRunner { $stash = MediaWikiServices::getInstance()->getMainObjectStash(); // @ToDo this is an amount between 1 and AbuseFilterProfileActionsCap, which means that the // reliability of this number may strongly vary. We should instead use a fixed one. - $totalActions = $stash->get( AbuseFilter::filterUsedKey( $this->group ) ); + $groupProfile = $stash->get( AbuseFilter::filterProfileGroupKey( $this->group ) ); + $totalActions = $groupProfile['total']; foreach ( $filters as $filter ) { $threshold = AbuseFilter::getEmergencyValue( 'threshold', $this->group ); diff --git a/includes/Views/AbuseFilterViewList.php b/includes/Views/AbuseFilterViewList.php index 247701bd1..aef2e0697 100644 --- a/includes/Views/AbuseFilterViewList.php +++ b/includes/Views/AbuseFilterViewList.php @@ -276,29 +276,35 @@ class AbuseFilterViewList extends AbuseFilterView { } /** - * Show stats + * Generates a summary of filter activity using the internal statistics. */ public function showStatus() { $stash = MediaWikiServices::getInstance()->getMainObjectStash(); - $overflow_count = (int)$stash->get( AbuseFilter::filterLimitReachedKey() ); - $match_count = (int)$stash->get( AbuseFilter::filterMatchesKey() ); - $total_count = 0; + + $totalCount = 0; + $matchCount = 0; + $overflowCount = 0; foreach ( $this->getConfig()->get( 'AbuseFilterValidGroups' ) as $group ) { - $total_count += (int)$stash->get( AbuseFilter::filterUsedKey( $group ) ); + $profile = $stash->get( AbuseFilter::filterProfileGroupKey( $group ) ); + if ( $profile !== false ) { + $totalCount += $profile[ 'total' ]; + $overflowCount += $profile[ 'overflow' ]; + $matchCount += $profile[ 'matches' ]; + } } - if ( $total_count > 0 ) { - $overflow_percent = sprintf( "%.2f", 100 * $overflow_count / $total_count ); - $match_percent = sprintf( "%.2f", 100 * $match_count / $total_count ); + if ( $totalCount > 0 ) { + $overflowPercent = round( 100 * $overflowCount / $totalCount, 2 ); + $matchPercent = round( 100 * $matchCount / $totalCount, 2 ); $status = $this->msg( 'abusefilter-status' ) ->numParams( - $total_count, - $overflow_count, - $overflow_percent, + $totalCount, + $overflowCount, + $overflowPercent, $this->getConfig()->get( 'AbuseFilterConditionLimit' ), - $match_count, - $match_percent + $matchCount, + $matchPercent )->parse(); $status = Xml::tags( 'div', [ 'class' => 'mw-abusefilter-status' ], $status ); diff --git a/tests/phpunit/AbuseFilterConsequencesTest.php b/tests/phpunit/AbuseFilterConsequencesTest.php index 05a595489..8a0558168 100644 --- a/tests/phpunit/AbuseFilterConsequencesTest.php +++ b/tests/phpunit/AbuseFilterConsequencesTest.php @@ -1735,9 +1735,8 @@ class AbuseFilterConsequencesTest extends MediaWikiTestCase { * @param array $actionParams Details of the action we need to execute to trigger filters * @param array $expectedGlobal Expected global stats * @param array $expectedPerFilter Expected stats for every created filter - * @covers AbuseFilter::filterMatchesKey - * @covers AbuseFilter::filterUsedKey - * @covers AbuseFilter::filterLimitReachedKey + * @covers AbuseFilter::filterProfileKey + * @covers AbuseFilter::filterProfileGroupKey * @covers AbuseFilter::getFilterProfile * @covers AbuseFilterRunner::checkAllFilters * @covers AbuseFilterRunner::recordStats @@ -1762,10 +1761,11 @@ class AbuseFilterConsequencesTest extends MediaWikiTestCase { $stash = MediaWikiServices::getInstance()->getMainObjectStash(); // Global stats shown on the top of Special:AbuseFilter + $globalStats = $stash->get( AbuseFilter::filterProfileGroupKey( 'default' ) ); $actualGlobalStats = [ - 'totalMatches' => $stash->get( AbuseFilter::filterMatchesKey() ), - 'totalActions' => $stash->get( AbuseFilter::filterUsedKey( 'default' ) ), - 'totalOverflows' => $stash->get( AbuseFilter::filterLimitReachedKey() ) + 'totalMatches' => $globalStats['matches'], + 'totalActions' => $globalStats['total'], + 'totalOverflows' => $globalStats['overflow'] ]; $this->assertSame( $expectedGlobal,