Merge "Merge global profiling keys"

i18n/en.json

@@ -165,7 +165,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",

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 );
 	}
 
 	/**

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

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

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,