Page MenuHomePhabricator

Unbreak EmergencyDisable and cleanup FilterProfiler API
Closed, ResolvedPublic

Description

Since rEABFbc9898f1a13a342ca44e928cee353ab5bf32e12e, the following is in AbuseFilterRunner::checkEmergencyDisable:

$filterProfile = $this->filterProfiler->getFilterProfile( $filter );
$matchCount = ( $filterProfile['matches'] ?? 0 ) + 1;

However, FilterProfiler::getFilterProfile returns stats in a different format:

public function getFilterProfile( $filter ) : array {
  $profile = $this->objectStash->get( $this->filterProfileKey( $filter ) );
  if ( $profile === false ) {
    return [ 0, 0, 0.0, 0.0 ];
  }
  $curCount = $profile['count'];
  $curTotalTime = $profile['total-time'];
  $curTotalConds = $profile['total-cond'];
  // Return in milliseconds, rounded to 2dp
  $avgTime = round( $curTotalTime / $curCount, 2 );
  $avgCond = round( $curTotalConds / $curCount, 1 );
  return [ $curCount, $profile['matches'], $avgTime, $avgCond ];
}

This probably broke EmergencyDisable feature. Since $matchCount is always 1 and AbuseFilterEmergencyDisableCount is (by default) 2, it would never throttle a filter. (Which maybe is better than every changed filter.)

Hotfix: change $filterProfile['matches'] to $filterProfile[1].
Further changes:

  • Make FilterProfiler::getFilterProfile return stats unchanged and move its logic to AbuseFilterViewEdit (where all the dividing and rounding can be done). Now it's inconsistent with its sibling getGroupProfile method and this probably was the source of confusion behind the regression.
  • EmergencyWatcher service & better test coverage (work in progress).

Event Timeline

Change 636612 had a related patch set uploaded (by Matěj Suchánek; owner: Matěj Suchánek):
[mediawiki/extensions/AbuseFilter@master] Unbreak EmergencyDisable

https://gerrit.wikimedia.org/r/636612

Change 636619 had a related patch set uploaded (by Matěj Suchánek; owner: Matěj Suchánek):
[mediawiki/extensions/AbuseFilter@master] Cleanup FilterProfiler API

https://gerrit.wikimedia.org/r/636619

For a future change, it might help to add @phan-return annotations to methods returning arrays with a specific shape.

Change 636612 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] Unbreak EmergencyDisable

https://gerrit.wikimedia.org/r/636612

Change 636619 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] Cleanup FilterProfiler API

https://gerrit.wikimedia.org/r/636619