Thijs/wiki.techinc.nl
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

RateLimiter: collect statistics

Browse source 
To prepare for changes to how, when and where we check rate limits,
we should start collecting statistics on how limits are checked and
enforced in production. This will alert us to unintended effects of
the refactoring. In addition, it provides information on the impact
each limit rule has.

Bug: T310476
Change-Id: I9dfbf3847b73ab5f145722c45b93056408ad9444
This commit is contained in:
daniel 2023-06-18 14:31:18 +02:00
parent 81bda09d48
commit e70e811ee3
3 changed files with 110 additions and 2 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

23
includes/Permissions/RateLimiter.php
View file

@ -21,6 +21,7 @@
namespace MediaWiki\Permissions;
use CentralIdLookup;
use Liuggio\StatsdClient\Factory\StatsdDataFactoryInterface;
use MediaWiki\Config\ServiceOptions;
use MediaWiki\HookContainer\HookContainer;
use MediaWiki\HookContainer\HookRunner;
@ -28,6 +29,7 @@ use MediaWiki\Logger\LoggerFactory;
use MediaWiki\MainConfigNames;
use MediaWiki\User\UserFactory;
use MediaWiki\User\UserGroupManager;
use NullStatsdDataFactory;
use Psr\Log\LoggerInterface;
use Wikimedia\IPUtils;
use Wikimedia\WRStats\LimitCondition;
@ -65,6 +67,8 @@ class RateLimiter {
/** @var UserFactory */
private $userFactory;
private StatsdDataFactoryInterface $stats;
/**
* @internal
*/
@ -90,6 +94,7 @@ class RateLimiter {
HookContainer $hookContainer
) {
$this->logger = LoggerFactory::getInstance( 'ratelimit' );
$this->stats = new NullStatsdDataFactory();
$options->assertRequiredOptions( self::CONSTRUCTOR_OPTIONS );
$this->options = $options;
@ -102,6 +107,14 @@ class RateLimiter {
$this->rateLimits = $this->options->get( MainConfigNames::RateLimits );
}
public function setStats( StatsdDataFactoryInterface $stats ) {
$this->stats = $stats;
}
private function incrementStats( $name ) {
$this->stats->increment( "RateLimiter.$name" );
}
/**
* Is this user exempt from rate limiting?
*
@ -145,6 +158,7 @@ class RateLimiter {
$result = false;
$legacyUser = $this->userFactory->newFromUserIdentity( $user );
if ( !$this->hookRunner->onPingLimiter( $legacyUser, $action, $result, $incrBy ) ) {
$this->incrementStats( "limit.$action.result." . ( $result ? 'tripped_by_hook' : 'passed_by_hook' ) );
return $result;
}
@ -154,6 +168,7 @@ class RateLimiter {
// Some groups shouldn't trigger the ping limiter, ever
if ( $this->canBypass( $action ) && $this->isExempt( $subject ) ) {
$this->incrementStats( "limit.$action.result.exempt" );
return false;
}
@ -272,9 +287,15 @@ class RateLimiter {
'key' => $type
] + $loggerInfo
);
$this->incrementStats( "limit.$action.tripped_by.$type" );
}
return !$batchResult->isAllowed();
$allowed = $batchResult->isAllowed();
$this->incrementStats( "limit.$action.result." . ( $allowed ? 'passed' : 'tripped' ) );
return !$allowed;
}
private function canBypass( string $action ) {

6
includes/ServiceWiring.php
View file

@ -1650,7 +1650,7 @@ return [
},
'RateLimiter' => static function ( MediaWikiServices $services ): RateLimiter {
return new RateLimiter(
$rateLimiter = new RateLimiter(
new ServiceOptions( RateLimiter::CONSTRUCTOR_OPTIONS, $services->getMainConfig() ),
$services->getWRStatsFactory(),
$services->getCentralIdLookupFactory()->getNonLocalLookup(),
@ -1658,6 +1658,10 @@ return [
$services->getUserGroupManager(),
$services->getHookContainer()
);
$rateLimiter->setStats( $services->getStatsdDataFactory() );
return $rateLimiter;
},
'ReadOnlyMode' => static function ( MediaWikiServices $services ): ReadOnlyMode {

83
tests/phpunit/integration/includes/Permissions/RateLimiterTest.php
View file

@ -2,8 +2,11 @@
namespace MediaWiki\Tests\Integration\Permissions;
use BufferingStatsdDataFactory;
use CentralIdLookup;
use HashBagOStuff;
use Liuggio\StatsdClient\Entity\StatsdData;
use Liuggio\StatsdClient\Entity\StatsdDataInterface;
use MediaWiki\Config\ServiceOptions;
use MediaWiki\MainConfigNames;
use MediaWiki\Permissions\RateLimiter;
@ -75,7 +78,9 @@ class RateLimiterTest extends MediaWikiIntegrationTestCase {
$statsFactory = new WRStatsFactory( new BagOStuffStatsStore( $cache ) );
$stats = new BufferingStatsdDataFactory( 'test.' );
$limiter = $this->newRateLimiter( $limits, [], $statsFactory );
$limiter->setStats( $stats );
// Set up some fake users
$anon1 = $this->newFakeAnon( '1.2.3.4' );
@ -134,6 +139,16 @@ class RateLimiterTest extends MediaWikiIntegrationTestCase {
// For a user without a global ID, user-global acts as a local restriction
$this->assertFalse( $limiter->limit( $karaY1, 'move' ), 'Move by another user' );
$this->assertTrue( $limiter->limit( $karaY1, 'move' ), 'Second move by another user' );
// Check stats entries for conditions
$statsData = $stats->getData();
$this->assertStatsHasCount( 'test.RateLimiter.limit.edit.tripped_by.anon', 1, $statsData );
$this->assertStatsHasCount( 'test.RateLimiter.limit.purge.tripped_by.ip', 1, $statsData );
$this->assertStatsHasCount( 'test.RateLimiter.limit.purge.tripped_by.subnet', 1, $statsData );
$this->assertStatsHasCount( 'test.RateLimiter.limit.delete.tripped_by.ip_all', 1, $statsData );
$this->assertStatsHasCount( 'test.RateLimiter.limit.delete.tripped_by.subnet_all', 1, $statsData );
$this->assertStatsHasCount( 'test.RateLimiter.limit.rollback.tripped_by.user', 1, $statsData );
$this->assertStatsHasCount( 'test.RateLimiter.limit.move.tripped_by.user_global', 1, $statsData );
}
/**
@ -221,8 +236,11 @@ class RateLimiterTest extends MediaWikiIntegrationTestCase {
],
];
$stats = new BufferingStatsdDataFactory( 'test.' );
$user = $this->newFakeUser( 'Frank', '1.2.3.4', 111 );
$limiter = $this->newRateLimiter( $limits, [] );
$limiter->setStats( $stats );
// Hook leaves $result false
$this->setTemporaryHook(
@ -256,6 +274,13 @@ class RateLimiterTest extends MediaWikiIntegrationTestCase {
$limiter->limit( $user, 'FakeActionWithNoRateLimit' ),
'Actions with no rate limit set do not trip the rate limiter'
);
$statsData = $stats->getData();
$this->assertStatsHasCount( 'test.RateLimiter.limit.edit.result.passed_by_hook', 1, $statsData );
$this->assertStatsHasCount( 'test.RateLimiter.limit.edit.result.tripped_by_hook', 1, $statsData );
$this->assertStatsNotHasCount( 'test.RateLimiter.limit.edit.result.passed', $statsData );
$this->assertStatsNotHasCount( 'test.RateLimiter.limit.edit.result.tripped', $statsData );
}
public static function provideIsExempt() {
@ -360,10 +385,18 @@ class RateLimiterTest extends MediaWikiIntegrationTestCase {
$user2 = $this->newFakeUser( 'User2', '127.0.0.1', 2, true );
$user3 = $this->newFakeUser( 'User3', '127.0.0.1', 3, true );
$stats = new BufferingStatsdDataFactory( 'test.' );
$limiter = $this->newRateLimiter( $limits, [] );
$limiter->setStats( $stats );
$this->assertFalse( $limiter->limit( $user1, 'edit' ) );
$this->assertFalse( $limiter->limit( $user2, 'edit' ) );
$this->assertTrue( $limiter->limit( $user3, 'edit' ) );
$statsData = $stats->getData();
$this->assertStatsHasCount( 'test.RateLimiter.limit.edit.result.passed', 1, $statsData );
$this->assertStatsHasCount( 'test.RateLimiter.limit.edit.result.tripped', 1, $statsData );
$this->assertStatsHasCount( 'test.RateLimiter.limit.edit.tripped_by.ip', 1, $statsData );
}
/**
@ -389,12 +422,20 @@ class RateLimiterTest extends MediaWikiIntegrationTestCase {
[ RateLimitSubject::EXEMPT => true ]
);
$stats = new BufferingStatsdDataFactory( 'test.' );
$limiter = $this->newRateLimiter( $limits, [] );
$limiter->setStats( $stats );
$this->assertFalse( $limiter->limit( $user, 'edit' ) );
$this->assertFalse( $limiter->limit( $user, 'delete' ) );
$this->assertFalse( $limiter->limit( $user, 'edit' ), 'bypass should be granted' );
$this->assertTrue( $limiter->limit( $user, 'delete' ), 'bypass should be denied' );
$statsData = $stats->getData();
$this->assertStatsHasCount( 'test.RateLimiter.limit.edit.result.exempt', 1, $statsData );
$this->assertStatsNotHasCount( 'test.RateLimiter.limit.delete.result.exempt', $statsData );
$this->assertStatsHasCount( 'test.RateLimiter.limit.delete.result.tripped', 1, $statsData );
}
/**
@ -414,10 +455,52 @@ class RateLimiterTest extends MediaWikiIntegrationTestCase {
$user = $this->getTestUser( [ 'autoconfirmed' ] )->getUser();
$user = new RateLimitSubject( $user, '127.0.0.1', [] );
$stats = new BufferingStatsdDataFactory( 'test.' );
$limiter = $this->newRateLimiter( $limits, [] );
$limiter->setStats( $stats );
$this->assertFalse( $limiter->limit( $user, 'edit' ) );
$this->assertFalse( $limiter->limit( $user, 'edit' ), 'limit for autoconfirmed used' );
$this->assertTrue( $limiter->limit( $user, 'edit' ), 'limit for autoconfirmed exceeded' );
$statsData = $stats->getData();
$this->assertStatsHasCount( 'test.RateLimiter.limit.edit.result.passed', 1, $statsData );
$this->assertStatsHasCount( 'test.RateLimiter.limit.edit.result.tripped', 1, $statsData );
$this->assertStatsHasCount( 'test.RateLimiter.limit.edit.tripped_by.autoconfirmed', 1, $statsData );
}
/**
* @param string $key
* @param ?int $value
* @param StatsdData[] $statsData
*/
private function assertStatsHasCount( string $key, ?int $value, array $statsData ) {
$metric = StatsdDataInterface::STATSD_METRIC_COUNT;
foreach ( $statsData as $data ) {
if ( $data->getMetric() === $metric && $data->getValue() === $value && $data->getKey() == $key ) {
$this->addToAssertionCount( 1 );
return;
}
}
$this->fail( "Missing metric data entry: $key/$metric/$value" );
}
/**
* @param string $key
* @param StatsdData[] $statsData
*/
private function assertStatsNotHasCount( string $key, array $statsData ) {
$metric = StatsdDataInterface::STATSD_METRIC_COUNT;
foreach ( $statsData as $data ) {
if ( $data->getMetric() === $metric && $data->getKey() == $key ) {
$this->fail( "Metric data entry was not expected to be present: $key/$metric" );
}
}
$this->addToAssertionCount( 1 );
}
}