From 59f1bcbcfd1341a44efa4442ed271904f5e47885 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Carlos=20Ch=C3=A1vez?= Date: Sat, 24 Feb 2018 00:28:43 +0100 Subject: [PATCH] Improves the stability of the namer. --- README.md | 18 ++-- .../DependencyInjection/Configuration.php | 14 ++-- .../DependencyInjection/ZipkinExtension.php | 1 - src/ZipkinBundle/Middleware.php | 27 +++--- .../DefaultNamer.php} | 4 +- .../Route/CacheWarmer.php | 2 +- .../Route/SpanNamer.php} | 19 +++-- .../SpanNamerInterface.php} | 4 +- src/ZipkinBundle/TracingFactory.php | 2 +- tests/Unit/MiddlewareTest.php | 82 ++++++++++++++++++- .../Route/CacheWarmerTest.php | 4 +- tests/Unit/SpanNamers/Route/NamingTest.php | 46 +++++++++++ tests/Unit/SpanNaming/Route/NamingTest.php | 17 ---- 13 files changed, 178 insertions(+), 62 deletions(-) rename src/ZipkinBundle/{SpanNaming/DefaultNaming.php => SpanNamers/DefaultNamer.php} (74%) rename src/ZipkinBundle/{SpanNaming => SpanNamers}/Route/CacheWarmer.php (96%) rename src/ZipkinBundle/{SpanNaming/Route/Naming.php => SpanNamers/Route/SpanNamer.php} (55%) rename src/ZipkinBundle/{SpanNaming/SpanNamingInterface.php => SpanNamers/SpanNamerInterface.php} (72%) rename tests/Unit/{SpanNaming => SpanNamers}/Route/CacheWarmerTest.php (90%) create mode 100644 tests/Unit/SpanNamers/Route/NamingTest.php delete mode 100644 tests/Unit/SpanNaming/Route/NamingTest.php diff --git a/README.md b/README.md index 1594b8d..569f0ca 100644 --- a/README.md +++ b/README.md @@ -141,24 +141,24 @@ services: - { name: kernel.event_listener, event: kernel.exception } ``` -## Span naming +## Span namers By default the span name is being defined by the HTTP verb. This approach is -a not so bad option seeking for low cardinality in span naming. A more useful +a not so bad option seeking for low cardinality in span namer. A more useful approach is to use the route path: `/user/{user_id}` however including the `@router` in the middleware is an expensive operation thus the best is to precompile a map of `name => path` in cache that can be used to resolve the path in runtime. ```yaml - zipkin.span_naming.route: - class: ZipkinBundle\SpanNaming\Route\Naming - factory [ZipkinBundle\SpanNaming\Route\Naming, 'create'] + zipkin.span_namer.route: + class: ZipkinBundle\SpanNamers\Route\SpanNamer + factory: [ZipkinBundle\SpanNamers\Route\SpanNamer, 'create'] arguments: - "%kernel.cache_dir%" - zipkin.span_naming.cache_warmer: - class: ZipkinBundle\SpanNaming\Route\CacheWarmer + zipkin.span_namer.cache_warmer: + class: ZipkinBundle\SpanNamers\Route\CacheWarmer arguments: - "@router" tags: @@ -170,9 +170,9 @@ services: arguments: - "@zipkin.default_tracing" - "@logger" - - "@zipkin.span_naming.route" + - "@zipkin.span_namer.route" tags: - - { name: kernel.event_listener, event: kernel.controller } + - { name: kernel.event_listener, event: kernel.request, priority: 256 } - { name: kernel.event_listener, event: kernel.terminate } - { name: kernel.event_listener, event: kernel.exception } ``` diff --git a/src/ZipkinBundle/DependencyInjection/Configuration.php b/src/ZipkinBundle/DependencyInjection/Configuration.php index 1856a94..4df226d 100755 --- a/src/ZipkinBundle/DependencyInjection/Configuration.php +++ b/src/ZipkinBundle/DependencyInjection/Configuration.php @@ -16,12 +16,12 @@ public function getConfigTreeBuilder() ->children() ->booleanNode('noop')->defaultFalse()->end() ->scalarNode('service_name')->defaultNull()->end() - ->arrayNode('sampler') + ->arrayNode('sampler')->addDefaultsIfNotSet() ->children() ->scalarNode('type') - ->defaultValue('always') + ->defaultValue('never') ->end() - ->arrayNode('route') + ->arrayNode('route')->addDefaultsIfNotSet() ->canBeDisabled() ->children() ->arrayNode('included_routes') @@ -32,7 +32,7 @@ public function getConfigTreeBuilder() ->end() ->end() ->end() - ->arrayNode('path') + ->arrayNode('path')->addDefaultsIfNotSet() ->canBeDisabled() ->children() ->arrayNode('included_paths')->defaultValue([]) @@ -46,10 +46,10 @@ public function getConfigTreeBuilder() ->scalarNode('percentage')->defaultValue(0.1)->end() ->end() ->end() - ->arrayNode('reporter') + ->arrayNode('reporter')->addDefaultsIfNotSet() ->children() - ->scalarNode('type')->defaultValue('http')->end() - ->arrayNode('http') + ->scalarNode('type')->defaultValue('log')->end() + ->arrayNode('http')->addDefaultsIfNotSet() ->children() ->scalarNode('endpoint_url')->defaultValue('http://zipkin:9411/api/v2/spans')->end() ->end() diff --git a/src/ZipkinBundle/DependencyInjection/ZipkinExtension.php b/src/ZipkinBundle/DependencyInjection/ZipkinExtension.php index a1ec44a..0c6bcf1 100755 --- a/src/ZipkinBundle/DependencyInjection/ZipkinExtension.php +++ b/src/ZipkinBundle/DependencyInjection/ZipkinExtension.php @@ -52,7 +52,6 @@ public function load(array $configs, ContainerBuilder $container) $config['sampler']['path']['included_paths'] ); - $container->setParameter( 'zipkin.sampler.path.excluded_paths', $config['sampler']['path']['excluded_paths'] diff --git a/src/ZipkinBundle/Middleware.php b/src/ZipkinBundle/Middleware.php index ce8552f..f3ca01d 100644 --- a/src/ZipkinBundle/Middleware.php +++ b/src/ZipkinBundle/Middleware.php @@ -16,8 +16,8 @@ use Zipkin\Tags; use Zipkin\Tracer; use Zipkin\Tracing; -use ZipkinBundle\SpanNaming\DefaultNaming; -use ZipkinBundle\SpanNaming\SpanNamingInterface; +use ZipkinBundle\SpanNamers\DefaultNamer; +use ZipkinBundle\SpanNamers\SpanNamerInterface; final class Middleware { @@ -37,18 +37,18 @@ final class Middleware private $scopeCloser; /** - * @var SpanNamingInterface + * @var SpanNamerInterface */ - private $spanNaming; + private $spanNamer; public function __construct( Tracing $tracing, LoggerInterface $logger, - SpanNamingInterface $spanNamer = null + SpanNamerInterface $spanNamer = null ) { $this->tracing = $tracing; $this->logger = $logger; - $this->spanNaming = $spanNamer ?: DefaultNaming::create(); + $this->spanNamer = $spanNamer ?: DefaultNamer::create(); } public function onKernelRequest(GetResponseEvent $event) @@ -69,7 +69,6 @@ public function onKernelRequest(GetResponseEvent $event) $span = $this->tracing->getTracer()->nextSpan($spanContext); $span->start(); - $span->setName($this->spanNaming->getName($request)); $span->setKind(Kind\SERVER); $span->tag(Tags\HTTP_HOST, $request->getHost()); $span->tag(Tags\HTTP_METHOD, $request->getMethod()); @@ -90,14 +89,16 @@ public function onKernelException(GetResponseForExceptionEvent $event) public function onKernelTerminate(PostResponseEvent $event) { $span = $this->tracing->getTracer()->getCurrentSpan(); - $request = $event->getRequest(); - - $routeName = $request->attributes->get('_route'); - if ($routeName) { - $span->tag('symfony.route', $routeName); - } if ($span !== null) { + $request = $event->getRequest(); + + $routeName = $request->attributes->get('_route'); + if ($routeName) { + $span->tag('symfony.route', $routeName); + } + + $span->setName($this->spanNamer->getName($request)); $span->tag(Tags\HTTP_STATUS_CODE, $event->getResponse()->getStatusCode()); $span->finish(); call_user_func($this->scopeCloser); diff --git a/src/ZipkinBundle/SpanNaming/DefaultNaming.php b/src/ZipkinBundle/SpanNamers/DefaultNamer.php similarity index 74% rename from src/ZipkinBundle/SpanNaming/DefaultNaming.php rename to src/ZipkinBundle/SpanNamers/DefaultNamer.php index 5484ccb..27ed96c 100644 --- a/src/ZipkinBundle/SpanNaming/DefaultNaming.php +++ b/src/ZipkinBundle/SpanNamers/DefaultNamer.php @@ -1,10 +1,10 @@ request->get('_route'); + $method = $request->getMethod(); + $routeName = $request->attributes->get('_route'); + + if ($routeName === null) { + return $method . ' ' . self::NOT_FOUND; + } if (array_key_exists($routeName, $this->routes)) { - return $request->getMethod() . ' ' . $this->routes[$routeName]; + return $method . ' ' . $this->routes[$routeName]; } - return $request->getMethod(); + return $method; } } diff --git a/src/ZipkinBundle/SpanNaming/SpanNamingInterface.php b/src/ZipkinBundle/SpanNamers/SpanNamerInterface.php similarity index 72% rename from src/ZipkinBundle/SpanNaming/SpanNamingInterface.php rename to src/ZipkinBundle/SpanNamers/SpanNamerInterface.php index 9a15d3a..fb1857a 100644 --- a/src/ZipkinBundle/SpanNaming/SpanNamingInterface.php +++ b/src/ZipkinBundle/SpanNamers/SpanNamerInterface.php @@ -1,10 +1,10 @@ getParameter('zipkin.reporter.http')); + return new Http(null, $container->getParameter('zipkin.reporter.http')); } } diff --git a/tests/Unit/MiddlewareTest.php b/tests/Unit/MiddlewareTest.php index 19033eb..b178c56 100644 --- a/tests/Unit/MiddlewareTest.php +++ b/tests/Unit/MiddlewareTest.php @@ -2,10 +2,14 @@ namespace ZipkinBundle\Tests\Unit; +use Exception; use PHPUnit_Framework_TestCase; use Psr\Log\NullLogger; use Symfony\Component\HttpFoundation\Request; +use Symfony\Component\HttpFoundation\Response; use Symfony\Component\HttpKernel\Event\GetResponseEvent; +use Symfony\Component\HttpKernel\Event\GetResponseForExceptionEvent; +use Symfony\Component\HttpKernel\Event\PostResponseEvent; use Zipkin\Span; use Zipkin\TracingBuilder; use ZipkinBundle\Middleware; @@ -27,7 +31,7 @@ public function testSpanIsNotCreatedOnNonMasterRequest() $this->assertNull($tracing->getTracer()->getCurrentSpan()); } - public function testSpanIsCreated() + public function testSpanIsCreatedOnKernelRequest() { $tracing = TracingBuilder::create()->build(); $logger = new NullLogger(); @@ -42,4 +46,80 @@ public function testSpanIsCreated() $this->assertInstanceOf(Span::class, $tracing->getTracer()->getCurrentSpan()); } + + public function testNoSpanIsTaggedOnKernelExceptionIfItIsNotStarted() + { + $tracing = TracingBuilder::create()->build(); + $logger = new NullLogger(); + + $middleware = new Middleware($tracing, $logger); + + $event = $this->prophesize(GetResponseEvent::class); + $event->isMasterRequest()->willReturn(false); + $event->getRequest()->willReturn(new Request()); + + $middleware->onKernelRequest($event->reveal()); + + $exceptionEvent = $this->prophesize(GetResponseForExceptionEvent::class); + $exceptionEvent->getException()->shouldNotBeCalled(); + $middleware->onKernelException($exceptionEvent->reveal()); + } + + public function testSpanIsTaggedOnKernelException() + { + $tracing = TracingBuilder::create()->build(); + $logger = new NullLogger(); + + $middleware = new Middleware($tracing, $logger); + + $event = $this->prophesize(GetResponseEvent::class); + $event->isMasterRequest()->willReturn(true); + $event->getRequest()->willReturn(new Request()); + + $middleware->onKernelRequest($event->reveal()); + + $exceptionEvent = $this->prophesize(GetResponseForExceptionEvent::class); + $exceptionEvent->getException()->shouldBeCalled()->willReturn(new Exception()); + $middleware->onKernelException($exceptionEvent->reveal()); + } + + public function testNoSpanIsTaggedOnKernelTerminateIfItIsNotStarted() + { + $tracing = TracingBuilder::create()->build(); + $logger = new NullLogger(); + + $middleware = new Middleware($tracing, $logger); + + $event = $this->prophesize(GetResponseEvent::class); + $event->isMasterRequest()->willReturn(false); + $event->getRequest()->willReturn(new Request()); + + $middleware->onKernelRequest($event->reveal()); + + $responseEvent = $this->prophesize(PostResponseEvent::class); + $responseEvent->getRequest()->shouldNotBeCalled(); + $middleware->onKernelTerminate($responseEvent->reveal()); + } + + public function testSpanIsTaggedOnKernelTerminate() + { + $tracing = TracingBuilder::create()->build(); + $logger = new NullLogger(); + + $middleware = new Middleware($tracing, $logger); + + $request = new Request(); + $event = $this->prophesize(GetResponseEvent::class); + $event->isMasterRequest()->willReturn(true); + $event->getRequest()->willReturn($request); + + $middleware->onKernelRequest($event->reveal()); + + $exceptionEvent = $this->prophesize(PostResponseEvent::class); + $exceptionEvent->getRequest()->shouldBeCalled()->willReturn($request); + $exceptionEvent->getResponse()->shouldBeCalled()->willReturn(new Response); + $middleware->onKernelTerminate($exceptionEvent->reveal()); + + $this->assertNull($tracing->getTracer()->getCurrentSpan()); + } } diff --git a/tests/Unit/SpanNaming/Route/CacheWarmerTest.php b/tests/Unit/SpanNamers/Route/CacheWarmerTest.php similarity index 90% rename from tests/Unit/SpanNaming/Route/CacheWarmerTest.php rename to tests/Unit/SpanNamers/Route/CacheWarmerTest.php index 9c03fd4..453e404 100644 --- a/tests/Unit/SpanNaming/Route/CacheWarmerTest.php +++ b/tests/Unit/SpanNamers/Route/CacheWarmerTest.php @@ -1,13 +1,13 @@ getName(new Request()); + $this->assertEquals('GET not_found', $name); + } + + public function testGetNameForExistingRouteSuccess() + { + $cacheDir = sys_get_temp_dir(); + $filename = CacheWarmer::buildOutputFilename($cacheDir); + file_put_contents( + $filename, + sprintf(' "%s"];', self::ROUTE_NAME, self::ROUTE_PATH) + ); + + $request = new Request( + [], + [], + ['_route' => self::ROUTE_NAME], + [], + [], + ['REQUEST_METHOD' => self::METHOD] + ); + + $naming = SpanNamer::create($cacheDir); + $name = $naming->getName($request); + $this->assertEquals(self::METHOD . ' ' . self::ROUTE_PATH, $name); + unlink($filename); + } +} diff --git a/tests/Unit/SpanNaming/Route/NamingTest.php b/tests/Unit/SpanNaming/Route/NamingTest.php deleted file mode 100644 index 8779286..0000000 --- a/tests/Unit/SpanNaming/Route/NamingTest.php +++ /dev/null @@ -1,17 +0,0 @@ -getName(new Request()); - $this->assertEquals('GET', $name); - } -}