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

REST: fix metrics keys

Browse source 
In Iebcde4645d472d2 I broke the way we generate metrics keys from
endpoint paths. Instead of using the declared paths with placeholders,
we were recording the actual paths, resulting in an explosion of metrics
keys.

This moves metrics logging from Router into Module, where the declared
path is available. This patch also introduces regression tests for the
issue.

Bug: T365111
Change-Id: I2c9ddfe6e28aaecd313356894f17033e2db59073
This commit is contained in:
daniel 2024-05-21 17:02:16 +02:00 committed by Daniel Kinzler
parent f5a53b215f
commit 68dc4845b5
5 changed files with 116 additions and 39 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

52
includes/Rest/Module/Module.php
View file

@ -2,6 +2,7 @@
namespace MediaWiki\Rest\Module;
use Liuggio\StatsdClient\Factory\StatsdDataFactoryInterface;
use LogicException;
use MediaWiki\Profiler\ProfilingContext;
use MediaWiki\Rest\BasicAccess\BasicAuthorizerInterface;
@ -17,6 +18,7 @@ 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;
@ -42,8 +44,9 @@ abstract class Module {
private ObjectFactory $objectFactory;
private Validator $restValidator;
private ErrorReporter $errorReporter;
private Router $router;
private StatsdDataFactoryInterface $stats;
private ?CorsUtils $cors = null;
/**
@ -71,6 +74,8 @@ abstract class Module {
$this->objectFactory = $objectFactory;
$this->restValidator = $restValidator;
$this->errorReporter = $errorReporter;
$this->stats = new NullStatsdDataFactory();
}
public function getPathPrefix(): string {
@ -252,6 +257,7 @@ abstract class Module {
*/
public function execute( string $path, RequestInterface $request ): ResponseInterface {
$handler = null;
$startTime = microtime( true );
try {
$handler = $this->getHandlerForPath( $path, $request, true );
@ -265,9 +271,40 @@ abstract class Module {
$response = $this->responseFactory->createFromException( $e );
}
$this->recordMetrics( $handler, $request, $response, $startTime );
return $response;
}
private function recordMetrics(
?Handler $handler,
RequestInterface $request,
ResponseInterface $response,
float $startTime
) {
$microtime = ( microtime( true ) - $startTime ) * 1000;
// NOTE: The "/" prefix is for consistency with old logs. It's rather ugly.
$pathForMetrics = '/' . $this->getPathPrefix();
$pathForMetrics .= $handler ? $handler->getPath() : '/UNKNOWN';
// Replace any characters that may have a special meaning in the metrics DB.
$pathForMetrics = strtr( $pathForMetrics, '{}:/.', '---__' );
$statusCode = $response->getStatusCode();
$requestMethod = $request->getMethod();
if ( $statusCode >= 400 ) {
// count how often we return which error code
$this->stats->increment( "rest_api_errors.$pathForMetrics.$requestMethod.$statusCode" );
} else {
// measure how long it takes to generate a response
$this->stats->timing(
"rest_api_latency.$pathForMetrics.$requestMethod.$statusCode",
$microtime
);
}
}
/**
* @internal for testing
*
@ -347,6 +384,19 @@ abstract class Module {
return $this;
}
/**
* @internal for use by Router
*
* @param StatsdDataFactoryInterface $stats
*
* @return self
*/
public function setStats( StatsdDataFactoryInterface $stats ): self {
$this->stats = $stats;
return $this;
}
/**
* Loads a module specification from a file.
*

37
includes/Rest/Router.php
View file

@ -15,7 +15,6 @@ use MediaWiki\Rest\PathTemplateMatcher\ModuleConfigurationException;
use MediaWiki\Rest\Reporter\ErrorReporter;
use MediaWiki\Rest\Validator\Validator;
use MediaWiki\Session\Session;
use NullStatsdDataFactory;
use Throwable;
use Wikimedia\Message\MessageValue;
use Wikimedia\ObjectFactory\ObjectFactory;
@ -85,8 +84,8 @@ class Router {
/** @var Session */
private $session;
/** @var StatsdDataFactoryInterface */
private $stats;
/** @var ?StatsdDataFactoryInterface */
private $stats = null;
/**
* @internal
@ -143,8 +142,6 @@ class Router {
$this->errorReporter = $errorReporter;
$this->hookContainer = $hookContainer;
$this->session = $session;
$this->stats = new NullStatsdDataFactory();
}
/**
@ -396,6 +393,10 @@ class Router {
$module->setCors( $this->cors );
}
if ( $this->stats ) {
$module->setStats( $this->stats );
}
$this->modules[$name] = $module;
return $module;
}
@ -460,8 +461,6 @@ class Router {
}
private function doExecute( string $fullPath, RequestInterface $request ): ResponseInterface {
$requestMethod = $request->getMethod();
[ $modulePrefix, $path ] = $this->splitPath( $fullPath );
$module = $this->getModule( $modulePrefix );
@ -473,29 +472,7 @@ class Router {
);
}
// Replace any characters that may have a special meaning in the metrics DB.
$pathForMetrics = $path;
$pathForMetrics = strtr( $pathForMetrics, '{}:', '-' );
$pathForMetrics = strtr( $pathForMetrics, '/.', '_' );
$statTime = microtime( true );
$response = $module->execute( $path, $request );
// gather metrics
$statusCode = $response->getStatusCode();
if ( $response->getStatusCode() >= 400 ) {
// count how often we return which error code
$this->stats->increment( "rest_api_errors.$pathForMetrics.$requestMethod.$statusCode" );
} else {
// measure how long it takes to generate a response
$microtime = ( microtime( true ) - $statTime ) * 1000;
$this->stats->timing(
"rest_api_latency.$pathForMetrics.$requestMethod.$statusCode",
$microtime
);
}
return $response;
return $module->execute( $path, $request );
}
/**

24
tests/phpunit/unit/includes/Rest/Module/RouteFileModuleTest.php
View file

@ -3,6 +3,7 @@
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;
@ -92,6 +93,12 @@ 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;
}
public function testWrongMethod() {
$request = new RequestData( [
'uri' => new Uri( '/rest/mock.v1/ModuleTest/hello' ),
@ -116,11 +123,18 @@ class RouteFileModuleTest extends \MediaWikiUnitTestCase {
public function testFlatRouteFile() {
$request = new RequestData( [
'uri' => new Uri( '/rest/foobar/ModuleTest/hello/two' ),
'uri' => new Uri( '/rest/foobar/ModuleTest/greetings/you' ),
'method' => 'HEAD'
] );
$module = $this->createRouteFileModule( $request );
$response = $module->execute( '/ModuleTest/hello', $request );
$module->setStats( $this->createMockStats(
'timing',
'rest_api_latency._mock_v1_foobar_ModuleTest_greetings_-name-.HEAD.200',
$this->greaterThan( 0 )
) );
$response = $module->execute( '/foobar/ModuleTest/greetings/you', $request );
$this->assertSame( 200, $response->getStatusCode() );
}
@ -135,6 +149,12 @@ class RouteFileModuleTest extends \MediaWikiUnitTestCase {
public function testHttpException() {
$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'
) );
$response = $module->execute( '/ModuleTest/throw', $request );
$this->assertSame( 555, $response->getStatusCode() );
$body = $response->getBody();

2
tests/phpunit/unit/includes/Rest/Module/moduleFlatRoutes.json
View file

@ -1,6 +1,6 @@
[
{
"path": "/mock.v1/ModuleTest/hello/two",
"path": "/foobar/ModuleTest/greetings/{name}",
"class": "MediaWiki\\Tests\\Rest\\Handler\\HelloHandler"
}
]

40
tests/phpunit/unit/includes/Rest/RouterTest.php
View file

@ -4,14 +4,18 @@ 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;
use MediaWiki\Rest\CorsUtils;
use MediaWiki\Rest\Handler;
use MediaWiki\Rest\HttpException;
use MediaWiki\Rest\RedirectException;
use MediaWiki\Rest\Reporter\ErrorReporter;
use MediaWiki\Rest\RequestData;
use MediaWiki\Rest\RequestInterface;
use MediaWiki\Rest\ResponseException;
use MediaWiki\Rest\ResponseFactory;
use MediaWiki\Rest\Router;
use MediaWiki\Rest\StringStream;
@ -72,6 +76,12 @@ 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;
}
public function testPrefixMismatch() {
$request = new RequestData( [ 'uri' => new Uri( '/bogus' ) ] );
$router = $this->createRouter( $request );
@ -176,8 +186,14 @@ class RouterTest extends MediaWikiUnitTestCase {
public function testHttpException() {
$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'
) );
$response = $router->execute( $request );
$this->assertSame( 555, $response->getStatusCode() );
$this->assertSame( 555, $response->getStatusCode(), (string)$response->getBody() );
$body = $response->getBody();
$body->rewind();
$data = json_decode( $body->getContents(), true );
@ -188,7 +204,7 @@ class RouterTest extends MediaWikiUnitTestCase {
$request = new RequestData( [ 'uri' => new Uri( '/rest/mock/v1/RouterTest/fatal' ) ] );
$router = $this->createRouter( $request );
$response = $router->execute( $request );
$this->assertSame( 500, $response->getStatusCode() );
$this->assertSame( 500, $response->getStatusCode(), (string)$response->getBody() );
$body = $response->getBody();
$body->rewind();
$data = json_decode( $body->getContents(), true );
@ -200,8 +216,15 @@ class RouterTest extends MediaWikiUnitTestCase {
public function testRedirectException() {
$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 )
) );
$response = $router->execute( $request );
$this->assertSame( 301, $response->getStatusCode() );
$this->assertSame( 301, $response->getStatusCode(), (string)$response->getBody() );
$this->assertSame( 'http://example.com', $response->getHeaderLine( 'Location' ) );
}
@ -210,15 +233,22 @@ class RouterTest extends MediaWikiUnitTestCase {
$request = new RequestData( [ 'uri' => new Uri( '/rest/mock/v1/RouterTest/redirect' ) ] );
$router = $this->createRouter( $request );
$response = $router->execute( $request );
$this->assertSame( 308, $response->getStatusCode() );
$this->assertSame( 308, $response->getStatusCode(), (string)$response->getBody() );
$this->assertSame( '/rest/mock/RouterTest/redirectTarget', $response->getHeaderLine( 'Location' ) );
}
public function testResponseException() {
$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 )
) );
$response = $router->execute( $request );
$this->assertSame( 200, $response->getStatusCode() );
$this->assertSame( 200, $response->getStatusCode(), (string)$response->getBody() );
}
public function testBasicAccess() {