Move per-filter matches profiling to per-filter data

They're currently stored separately, so move matches count together with
other per-filter data to keep it consistent. This also removes a
parameter from filterMatchesKey, as it's not needed anymore.
Split from child patch by Dragons flight.

Bug: T53294
Depends-On: I8f47beb73cfc1b63c4b3c809fc6d65a1e66ee334
Change-Id: I2d8c8f8278073a9420e3eb373fb89a655925618a

includes/AbuseFilter.php

@@ -591,7 +591,7 @@ class AbuseFilter {
 			$curTotalTime = $profile['total-time'];
 			$curTotalConds = $profile['total-cond'];
 		} else {
-			return [ 0, 0 ];
+			return [ 0, 0, 0, 0 ];
 		}
 
 		// 1000 ms in a sec
@@ -602,7 +602,7 @@ class AbuseFilter {
 		$condProfile = ( $curTotalConds / $curCount );
 		$condProfile = round( $condProfile, 0 );
 
-		return [ $timeProfile, $condProfile ];
+		return [ $curCount, $profile['matches'], $timeProfile, $condProfile ];
 	}
 
 	/**
@@ -1014,7 +1014,7 @@ class AbuseFilter {
 	 */
 	public static function filterProfileKey( $filter ) {
 		$cache = MediaWikiServices::getInstance()->getMainWANObjectCache();
-		return $cache->makeKey( 'abusefilter-profile', 'v1', $filter );
+		return $cache->makeKey( 'abusefilter-profile', 'v2', $filter );
 	}
 
 	/**
@@ -1026,11 +1026,10 @@ class AbuseFilter {
 	}
 
 	/**
-	 * @param string|null $filter
 	 * @return string
 	 */
-	public static function filterMatchesKey( $filter = null ) {
-		return wfMemcKey( 'abusefilter', 'stats', 'matches', $filter );
+	public static function filterMatchesKey() {
+		return wfMemcKey( 'abusefilter', 'stats', 'matches', null );
 	}
 
 	/**

includes/AbuseFilterRunner.php

@@ -156,7 +156,7 @@ class AbuseFilterRunner {
 			$result['runtime']
 		);
 		$this->recordPerFilterProfiling( $result['profiling'] );
-		$this->recordStats( $result['matches'], $result['condCount'] );
+		$this->recordStats( $result['condCount'] );
 
 		if ( count( $matchedFilters ) === 0 ) {
 			return Status::newGood();
@@ -400,7 +400,7 @@ class AbuseFilterRunner {
 			if ( !$global ) {
 				// @todo Maybe add a parameter to recordProfilingResult to record global filters
 				// data separately (in the foreign wiki)
-				$this->recordProfilingResult( $filterID, $params['time'], $params['conds'] );
+				$this->recordProfilingResult( $filterID, $params['time'], $params['conds'], $params['result'] );
 			}
 
 			$runtime = $params['time'] * 1000;
@@ -416,10 +416,11 @@ class AbuseFilterRunner {
 	 * @param int $filter
 	 * @param float $time
 	 * @param int $conds
+	 * @param bool $matched
 	 */
-	protected function recordProfilingResult( $filter, $time, $conds ) {
+	protected function recordProfilingResult( $filter, $time, $conds, $matched ) {
 		// Defer updates to avoid massive (~1 second) edit time increases
-		DeferredUpdates::addCallableUpdate( function () use ( $filter, $time, $conds ) {
+		DeferredUpdates::addCallableUpdate( function () use ( $filter, $time, $conds, $matched ) {
 			$stash = MediaWikiServices::getInstance()->getMainObjectStash();
 			$profileKey = AbuseFilter::filterProfileKey( $filter );
 			$profile = $stash->get( $profileKey );
@@ -427,6 +428,10 @@ class AbuseFilterRunner {
 			if ( $profile !== false ) {
 				// Number of observed executions of this filter
 				$profile['count']++;
+				if ( $matched ) {
+					// Number of observed matches of this filter
+					$profile['matches']++;
+				}
 				// Total time spent on this filter from all observed executions
 				$profile['total-time'] += $time;
 				// Total number of conditions for this filter from all executions
@@ -434,6 +439,7 @@ class AbuseFilterRunner {
 			} else {
 				$profile = [
 					'count' => 1,
+					'matches' => (int)$matched,
 					'total-time' => $time,
 					'total-cond' => $conds
 				];
@@ -471,10 +477,9 @@ class AbuseFilterRunner {
 	/**
 	 * Update global statistics
 	 *
-	 * @param bool[] $filters
 	 * @param int $condsUsed The amount of used conditions
 	 */
-	protected function recordStats( array $filters, $condsUsed ) {
+	protected function recordStats( $condsUsed ) {
 		global $wgAbuseFilterConditionLimit, $wgAbuseFilterProfileActionsCap;
 
 		$stash = MediaWikiServices::getInstance()->getMainObjectStash();
@@ -492,11 +497,6 @@ class AbuseFilterRunner {
 			$stash->set( $totalKey, 0, $storagePeriod );
 			$stash->set( $overflowKey, 0, $storagePeriod );
 
-			foreach ( array_keys( $filters ) as $filter ) {
-				// @todo This should actually reset keys for all filters, and not only the ones
-				// passed in (which are the ones returned by checkAllFilters, i.e. only enabled filters)
-				$stash->set( AbuseFilter::filterMatchesKey( $filter ), 0, $storagePeriod );
-			}
 			$stash->set( AbuseFilter::filterMatchesKey(), 0, $storagePeriod );
 		}
 
@@ -1245,15 +1245,8 @@ class AbuseFilterRunner {
 			$hitCountLimit = AbuseFilter::getEmergencyValue( 'count', $this->group );
 			$maxAge = AbuseFilter::getEmergencyValue( 'age', $this->group );
 
-			$matchCount = $stash->get( AbuseFilter::filterMatchesKey( $filter ) );
-
-			// Handle missing keys...
-			if ( !$matchCount ) {
-				$stash->set( AbuseFilter::filterMatchesKey( $filter ), 1, AbuseFilter::$statsStoragePeriod );
-			} else {
-				$stash->incr( AbuseFilter::filterMatchesKey( $filter ) );
-			}
-			$matchCount++;
+			$filterProfile = $stash->get( AbuseFilter::filterProfileKey( $filter ) );
+			$matchCount = $filterProfile['matches'] ?? 1;
 
 			// Figure out if the filter is subject to being throttled.
 			$filterAge = wfTimestamp( TS_UNIX, AbuseFilter::getFilter( $filter )->af_timestamp );

includes/Views/AbuseFilterViewEdit.php

@@ -1,7 +1,5 @@
 <?php
 
-use MediaWiki\MediaWikiServices;
-
 class AbuseFilterViewEdit extends AbuseFilterView {
 	/**
 	 * @var stdClass|null An abuse_filter row describing a filter
@@ -245,15 +243,13 @@ class AbuseFilterViewEdit extends AbuseFilterView {
 
 		if ( $filter !== 'new' ) {
 			// Statistics
-			$stash = MediaWikiServices::getInstance()->getMainObjectStash();
-			$matches_count = (int)$stash->get( AbuseFilter::filterMatchesKey( $filter ) );
-			$total = (int)$stash->get( AbuseFilter::filterUsedKey( $row->af_group ) );
+			list( $totalCount, $matchesCount, $avgTime, $avgCond ) =
+				AbuseFilter::getFilterProfile( $filter );
 
-			if ( $total > 0 ) {
-				$matches_percent = sprintf( '%.2f', 100 * $matches_count / $total );
-				list( $timeProfile, $condProfile ) = AbuseFilter::getFilterProfile( $filter );
+			if ( $totalCount > 0 ) {
+				$matchesPercent = round( 100 * $matchesCount / $totalCount, 2 );
 				$fields['abusefilter-edit-status-label'] = $this->msg( 'abusefilter-edit-status' )
-					->numParams( $total, $matches_count, $matches_percent, $timeProfile, $condProfile )
+					->numParams( $totalCount, $matchesCount, $matchesPercent, $avgTime, $avgCond )
 					->parse();
 			}
 		}

tests/phpunit/AbuseFilterConsequencesTest.php

@@ -1741,7 +1741,6 @@ class AbuseFilterConsequencesTest extends MediaWikiTestCase {
 	 * @covers AbuseFilter::getFilterProfile
 	 * @covers AbuseFilterRunner::checkAllFilters
 	 * @covers AbuseFilterRunner::recordStats
-	 * @covers AbuseFilterRunner::checkEmergencyDisable
 	 * @dataProvider provideProfilingFilters
 	 */
 	public function testProfiling( $createIds, $actionParams, $expectedGlobal, $expectedPerFilter ) {
@@ -1776,10 +1775,11 @@ class AbuseFilterConsequencesTest extends MediaWikiTestCase {
 
 		// Per-filter stats shown on the top of Special:AbuseFilter/xxx
 		foreach ( $createIds as $id ) {
+			list( $totalActions, $matches, , $conds ) = AbuseFilter::getFilterProfile( $id );
 			$actualStats = [
-				'matches' => $stash->get( AbuseFilter::filterMatchesKey( $id ) ),
-				'actions' => $stash->get( AbuseFilter::filterUsedKey( 'default' ) ),
-				'averageConditions' => AbuseFilter::getFilterProfile( $id )[1]
+				'matches' => $matches,
+				'actions' => $totalActions,
+				'averageConditions' => $conds
 			];
 			$this->assertSame(
 				$expectedPerFilter[ $id ],