diff --git a/RELEASE-NOTES-1.43 b/RELEASE-NOTES-1.43 index ac784b4b336..e4985c7aa0c 100644 --- a/RELEASE-NOTES-1.43 +++ b/RELEASE-NOTES-1.43 @@ -255,6 +255,9 @@ because of Phabricator reports. * `.background-image()` mixin from mediawiki.mixins.less, deprecated since 1.38, has been removed. * IDatabase::nextSequenceValue(), deprecated since 1.30, was removed. +* The setStats() method in MediaWiki\Rest\Router now expects a StatsFactory + rather than a StatsdDataFactoryInterface. The method has been marked @internal + and should not be called by extensions. * … === Deprecations in 1.43 === diff --git a/includes/Rest/EntryPoint.php b/includes/Rest/EntryPoint.php index 00b87345e12..f958327cc6a 100644 --- a/includes/Rest/EntryPoint.php +++ b/includes/Rest/EntryPoint.php @@ -60,7 +60,7 @@ class EntryPoint extends MediaWikiEntryPoint { $authority ); - $stats = $services->getStatsdDataFactory(); + $stats = $services->getStatsFactory(); return ( new Router( self::getRouteFiles( $conf ), diff --git a/includes/Rest/Module/Module.php b/includes/Rest/Module/Module.php index f702f972b91..41c2d2ea6bb 100644 --- a/includes/Rest/Module/Module.php +++ b/includes/Rest/Module/Module.php @@ -2,7 +2,6 @@ namespace MediaWiki\Rest\Module; -use Liuggio\StatsdClient\Factory\StatsdDataFactoryInterface; use LogicException; use MediaWiki\Profiler\ProfilingContext; use MediaWiki\Rest\BasicAccess\BasicAuthorizerInterface; @@ -18,10 +17,10 @@ use MediaWiki\Rest\ResponseFactory; use MediaWiki\Rest\ResponseInterface; use MediaWiki\Rest\Router; use MediaWiki\Rest\Validator\Validator; -use NullStatsdDataFactory; use Throwable; use Wikimedia\Message\MessageValue; use Wikimedia\ObjectFactory\ObjectFactory; +use Wikimedia\Stats\StatsFactory; /** * A REST module represents a collection of endpoints. @@ -46,7 +45,7 @@ abstract class Module { private ErrorReporter $errorReporter; private Router $router; - private StatsdDataFactoryInterface $stats; + private StatsFactory $stats; private ?CorsUtils $cors = null; /** @@ -75,7 +74,7 @@ abstract class Module { $this->restValidator = $restValidator; $this->errorReporter = $errorReporter; - $this->stats = new NullStatsdDataFactory(); + $this->stats = StatsFactory::newNull(); } public function getPathPrefix(): string { @@ -282,7 +281,7 @@ abstract class Module { ResponseInterface $response, float $startTime ) { - $microtime = ( microtime( true ) - $startTime ) * 1000; + $latency = ( microtime( true ) - $startTime ) * 1000; // NOTE: The "/" prefix is for consistency with old logs. It's rather ugly. $pathForMetrics = '/' . $this->getPathPrefix(); @@ -295,13 +294,20 @@ abstract class Module { $requestMethod = $request->getMethod(); if ( $statusCode >= 400 ) { // count how often we return which error code - $this->stats->increment( "rest_api_errors.$pathForMetrics.$requestMethod.$statusCode" ); + $this->stats->getCounter( 'rest_api_errors_total' ) + ->setLabel( 'path', $pathForMetrics ) + ->setLabel( 'method', $requestMethod ) + ->setLabel( 'status', "$statusCode" ) + ->copyToStatsdAt( [ "rest_api_errors.$pathForMetrics.$requestMethod.$statusCode" ] ) + ->increment(); } else { // measure how long it takes to generate a response - $this->stats->timing( - "rest_api_latency.$pathForMetrics.$requestMethod.$statusCode", - $microtime - ); + $this->stats->getTiming( 'rest_api_latency_seconds' ) + ->setLabel( 'path', $pathForMetrics ) + ->setLabel( 'method', $requestMethod ) + ->setLabel( 'status', "$statusCode" ) + ->copyToStatsdAt( "rest_api_latency.$pathForMetrics.$requestMethod.$statusCode" ) + ->observe( $latency ); } } @@ -387,11 +393,11 @@ abstract class Module { /** * @internal for use by Router * - * @param StatsdDataFactoryInterface $stats + * @param StatsFactory $stats * * @return self */ - public function setStats( StatsdDataFactoryInterface $stats ): self { + public function setStats( StatsFactory $stats ): self { $this->stats = $stats; return $this; diff --git a/includes/Rest/Router.php b/includes/Rest/Router.php index 5f14b4dc07a..01de3bda212 100644 --- a/includes/Rest/Router.php +++ b/includes/Rest/Router.php @@ -3,7 +3,6 @@ namespace MediaWiki\Rest; use BagOStuff; -use Liuggio\StatsdClient\Factory\StatsdDataFactoryInterface; use MediaWiki\Config\ServiceOptions; use MediaWiki\HookContainer\HookContainer; use MediaWiki\MainConfigNames; @@ -19,6 +18,7 @@ use MediaWiki\Session\Session; use Throwable; use Wikimedia\Message\MessageValue; use Wikimedia\ObjectFactory\ObjectFactory; +use Wikimedia\Stats\StatsFactory; /** * The REST router is responsible for gathering module configuration, matching @@ -88,7 +88,7 @@ class Router { /** @var Session */ private $session; - /** @var ?StatsdDataFactoryInterface */ + /** @var ?StatsFactory */ private $stats = null; /** @@ -528,11 +528,13 @@ class Router { } /** - * @param StatsdDataFactoryInterface $stats + * @internal + * + * @param StatsFactory $stats * * @return self */ - public function setStats( StatsdDataFactoryInterface $stats ): self { + public function setStats( StatsFactory $stats ): self { $this->stats = $stats; return $this; diff --git a/tests/phpunit/structure/RestStructureTest.php b/tests/phpunit/structure/RestStructureTest.php index 0196969a86b..4195dc49b8c 100644 --- a/tests/phpunit/structure/RestStructureTest.php +++ b/tests/phpunit/structure/RestStructureTest.php @@ -22,6 +22,7 @@ use MediaWiki\Tests\Unit\DummyServicesTrait; use MediaWiki\Title\Title; use MediaWiki\User\UserIdentityValue; use Wikimedia\ParamValidator\ParamValidator; +use Wikimedia\Stats\StatsFactory; use Wikimedia\TestingAccessWrapper; /** @@ -60,14 +61,14 @@ class RestStructureTest extends MediaWikiIntegrationTestCase { 'getHookContainer', 'getObjectFactory', 'getLocalServerObjectCache', - 'getStatsdDataFactory', + 'getStatsFactory', ] ); $services->method( 'getMainConfig' )->willReturn( $config ); $services->method( 'getHookContainer' )->willReturn( $hookContainer ); $services->method( 'getObjectFactory' )->willReturn( $objectFactory ); $services->method( 'getLocalServerObjectCache' )->willReturn( new EmptyBagOStuff() ); - $services->method( 'getStatsdDataFactory' )->willReturn( new NullStatsdDataFactory() ); + $services->method( 'getStatsFactory' )->willReturn( StatsFactory::newNull() ); return $services; } diff --git a/tests/phpunit/unit/includes/Rest/Module/RouteFileModuleTest.php b/tests/phpunit/unit/includes/Rest/Module/RouteFileModuleTest.php index f3648ba3c5a..b7e48769a1f 100644 --- a/tests/phpunit/unit/includes/Rest/Module/RouteFileModuleTest.php +++ b/tests/phpunit/unit/includes/Rest/Module/RouteFileModuleTest.php @@ -3,7 +3,6 @@ namespace MediaWiki\Tests\Rest\Module; use GuzzleHttp\Psr7\Uri; -use Liuggio\StatsdClient\Factory\StatsdDataFactoryInterface; use MediaWiki\MainConfigNames; use MediaWiki\Rest\BasicAccess\StaticBasicAuthorizer; use MediaWiki\Rest\Module\Module; @@ -16,8 +15,13 @@ use MediaWiki\Rest\Validator\Validator; use MediaWiki\Tests\Rest\RestTestTrait; use MediaWiki\Tests\Unit\DummyServicesTrait; use PHPUnit\Framework\MockObject\MockObject; +use Psr\Log\NullLogger; use RuntimeException; use Throwable; +use UDPTransport; +use Wikimedia\Stats\OutputFormats; +use Wikimedia\Stats\StatsCache; +use Wikimedia\Stats\StatsFactory; use Wikimedia\TestingAccessWrapper; /** @@ -93,10 +97,21 @@ class RouteFileModuleTest extends \MediaWikiUnitTestCase { return $module; } - private function createMockStats( string $method, ...$with ): StatsdDataFactoryInterface { - $stats = $this->createNoOpMock( StatsdDataFactoryInterface::class, [ $method ] ); - $stats->expects( $this->atLeastOnce() )->method( $method )->with( ...$with ); - return $stats; + private function createMockStatsFactory( string $expectedPattern ): StatsFactory { + $statsCache = new StatsCache(); + $emitter = OutputFormats::getNewEmitter( + 'mediawiki', + $statsCache, + OutputFormats::getNewFormatter( OutputFormats::DOGSTATSD ) + ); + + $transport = $this->createMock( UDPTransport::class ); + + $transport->expects( $this->once() )->method( "emit" ) + ->with( $this->matchesRegularExpression( $expectedPattern ) ); + + $emitter = $emitter->withTransport( $transport ); + return new StatsFactory( $statsCache, $emitter, new NullLogger ); } public function testWrongMethod() { @@ -128,13 +143,13 @@ class RouteFileModuleTest extends \MediaWikiUnitTestCase { ] ); $module = $this->createRouteFileModule( $request ); - $module->setStats( $this->createMockStats( - 'timing', - 'rest_api_latency._mock_v1_foobar_ModuleTest_greetings_-name-.HEAD.200', - $this->greaterThan( 0 ) - ) ); + $stats = $this->createMockStatsFactory( + "/^mediawiki\.rest_api_latency_seconds:\d+\.\d+\|ms\|#path:mock_v1_foobar_ModuleTest_greetings_name,method:HEAD,status:200\nmediawiki\.stats_buffered_total:1\|c$/" + ); + $module->setStats( $stats ); $response = $module->execute( '/foobar/ModuleTest/greetings/you', $request ); + $stats->flush(); $this->assertSame( 200, $response->getStatusCode() ); } @@ -150,12 +165,13 @@ class RouteFileModuleTest extends \MediaWikiUnitTestCase { $request = new RequestData( [ 'uri' => new Uri( '/rest/mock.v1/ModuleTest/throw' ) ] ); $module = $this->createRouteFileModule( $request ); - $module->setStats( $this->createMockStats( - 'increment', - 'rest_api_errors._mock_v1_ModuleTest_throw.GET.555' - ) ); + $stats = $this->createMockStatsFactory( + "/^mediawiki\.rest_api_errors_total:1\|c\|#path:mock_v1_ModuleTest_throw,method:GET,status:555\nmediawiki\.stats_buffered_total:1\|c$/" + ); + $module->setStats( $stats ); $response = $module->execute( '/ModuleTest/throw', $request ); + $stats->flush(); $this->assertSame( 555, $response->getStatusCode() ); $body = $response->getBody(); $body->rewind(); diff --git a/tests/phpunit/unit/includes/Rest/RouterTest.php b/tests/phpunit/unit/includes/Rest/RouterTest.php index b5759bc186b..9daf2d9286e 100644 --- a/tests/phpunit/unit/includes/Rest/RouterTest.php +++ b/tests/phpunit/unit/includes/Rest/RouterTest.php @@ -4,7 +4,6 @@ namespace MediaWiki\Tests\Rest; use GuzzleHttp\Psr7\Uri; use HashBagOStuff; -use Liuggio\StatsdClient\Factory\StatsdDataFactoryInterface; use MediaWiki\Config\ServiceOptions; use MediaWiki\MainConfigNames; use MediaWiki\Rest\BasicAccess\StaticBasicAuthorizer; @@ -25,9 +24,14 @@ use MediaWiki\User\UserIdentityValue; use MediaWikiUnitTestCase; use PHPUnit\Framework\Assert; use PHPUnit\Framework\MockObject\MockObject; +use Psr\Log\NullLogger; use RuntimeException; use Throwable; +use UDPTransport; use Wikimedia\ParamValidator\ParamValidator; +use Wikimedia\Stats\OutputFormats; +use Wikimedia\Stats\StatsCache; +use Wikimedia\Stats\StatsFactory; use Wikimedia\TestingAccessWrapper; /** @@ -83,10 +87,21 @@ class RouterTest extends MediaWikiUnitTestCase { ] ); } - private function createMockStats( string $method, ...$with ): StatsdDataFactoryInterface { - $stats = $this->createNoOpMock( StatsdDataFactoryInterface::class, [ $method ] ); - $stats->expects( $this->atLeastOnce() )->method( $method )->with( ...$with ); - return $stats; + private function createMockStatsFactory( string $expectedValue ): StatsFactory { + $statsCache = new StatsCache(); + $emitter = OutputFormats::getNewEmitter( + 'mediawiki', + $statsCache, + OutputFormats::getNewFormatter( OutputFormats::DOGSTATSD ) + ); + + $transport = $this->createMock( UDPTransport::class ); + + $transport->expects( $this->once() )->method( "emit" ) + ->with( $this->matchesRegularExpression( $expectedValue ) ); + + $emitter = $emitter->withTransport( $transport ); + return new StatsFactory( $statsCache, $emitter, new NullLogger ); } public function testEmptyPath() { @@ -223,12 +238,13 @@ class RouterTest extends MediaWikiUnitTestCase { $request = new RequestData( [ 'uri' => new Uri( '/rest/mock/v1/RouterTest/throw' ) ] ); $router = $this->createRouter( $request ); - $router->setStats( $this->createMockStats( - 'increment', - 'rest_api_errors._mock_v1_RouterTest_throw.GET.555' - ) ); + $stats = $this->createMockStatsFactory( + "/^mediawiki\.rest_api_errors_total:1\|c\|#path:mock_v1_RouterTest_throw,method:GET,status:555\nmediawiki\.stats_buffered_total:1\|c$/" + ); + $router->setStats( $stats ); $response = $router->execute( $request ); + $stats->flush(); $this->assertSame( 555, $response->getStatusCode(), (string)$response->getBody() ); $body = $response->getBody(); $body->rewind(); @@ -253,13 +269,13 @@ class RouterTest extends MediaWikiUnitTestCase { $request = new RequestData( [ 'uri' => new Uri( '/rest/mock/v1/RouterTest/throwRedirect' ) ] ); $router = $this->createRouter( $request ); - $router->setStats( $this->createMockStats( - 'timing', - 'rest_api_latency._mock_v1_RouterTest_throwRedirect.GET.301', - $this->greaterThan( 0 ) - ) ); + $stats = $this->createMockStatsFactory( + "/^mediawiki\.rest_api_latency_seconds:\d+\.\d+\|ms\|#path:mock_v1_RouterTest_throwRedirect,method:GET,status:301\nmediawiki\.stats_buffered_total:1\|c$/" + ); + $router->setStats( $stats ); $response = $router->execute( $request ); + $stats->flush(); $this->assertSame( 301, $response->getStatusCode(), (string)$response->getBody() ); $this->assertSame( 'http://example.com', $response->getHeaderLine( 'Location' ) ); } @@ -277,13 +293,13 @@ class RouterTest extends MediaWikiUnitTestCase { $request = new RequestData( [ 'uri' => new Uri( '/rest/mock/v1/RouterTest/throwWrapped' ) ] ); $router = $this->createRouter( $request ); - $router->setStats( $this->createMockStats( - 'timing', - 'rest_api_latency._mock_v1_RouterTest_throwWrapped.GET.200', - $this->greaterThan( 0 ) - ) ); + $stats = $this->createMockStatsFactory( + "/^mediawiki\.rest_api_latency_seconds:\d+\.\d+\|ms\|#path:mock_v1_RouterTest_throwWrapped,method:GET,status:200\nmediawiki\.stats_buffered_total:1\|c$/" + ); + $router->setStats( $stats ); $response = $router->execute( $request ); + $stats->flush(); $this->assertSame( 200, $response->getStatusCode(), (string)$response->getBody() ); }