From 1d18a247bb7c30fe6a6db9f948dd5fd16da09ce6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Carlos=20Ch=C3=A1vez?= Date: Wed, 24 Jun 2020 17:38:35 +0200 Subject: [PATCH 01/20] feat(http-server): adds middleware and request handler instrumentation. --- .../Http/Server/DefaultParser.php | 39 ++++++++ .../Http/Server/Middleware.php | 88 +++++++++++++++++ .../Instrumentation/Http/Server/Parser.php | 53 +++++++++++ .../Http/Server/RequestHandler.php | 94 +++++++++++++++++++ .../Http/Server/RequestHeaders.php | 17 ++++ .../Http/Server/ServerTracing.php | 68 ++++++++++++++ 6 files changed, 359 insertions(+) create mode 100644 src/Zipkin/Instrumentation/Http/Server/DefaultParser.php create mode 100644 src/Zipkin/Instrumentation/Http/Server/Middleware.php create mode 100644 src/Zipkin/Instrumentation/Http/Server/Parser.php create mode 100644 src/Zipkin/Instrumentation/Http/Server/RequestHandler.php create mode 100644 src/Zipkin/Instrumentation/Http/Server/RequestHeaders.php create mode 100644 src/Zipkin/Instrumentation/Http/Server/ServerTracing.php diff --git a/src/Zipkin/Instrumentation/Http/Server/DefaultParser.php b/src/Zipkin/Instrumentation/Http/Server/DefaultParser.php new file mode 100644 index 00000000..0f5f068a --- /dev/null +++ b/src/Zipkin/Instrumentation/Http/Server/DefaultParser.php @@ -0,0 +1,39 @@ +getMethod(); + } + + public function request(ServerRequestInterface $request, TraceContext $context, SpanCustomizer $span): void + { + $span->tag(Tags\HTTP_METHOD, $request->getMethod()); + $span->tag(Tags\HTTP_PATH, $request->getUri()->getPath() ?: "/"); + } + + public function response(ResponseInterface $response, TraceContext $context, SpanCustomizer $span): void + { + $span->tag(Tags\HTTP_STATUS_CODE, (string) $response->getStatusCode()); + if ($response->getStatusCode() > 399) { + $span->tag(Tags\ERROR, (string) $response->getStatusCode()); + } + } + + public function error(Throwable $e, TraceContext $context, SpanCustomizer $span): void + { + $span->tag(Tags\ERROR, $e->getMessage()); + } +} diff --git a/src/Zipkin/Instrumentation/Http/Server/Middleware.php b/src/Zipkin/Instrumentation/Http/Server/Middleware.php new file mode 100644 index 00000000..054568f6 --- /dev/null +++ b/src/Zipkin/Instrumentation/Http/Server/Middleware.php @@ -0,0 +1,88 @@ +tracer = $tracing->getTracing()->getTracer(); + $this->extractor = $tracing->getTracing()->getPropagation()->getExtractor(new RequestHeaders()); + $this->parser = $tracing->getParser(); + $this->requestSampler = $tracing->getRequestSampler(); + } + + public function process(ServerRequestInterface $request, RequestHandlerInterface $handler): ResponseInterface + { + $extractedContext = ($this->extractor)($request); + + if ($extractedContext instanceof TraceContext) { + $span = $this->tracer->joinSpan($extractedContext); + } elseif ($this->requestSampler === null) { + $span = $this->tracer->nextSpan($extractedContext); + } else { + $span = $this->tracer->nextSpanWithSampler( + $this->requestSampler, + [$request], + $extractedContext + ); + } + + $spanCustomizer = null; + if (!$span->isNoop()) { + $span->setKind(Kind\SERVER); + // If span is NOOP it does not make sense to add customizations + // to it like tags or annotations. + $spanCustomizer = new SpanCustomizerShield($span); + $span->setName($this->parser->spanName($request)); + $this->parser->request($request, $span->getContext(), $spanCustomizer); + } + + try { + $response = $handler->handle($request); + if ($spanCustomizer !== null) { + $this->parser->response($response, $span->getContext(), $spanCustomizer); + } + return $response; + } catch (Throwable $e) { + if ($spanCustomizer !== null) { + $this->parser->error($e, $span->getContext(), $spanCustomizer); + } + throw $e; + } finally { + $span->finish(); + } + } +} diff --git a/src/Zipkin/Instrumentation/Http/Server/Parser.php b/src/Zipkin/Instrumentation/Http/Server/Parser.php new file mode 100644 index 00000000..c0e13374 --- /dev/null +++ b/src/Zipkin/Instrumentation/Http/Server/Parser.php @@ -0,0 +1,53 @@ +delegate = $delegate; + $this->tracer = $tracing->getTracing()->getTracer(); + $this->extractor = $tracing->getTracing()->getPropagation()->getExtractor(new RequestHeaders()); + $this->parser = $tracing->getParser(); + $this->requestSampler = $tracing->getRequestSampler(); + } + + public function handle(ServerRequestInterface $request): ResponseInterface + { + $extractedContext = ($this->extractor)($request); + + if ($extractedContext instanceof TraceContext) { + $span = $this->tracer->joinSpan($extractedContext); + } elseif ($this->requestSampler === null) { + $span = $this->tracer->nextSpan($extractedContext); + } else { + $span = $this->tracer->nextSpanWithSampler( + $this->requestSampler, + [$request], + $extractedContext + ); + } + + $spanCustomizer = null; + if (!$span->isNoop()) { + $span->setKind(Kind\SERVER); + // If span is NOOP it does not make sense to add customizations + // to it like tags or annotations. + $spanCustomizer = new SpanCustomizerShield($span); + $span->setName($this->parser->spanName($request)); + $this->parser->request($request, $span->getContext(), $spanCustomizer); + } + + try { + $response = $this->delegate->handle($request); + if ($spanCustomizer !== null) { + $this->parser->response($response, $span->getContext(), $spanCustomizer); + } + return $response; + } catch (Throwable $e) { + if ($spanCustomizer !== null) { + $this->parser->error($e, $span->getContext(), $spanCustomizer); + } + throw $e; + } finally { + $span->finish(); + } + } +} diff --git a/src/Zipkin/Instrumentation/Http/Server/RequestHeaders.php b/src/Zipkin/Instrumentation/Http/Server/RequestHeaders.php new file mode 100644 index 00000000..2cc6c213 --- /dev/null +++ b/src/Zipkin/Instrumentation/Http/Server/RequestHeaders.php @@ -0,0 +1,17 @@ + + * function (RequestInterface $request): ?bool {} + * + * + * @var callable|null + */ + private $requestSampler; + + public function __construct( + Tracing $tracing, + Parser $parser = null, + callable $requestSampler = null + ) { + $this->tracing = $tracing; + $this->parser = $parser ?? new DefaultParser; + $this->requestSampler = $requestSampler; + } + + public function getTracing(): Tracing + { + return $this->tracing; + } + + /** + * @return callable|null with the signature: + * + *
+     * function (RequestInterface $request): ?bool
+     * 
+ */ + public function getRequestSampler(): ?callable + { + return $this->requestSampler; + } + + public function getParser(): Parser + { + return $this->parser; + } +} From bd11c76ece4233fda93cb70670d614eb25c7d266 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Carlos=20Ch=C3=A1vez?= Date: Wed, 24 Jun 2020 21:53:22 +0200 Subject: [PATCH 02/20] tests(http-server): adds integration test for the Middleware. --- composer.json | 15 ++- .../Http/Server/Middleware.php | 1 - .../Instrumentation/Http/Server/Parser.php | 2 +- .../Instrumentation/Http/Server/README.md | 41 ++++++ .../Http/Server/ServerTest.php | 92 +++++++++++++ .../Http/Client/ClientTest.php | 2 +- .../Http/Server/ServerTest.php | 121 ++++++++++++++++++ 7 files changed, 268 insertions(+), 6 deletions(-) create mode 100644 src/Zipkin/Instrumentation/Http/Server/README.md create mode 100644 tests/Integration/Instrumentation/Http/Server/ServerTest.php create mode 100644 tests/Unit/Instrumentation/Http/Server/ServerTest.php diff --git a/composer.json b/composer.json index 103b5fa0..d3538db6 100644 --- a/composer.json +++ b/composer.json @@ -7,13 +7,17 @@ "tracing", "openzipkin" ], - "license": "MIT", + "license": "Apache-2.0", "authors": [ { "name": "José Carlos Chávez", "email": "jcchavezs@gmail.com" } ], + "homepage": "https://github.com/openzipkin/zipkin-php", + "support": { + "issues": "https://github.com/openzipkin/zipkin-php/issues" + }, "require": { "php": "^7.1", "ext-curl": "*", @@ -21,11 +25,15 @@ "psr/log": "^1.0" }, "require-dev": { - "guzzlehttp/psr7": "^1.4", + "guzzlehttp/psr7": "^1.6", "jcchavezs/httptest": "~0.2", "phpstan/phpstan": "~0.12.28", "phpunit/phpunit": "~7.5.20", "psr/http-client": "^1.0", + "psr/http-server-middleware": "^1.0", + "psr/http-server-handler": "^1.0", + "middlewares/fast-route": "^1.2.1", + "middlewares/request-handler": "^1.4.0", "squizlabs/php_codesniffer": "3.*" }, "config": { @@ -60,7 +68,8 @@ "static-check": "phpstan analyse src --level 8" }, "suggest": { - "psr/http-client": "Allows to instrument HTTP clients following PSR18." + "psr/http-client": "Allows to instrument HTTP clients following PSR18.", + "psr/http-server": "Allows to instrument HTTP servers following PSR15." }, "extra": { "branch-alias": { diff --git a/src/Zipkin/Instrumentation/Http/Server/Middleware.php b/src/Zipkin/Instrumentation/Http/Server/Middleware.php index 054568f6..3da48e08 100644 --- a/src/Zipkin/Instrumentation/Http/Server/Middleware.php +++ b/src/Zipkin/Instrumentation/Http/Server/Middleware.php @@ -59,7 +59,6 @@ public function process(ServerRequestInterface $request, RequestHandlerInterface $extractedContext ); } - $spanCustomizer = null; if (!$span->isNoop()) { $span->setKind(Kind\SERVER); diff --git a/src/Zipkin/Instrumentation/Http/Server/Parser.php b/src/Zipkin/Instrumentation/Http/Server/Parser.php index c0e13374..d55b1f9c 100644 --- a/src/Zipkin/Instrumentation/Http/Server/Parser.php +++ b/src/Zipkin/Instrumentation/Http/Server/Parser.php @@ -22,7 +22,7 @@ interface Parser * usually the HTTP method is enough (e.g GET or POST) but ideally * the http.route is desired (e.g. /user/{user_id}). */ - public function spanName(RequestInterface $request): string; + public function spanName(ServerRequestInterface $request): string; /** * request parses the incoming data related to a request in order to add diff --git a/src/Zipkin/Instrumentation/Http/Server/README.md b/src/Zipkin/Instrumentation/Http/Server/README.md new file mode 100644 index 00000000..902b8d8c --- /dev/null +++ b/src/Zipkin/Instrumentation/Http/Server/README.md @@ -0,0 +1,41 @@ +# Zipkin instrumentation for PSR15 HTTP Server + +This component contains the instrumentation for the standard [PSR15 HTTP servers](https://www.php-fig.org/psr/psr-15/). + +## Getting started + +Before using this library, make sure the interfaces for PSR15 HTTP server are installed: + +```bash +composer require psr/http-server-middleware +``` + +## Usage + +In this example we use [fast-route](https://github.com/middlewares/fast-route) and [request-handler](https://github.com/middlewares/request-handler) middlewares but any HTTP server middleware supporting PSR15 middlewares will work. + +```php +use Zipkin\Instrumentation\Http\Server\Middleware as ZipkinMiddleware; +use Zipkin\Instrumentation\Http\Server\ServerTracing; + +// Create the routing dispatcher +$fastRouteDispatcher = FastRoute\simpleDispatcher(function (FastRoute\RouteCollector $r) { + $r->get('/hello/{name}', HelloWorldController::class); +}); + +// Creates tracing component +$tracing = TracingBuilder::create() + ->havingLocalServiceName('my_service') + ->build(); + +$httpClientTracing = new ServerTracing($tracing); + +$dispatcher = new Dispatcher([ + new Middlewares\FastRoute($fastRouteDispatcher), + // ... + new ZipkinMiddleware($serverTracing), + new Middlewares\RequestHandler(), +]); + +$response = $dispatcher->dispatch(new ServerRequest('/hello/world')); +``` diff --git a/tests/Integration/Instrumentation/Http/Server/ServerTest.php b/tests/Integration/Instrumentation/Http/Server/ServerTest.php new file mode 100644 index 00000000..ac84750d --- /dev/null +++ b/tests/Integration/Instrumentation/Http/Server/ServerTest.php @@ -0,0 +1,92 @@ +havingReporter($reporter) + ->havingSampler(BinarySampler::createAsAlwaysSample()) + ->build(); + $tracer = $tracing->getTracer(); + + return [ + new ServerTracing($tracing, $parser), + static function () use ($tracer, $reporter): array { + $tracer->flush(); + return $reporter->flush(); + } + ]; + } + + public function testMiddleware() + { + $parser = new class() extends DefaultParser { + public function request(ServerRequestInterface $request, TraceContext $context, SpanCustomizer $span): void + { + // This parser retrieves the user_id from the request and add + // is a tag. + $userId = $request->getAttribute('user_id'); + $span->tag('user_id', $userId); + parent::request($request, $context, $span); + } + }; + + list($serverTracing, $flusher) = self::createTracing($parser); + + $fastRouteDispatcher = simpleDispatcher(function (RouteCollector $r) { + $r->addRoute('GET', '/users/{user_id}', function ($request) { + return new Response(201); + }); + }); + + $request = Factory::createServerRequest('GET', '/users/abc123'); + + $response = Dispatcher::run([ + new Middlewares\FastRoute($fastRouteDispatcher), + new Middleware($serverTracing), + new Middlewares\RequestHandler(), + ], $request); + + $this->assertEquals(201, $response->getStatusCode()); + + $spans = ($flusher)(); + + $this->assertCount(1, $spans); + + $span = $spans[0]->toArray(); + + $this->assertEquals('GET', $span['name']); + $this->assertEquals([ + 'http.method' => 'GET', + 'http.path' => '/users/abc123', + 'http.status_code' => '201', + 'user_id' => 'abc123', + ], $span['tags']); + } +} diff --git a/tests/Unit/Instrumentation/Http/Client/ClientTest.php b/tests/Unit/Instrumentation/Http/Client/ClientTest.php index bdd71425..7586c8c6 100644 --- a/tests/Unit/Instrumentation/Http/Client/ClientTest.php +++ b/tests/Unit/Instrumentation/Http/Client/ClientTest.php @@ -2,7 +2,7 @@ declare(strict_types=1); -namespace ZipkinTests\Instrumentation\Http\Client; +namespace ZipkinTests\Unit\Instrumentation\Http\Client; use Zipkin\TracingBuilder; use Zipkin\Samplers\BinarySampler; diff --git a/tests/Unit/Instrumentation/Http/Server/ServerTest.php b/tests/Unit/Instrumentation/Http/Server/ServerTest.php new file mode 100644 index 00000000..0e3a500e --- /dev/null +++ b/tests/Unit/Instrumentation/Http/Server/ServerTest.php @@ -0,0 +1,121 @@ +havingReporter($reporter) + ->havingSampler(BinarySampler::createAsAlwaysSample()) + ->build(); + $tracer = $tracing->getTracer(); + + return [ + new ServerTracing($tracing), + static function () use ($tracer, $reporter): array { + $tracer->flush(); + return $reporter->flush(); + } + ]; + } + + public function testMiddlewareHandlesRequestSuccessfully() + { + list($tracing, $flusher) = self::createTracing(); + $request = new ServerRequest('GET', 'http://mytest'); + + $handler = new class() implements RequestHandlerInterface { + private $lastRequest; + + public function handle(ServerRequestInterface $request): ResponseInterface + { + $this->lastRequest = $request; + + return new Psr7Response(); + } + + public function getLastRequest(): ?RequestInterface + { + return $this->lastRequest; + } + }; + + $middleware = new Middleware($tracing); + $middleware->process($request, $handler); + + $this->assertSame($request, $handler->getLastRequest()); + + $spans = ($flusher)(); + + $this->assertCount(1, $spans); + + $span = $spans[0]->toArray(); + + $this->assertEquals('GET', $span['name']); + $this->assertEquals([ + 'http.method' => 'GET', + 'http.path' => '/', + 'http.status_code' => '200', + ], $span['tags']); + } + + public function testMiddlewareParsesRequestSuccessfullyWithNon2xx() + { + list($tracing, $flusher) = self::createTracing(); + $request = new ServerRequest('GET', 'http://mytest'); + + $handler = new class() implements RequestHandlerInterface { + private $lastRequest; + + public function handle(ServerRequestInterface $request): ResponseInterface + { + $this->lastRequest = $request; + + return new Psr7Response(404); + } + + public function getLastRequest(): ?RequestInterface + { + return $this->lastRequest; + } + }; + + $middleware = new Middleware($tracing); + $middleware->process($request, $handler); + + $this->assertSame($request, $handler->getLastRequest()); + + $spans = ($flusher)(); + + $this->assertCount(1, $spans); + + $span = $spans[0]->toArray(); + + $this->assertEquals('GET', $span['name']); + $this->assertEquals([ + 'http.method' => 'GET', + 'http.path' => '/', + 'http.status_code' => '404', + 'error' => '404' + ], $span['tags']); + } +} From 14fcc62afb26d493c7986f37a4d0357da297a776 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Carlos=20Ch=C3=A1vez?= Date: Wed, 24 Jun 2020 21:58:16 +0200 Subject: [PATCH 03/20] docs(http-server): improves documentation. --- README.md | 1 + composer.json | 3 ++- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index fce9bf1e..01d876c2 100644 --- a/README.md +++ b/README.md @@ -295,6 +295,7 @@ request. To do this, simply pass null to `openScope`. ## Instrumentation - [PSR18 HTTP Client](src/Zipkin/Instrumentation/Http/Client) +- [PSR15 HTTP Server](src/Zipkin/Instrumentation/Http/Server) ## Tests diff --git a/composer.json b/composer.json index d3538db6..bbe7f80d 100644 --- a/composer.json +++ b/composer.json @@ -69,7 +69,8 @@ }, "suggest": { "psr/http-client": "Allows to instrument HTTP clients following PSR18.", - "psr/http-server": "Allows to instrument HTTP servers following PSR15." + "psr/http-server-middleware": "Allows to instrument HTTP servers following PSR15.", + "psr/http-server-handler": "Allows to instrument HTTP servers following PSR15." }, "extra": { "branch-alias": { From 1f462b0a2991a0a76dba69cd6fe1f377824b5e46 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Carlos=20Ch=C3=A1vez?= Date: Wed, 24 Jun 2020 22:02:09 +0200 Subject: [PATCH 04/20] chore(http-server): makes linter happy. --- src/Zipkin/Instrumentation/Http/Server/Middleware.php | 2 +- src/Zipkin/Instrumentation/Http/Server/RequestHandler.php | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Zipkin/Instrumentation/Http/Server/Middleware.php b/src/Zipkin/Instrumentation/Http/Server/Middleware.php index 3da48e08..d8c26c1c 100644 --- a/src/Zipkin/Instrumentation/Http/Server/Middleware.php +++ b/src/Zipkin/Instrumentation/Http/Server/Middleware.php @@ -32,7 +32,7 @@ final class Middleware implements MiddlewareInterface private $parser; /** - * @var callable + * @var callable|null */ private $requestSampler; diff --git a/src/Zipkin/Instrumentation/Http/Server/RequestHandler.php b/src/Zipkin/Instrumentation/Http/Server/RequestHandler.php index 2378f9a2..879b365f 100644 --- a/src/Zipkin/Instrumentation/Http/Server/RequestHandler.php +++ b/src/Zipkin/Instrumentation/Http/Server/RequestHandler.php @@ -37,7 +37,7 @@ final class RequestHandler implements RequestHandlerInterface private $parser; /** - * @var callable + * @var callable|null */ private $requestSampler; From 31faf67b0b4ae888f542eb407adf9d8cc8b5c4aa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Carlos=20Ch=C3=A1vez?= Date: Fri, 26 Jun 2020 10:19:04 +0200 Subject: [PATCH 05/20] chore(http-server): removes RequestHandler as middleware is enough for tracing. --- composer.json | 5 +- .../Http/Server/Middleware.php | 31 +++--- .../Http/Server/RequestHandler.php | 94 ------------------- .../Http/Server/ServerTest.php | 39 ++++---- 4 files changed, 37 insertions(+), 132 deletions(-) delete mode 100644 src/Zipkin/Instrumentation/Http/Server/RequestHandler.php diff --git a/composer.json b/composer.json index bbe7f80d..3af9fab2 100644 --- a/composer.json +++ b/composer.json @@ -4,6 +4,7 @@ "description": "A Zipkin instrumentation for PHP", "keywords": [ "zipkin", + "distributed-tracing", "tracing", "openzipkin" ], @@ -31,7 +32,6 @@ "phpunit/phpunit": "~7.5.20", "psr/http-client": "^1.0", "psr/http-server-middleware": "^1.0", - "psr/http-server-handler": "^1.0", "middlewares/fast-route": "^1.2.1", "middlewares/request-handler": "^1.4.0", "squizlabs/php_codesniffer": "3.*" @@ -69,8 +69,7 @@ }, "suggest": { "psr/http-client": "Allows to instrument HTTP clients following PSR18.", - "psr/http-server-middleware": "Allows to instrument HTTP servers following PSR15.", - "psr/http-server-handler": "Allows to instrument HTTP servers following PSR15." + "psr/http-server-middleware": "Allows to instrument HTTP servers via middlewares following PSR15." }, "extra": { "branch-alias": { diff --git a/src/Zipkin/Instrumentation/Http/Server/Middleware.php b/src/Zipkin/Instrumentation/Http/Server/Middleware.php index d8c26c1c..87280eb2 100644 --- a/src/Zipkin/Instrumentation/Http/Server/Middleware.php +++ b/src/Zipkin/Instrumentation/Http/Server/Middleware.php @@ -59,29 +59,32 @@ public function process(ServerRequestInterface $request, RequestHandlerInterface $extractedContext ); } - $spanCustomizer = null; - if (!$span->isNoop()) { - $span->setKind(Kind\SERVER); - // If span is NOOP it does not make sense to add customizations - // to it like tags or annotations. - $spanCustomizer = new SpanCustomizerShield($span); - $span->setName($this->parser->spanName($request)); - $this->parser->request($request, $span->getContext(), $spanCustomizer); + $scopeCloser = $this->tracer->openScope($span); + + if ($span->isNoop()) { + try { + return $handler->handle($request); + } finally { + $span->finish(); + $scopeCloser(); + } } + $span->setKind(Kind\SERVER); + $spanCustomizer = new SpanCustomizerShield($span); + $span->setName($this->parser->spanName($request)); + $this->parser->request($request, $span->getContext(), $spanCustomizer); + try { $response = $handler->handle($request); - if ($spanCustomizer !== null) { - $this->parser->response($response, $span->getContext(), $spanCustomizer); - } + $this->parser->response($response, $span->getContext(), $spanCustomizer); return $response; } catch (Throwable $e) { - if ($spanCustomizer !== null) { - $this->parser->error($e, $span->getContext(), $spanCustomizer); - } + $this->parser->error($e, $span->getContext(), $spanCustomizer); throw $e; } finally { $span->finish(); + $scopeCloser(); } } } diff --git a/src/Zipkin/Instrumentation/Http/Server/RequestHandler.php b/src/Zipkin/Instrumentation/Http/Server/RequestHandler.php deleted file mode 100644 index 879b365f..00000000 --- a/src/Zipkin/Instrumentation/Http/Server/RequestHandler.php +++ /dev/null @@ -1,94 +0,0 @@ -delegate = $delegate; - $this->tracer = $tracing->getTracing()->getTracer(); - $this->extractor = $tracing->getTracing()->getPropagation()->getExtractor(new RequestHeaders()); - $this->parser = $tracing->getParser(); - $this->requestSampler = $tracing->getRequestSampler(); - } - - public function handle(ServerRequestInterface $request): ResponseInterface - { - $extractedContext = ($this->extractor)($request); - - if ($extractedContext instanceof TraceContext) { - $span = $this->tracer->joinSpan($extractedContext); - } elseif ($this->requestSampler === null) { - $span = $this->tracer->nextSpan($extractedContext); - } else { - $span = $this->tracer->nextSpanWithSampler( - $this->requestSampler, - [$request], - $extractedContext - ); - } - - $spanCustomizer = null; - if (!$span->isNoop()) { - $span->setKind(Kind\SERVER); - // If span is NOOP it does not make sense to add customizations - // to it like tags or annotations. - $spanCustomizer = new SpanCustomizerShield($span); - $span->setName($this->parser->spanName($request)); - $this->parser->request($request, $span->getContext(), $spanCustomizer); - } - - try { - $response = $this->delegate->handle($request); - if ($spanCustomizer !== null) { - $this->parser->response($response, $span->getContext(), $spanCustomizer); - } - return $response; - } catch (Throwable $e) { - if ($spanCustomizer !== null) { - $this->parser->error($e, $span->getContext(), $spanCustomizer); - } - throw $e; - } finally { - $span->finish(); - } - } -} diff --git a/tests/Unit/Instrumentation/Http/Server/ServerTest.php b/tests/Unit/Instrumentation/Http/Server/ServerTest.php index 0e3a500e..f37d356c 100644 --- a/tests/Unit/Instrumentation/Http/Server/ServerTest.php +++ b/tests/Unit/Instrumentation/Http/Server/ServerTest.php @@ -38,19 +38,22 @@ static function () use ($tracer, $reporter): array { ]; } - public function testMiddlewareHandlesRequestSuccessfully() + private static function createRequestHandler($response = null): RequestHandlerInterface { - list($tracing, $flusher) = self::createTracing(); - $request = new ServerRequest('GET', 'http://mytest'); - - $handler = new class() implements RequestHandlerInterface { + return new class($response) implements RequestHandlerInterface { + private $response; private $lastRequest; + public function __construct(?ResponseInterface $response) + { + $this->response = $response ?? new Psr7Response(); + } + public function handle(ServerRequestInterface $request): ResponseInterface { $this->lastRequest = $request; - return new Psr7Response(); + return $this->response; } public function getLastRequest(): ?RequestInterface @@ -58,6 +61,14 @@ public function getLastRequest(): ?RequestInterface return $this->lastRequest; } }; + } + + public function testMiddlewareHandlesRequestSuccessfully() + { + list($tracing, $flusher) = self::createTracing(); + $request = new ServerRequest('GET', 'http://mytest'); + + $handler = self::createRequestHandler(); $middleware = new Middleware($tracing); $middleware->process($request, $handler); @@ -83,21 +94,7 @@ public function testMiddlewareParsesRequestSuccessfullyWithNon2xx() list($tracing, $flusher) = self::createTracing(); $request = new ServerRequest('GET', 'http://mytest'); - $handler = new class() implements RequestHandlerInterface { - private $lastRequest; - - public function handle(ServerRequestInterface $request): ResponseInterface - { - $this->lastRequest = $request; - - return new Psr7Response(404); - } - - public function getLastRequest(): ?RequestInterface - { - return $this->lastRequest; - } - }; + $handler = self::createRequestHandler(new Psr7Response(404)); $middleware = new Middleware($tracing); $middleware->process($request, $handler); From 754340ddd95233764bfa9305a1c6c5db0c8636fc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Carlos=20Ch=C3=A1vez?= Date: Mon, 6 Jul 2020 17:11:15 +0200 Subject: [PATCH 06/20] chore: uses span error API. --- src/Zipkin/Instrumentation/Http/Server/DefaultParser.php | 5 ----- src/Zipkin/Instrumentation/Http/Server/Middleware.php | 2 +- src/Zipkin/Instrumentation/Http/Server/Parser.php | 7 ------- 3 files changed, 1 insertion(+), 13 deletions(-) diff --git a/src/Zipkin/Instrumentation/Http/Server/DefaultParser.php b/src/Zipkin/Instrumentation/Http/Server/DefaultParser.php index 0f5f068a..1c429206 100644 --- a/src/Zipkin/Instrumentation/Http/Server/DefaultParser.php +++ b/src/Zipkin/Instrumentation/Http/Server/DefaultParser.php @@ -31,9 +31,4 @@ public function response(ResponseInterface $response, TraceContext $context, Spa $span->tag(Tags\ERROR, (string) $response->getStatusCode()); } } - - public function error(Throwable $e, TraceContext $context, SpanCustomizer $span): void - { - $span->tag(Tags\ERROR, $e->getMessage()); - } } diff --git a/src/Zipkin/Instrumentation/Http/Server/Middleware.php b/src/Zipkin/Instrumentation/Http/Server/Middleware.php index 87280eb2..b9da73b6 100644 --- a/src/Zipkin/Instrumentation/Http/Server/Middleware.php +++ b/src/Zipkin/Instrumentation/Http/Server/Middleware.php @@ -80,7 +80,7 @@ public function process(ServerRequestInterface $request, RequestHandlerInterface $this->parser->response($response, $span->getContext(), $spanCustomizer); return $response; } catch (Throwable $e) { - $this->parser->error($e, $span->getContext(), $spanCustomizer); + $span->setError($e); throw $e; } finally { $span->finish(); diff --git a/src/Zipkin/Instrumentation/Http/Server/Parser.php b/src/Zipkin/Instrumentation/Http/Server/Parser.php index d55b1f9c..5f9a1df7 100644 --- a/src/Zipkin/Instrumentation/Http/Server/Parser.php +++ b/src/Zipkin/Instrumentation/Http/Server/Parser.php @@ -43,11 +43,4 @@ public function request(ServerRequestInterface $request, TraceContext $context, * payload can be added. */ public function response(ResponseInterface $response, TraceContext $context, SpanCustomizer $span): void; - - /** - * error parses the exception when doing a HTTP call, usually it is good enough to tag - * the throwable message but depending on the wrapping client, one might want to enrich - * the error with meaningful information from the exception. - */ - public function error(Throwable $e, TraceContext $context, SpanCustomizer $span): void; } From 9fa99f14bd9d3588c3b8c05bdf816027317f7b36 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Carlos=20Ch=C3=A1vez?= Date: Mon, 13 Jul 2020 15:17:46 +0200 Subject: [PATCH 07/20] chore(http-server): decouples client/server tracing from PSR implementation. --- phpstan.neon | 8 ++- .../Http/Client/DefaultParser.php | 34 ------------ ...lientTracing.php => HttpClientTracing.php} | 12 ++--- .../Instrumentation/Http/Client/Parser.php | 16 +++--- .../Http/Client/{ => Psr18}/Client.php | 6 ++- .../Http/Client/Psr18/DefaultParser.php | 52 +++++++++++++++++++ .../Http/Client/{ => Psr18}/README.md | 7 +-- .../Client/{ => Psr18}/RequestHeaders.php | 2 +- .../Http/Server/DefaultParser.php | 34 ------------ ...erverTracing.php => HttpServerTracing.php} | 20 +++---- .../Instrumentation/Http/Server/Parser.php | 17 +++--- .../Http/Server/Psr15/DefaultParser.php | 52 +++++++++++++++++++ .../Http/Server/{ => Psr15}/Middleware.php | 6 ++- .../Http/Server/{ => Psr15}/README.md | 7 +-- .../Server/{ => Psr15}/RequestHeaders.php | 2 +- .../Http/Server/ServerTest.php | 11 ++-- .../Http/Client/{ => Psr18}/ClientTest.php | 9 ++-- .../Http/Server/{ => Psr15}/ServerTest.php | 9 ++-- 18 files changed, 175 insertions(+), 129 deletions(-) delete mode 100644 src/Zipkin/Instrumentation/Http/Client/DefaultParser.php rename src/Zipkin/Instrumentation/Http/Client/{ClientTracing.php => HttpClientTracing.php} (82%) rename src/Zipkin/Instrumentation/Http/Client/{ => Psr18}/Client.php (92%) create mode 100644 src/Zipkin/Instrumentation/Http/Client/Psr18/DefaultParser.php rename src/Zipkin/Instrumentation/Http/Client/{ => Psr18}/README.md (75%) rename src/Zipkin/Instrumentation/Http/Client/{ => Psr18}/RequestHeaders.php (85%) delete mode 100644 src/Zipkin/Instrumentation/Http/Server/DefaultParser.php rename src/Zipkin/Instrumentation/Http/Server/{ServerTracing.php => HttpServerTracing.php} (70%) create mode 100644 src/Zipkin/Instrumentation/Http/Server/Psr15/DefaultParser.php rename src/Zipkin/Instrumentation/Http/Server/{ => Psr15}/Middleware.php (91%) rename src/Zipkin/Instrumentation/Http/Server/{ => Psr15}/README.md (81%) rename src/Zipkin/Instrumentation/Http/Server/{ => Psr15}/RequestHeaders.php (85%) rename tests/Unit/Instrumentation/Http/Client/{ => Psr18}/ClientTest.php (94%) rename tests/Unit/Instrumentation/Http/Server/{ => Psr15}/ServerTest.php (91%) diff --git a/phpstan.neon b/phpstan.neon index b15106d2..19258744 100644 --- a/phpstan.neon +++ b/phpstan.neon @@ -36,4 +36,10 @@ parameters: - # SpanCustomizer is definitively not null message: '#Parameter \#3 \$ of callable callable\(.*, Zipkin\\Propagation\\TraceContext, Zipkin\\SpanCustomizer\): void expects Zipkin\\SpanCustomizer, Zipkin\\SpanCustomizerShield\|null given.#' - path: src/Zipkin/Tracer \ No newline at end of file + path: src/Zipkin/Tracer + - + message: '#Method .* with no typehint specified.#' + path: src/Zipkin/Instrumentation/Http/Server + - + message: '#Method .* with no typehint specified.#' + path: src/Zipkin/Instrumentation/Http/Client \ No newline at end of file diff --git a/src/Zipkin/Instrumentation/Http/Client/DefaultParser.php b/src/Zipkin/Instrumentation/Http/Client/DefaultParser.php deleted file mode 100644 index 7ec3b2f9..00000000 --- a/src/Zipkin/Instrumentation/Http/Client/DefaultParser.php +++ /dev/null @@ -1,34 +0,0 @@ -getMethod(); - } - - public function request(RequestInterface $request, TraceContext $context, SpanCustomizer $span): void - { - $span->tag(Tags\HTTP_METHOD, $request->getMethod()); - $span->tag(Tags\HTTP_PATH, $request->getUri()->getPath() ?: "/"); - } - - public function response(ResponseInterface $response, TraceContext $context, SpanCustomizer $span): void - { - $span->tag(Tags\HTTP_STATUS_CODE, (string) $response->getStatusCode()); - if ($response->getStatusCode() > 399) { - $span->tag(Tags\ERROR, (string) $response->getStatusCode()); - } - } -} diff --git a/src/Zipkin/Instrumentation/Http/Client/ClientTracing.php b/src/Zipkin/Instrumentation/Http/Client/HttpClientTracing.php similarity index 82% rename from src/Zipkin/Instrumentation/Http/Client/ClientTracing.php rename to src/Zipkin/Instrumentation/Http/Client/HttpClientTracing.php index 31fd3842..4cbb6f31 100644 --- a/src/Zipkin/Instrumentation/Http/Client/ClientTracing.php +++ b/src/Zipkin/Instrumentation/Http/Client/HttpClientTracing.php @@ -11,7 +11,7 @@ * ClientTracing includes all the elements needed to instrument a * HTTP client. */ -class ClientTracing +class HttpClientTracing { /** * @var Tracing @@ -25,11 +25,7 @@ class ClientTracing /** * function that decides to sample or not an unsampled - * request. The signature is: - * - *
-     * function (RequestInterface $request): ?bool {}
-     * 
+ * request. * * @var callable(RequestInterface):?bool|null */ @@ -37,11 +33,11 @@ class ClientTracing public function __construct( Tracing $tracing, - Parser $parser = null, + Parser $parser, callable $requestSampler = null ) { $this->tracing = $tracing; - $this->parser = $parser ?? new DefaultParser; + $this->parser = $parser; $this->requestSampler = $requestSampler; } diff --git a/src/Zipkin/Instrumentation/Http/Client/Parser.php b/src/Zipkin/Instrumentation/Http/Client/Parser.php index 2a1592e4..af8462fc 100644 --- a/src/Zipkin/Instrumentation/Http/Client/Parser.php +++ b/src/Zipkin/Instrumentation/Http/Client/Parser.php @@ -6,13 +6,17 @@ use Zipkin\SpanCustomizer; use Zipkin\Propagation\TraceContext; -use Throwable; -use Psr\Http\Message\ResponseInterface; -use Psr\Http\Message\RequestInterface; /** * Parser includes the methods to obtain meaningful span information * out of HTTP request/response elements. + * + * $request and $response objects are not typed on purpose as different + * frameworks can use different request models (e.g. symfony uses a list + * parameters including method, url and options). Since PHP does not support + * generics this is the only way to make this interface reusable but + * implementors have to make sure the request/response objects have the right + * type. */ interface Parser { @@ -20,7 +24,7 @@ interface Parser * spanName returns an appropiate span name based on the request, * usually the HTTP method is good enough (e.g GET or POST). */ - public function spanName(RequestInterface $request): string; + public function spanName($request): string; /** * request parses the incoming data related to a request in order to add @@ -29,7 +33,7 @@ public function spanName(RequestInterface $request): string; * Basic data being tagged is HTTP method, HTTP path but other information * such as query parameters can be added to enrich the span information. */ - public function request(RequestInterface $request, TraceContext $context, SpanCustomizer $span): void; + public function request($request, TraceContext $context, SpanCustomizer $span): void; /** @@ -40,5 +44,5 @@ public function request(RequestInterface $request, TraceContext $context, SpanCu * as any response header or more granular information based on the response * payload can be added. */ - public function response(ResponseInterface $response, TraceContext $context, SpanCustomizer $span): void; + public function response($response, TraceContext $context, SpanCustomizer $span): void; } diff --git a/src/Zipkin/Instrumentation/Http/Client/Client.php b/src/Zipkin/Instrumentation/Http/Client/Psr18/Client.php similarity index 92% rename from src/Zipkin/Instrumentation/Http/Client/Client.php rename to src/Zipkin/Instrumentation/Http/Client/Psr18/Client.php index 7e817eea..c8b2450d 100644 --- a/src/Zipkin/Instrumentation/Http/Client/Client.php +++ b/src/Zipkin/Instrumentation/Http/Client/Psr18/Client.php @@ -2,12 +2,14 @@ declare(strict_types=1); -namespace Zipkin\Instrumentation\Http\Client; +namespace Zipkin\Instrumentation\Http\Client\Psr18; use Zipkin\Tracer; use Zipkin\SpanCustomizerShield; use Zipkin\Propagation\TraceContext; use Zipkin\Kind; +use Zipkin\Instrumentation\Http\Client\Parser; +use Zipkin\Instrumentation\Http\Client\HttpClientTracing; use Throwable; use Psr\Http\Message\ResponseInterface; use Psr\Http\Message\RequestInterface; @@ -42,7 +44,7 @@ final class Client implements ClientInterface public function __construct( ClientInterface $delegate, - ClientTracing $tracing + HttpClientTracing $tracing ) { $this->delegate = $delegate; $this->injector = $tracing->getTracing()->getPropagation()->getInjector(new RequestHeaders()); diff --git a/src/Zipkin/Instrumentation/Http/Client/Psr18/DefaultParser.php b/src/Zipkin/Instrumentation/Http/Client/Psr18/DefaultParser.php new file mode 100644 index 00000000..d23389de --- /dev/null +++ b/src/Zipkin/Instrumentation/Http/Client/Psr18/DefaultParser.php @@ -0,0 +1,52 @@ +getMethod(); + } + + public function request($request, TraceContext $context, SpanCustomizer $span): void + { + self::assertRequestType($request); + $span->tag(Tags\HTTP_METHOD, $request->getMethod()); + $span->tag(Tags\HTTP_PATH, $request->getUri()->getPath() ?: "/"); + } + + public function response($response, TraceContext $context, SpanCustomizer $span): void + { + self::assertResponseType($response); + $span->tag(Tags\HTTP_STATUS_CODE, (string) $response->getStatusCode()); + if ($response->getStatusCode() > 399) { + $span->tag(Tags\ERROR, (string) $response->getStatusCode()); + } + } + + // Makes sure the type for request object is correct. + private static function assertRequestType(RequestInterface $request): void + { + } + + // Makes sure the type for response object is correct. + private static function assertResponseType(ResponseInterface $response): void + { + } +} diff --git a/src/Zipkin/Instrumentation/Http/Client/README.md b/src/Zipkin/Instrumentation/Http/Client/Psr18/README.md similarity index 75% rename from src/Zipkin/Instrumentation/Http/Client/README.md rename to src/Zipkin/Instrumentation/Http/Client/Psr18/README.md index 1316838a..8b0dc4d3 100644 --- a/src/Zipkin/Instrumentation/Http/Client/README.md +++ b/src/Zipkin/Instrumentation/Http/Client/Psr18/README.md @@ -17,14 +17,15 @@ In this example we use Guzzle 7 but any HTTP client supporting PSR18 clients wil ```php use GuzzleHttp\Client; use GuzzleHttp\Psr7\Request; -use Zipkin\Instrumentation\Http\Client\Client as ZipkinClient; -use Zipkin\Instrumentation\Http\Client\ClientTracing; +use Zipkin\Instrumentation\Http\Client\HttpClientTracing; +use Zipkin\Instrumentation\Http\Client\Psr\Client as ZipkinClient; +use Zipkin\Instrumentation\Http\Client\Psr\DefaultParser; $tracing = TracingBuilder::create() ->havingLocalServiceName('my_service') ->build(); -$httpClientTracing = new ClientTracing($tracing); +$httpClientTracing = new HttpClientTracing($tracing, new DefaultParser); ... $httpClient = new ZipkinClient(new Client, $httpClientTracing); diff --git a/src/Zipkin/Instrumentation/Http/Client/RequestHeaders.php b/src/Zipkin/Instrumentation/Http/Client/Psr18/RequestHeaders.php similarity index 85% rename from src/Zipkin/Instrumentation/Http/Client/RequestHeaders.php rename to src/Zipkin/Instrumentation/Http/Client/Psr18/RequestHeaders.php index 374bfe03..fab06551 100644 --- a/src/Zipkin/Instrumentation/Http/Client/RequestHeaders.php +++ b/src/Zipkin/Instrumentation/Http/Client/Psr18/RequestHeaders.php @@ -2,7 +2,7 @@ declare(strict_types=1); -namespace Zipkin\Instrumentation\Http\Client; +namespace Zipkin\Instrumentation\Http\Client\Psr18; use Zipkin\Propagation\RequestHeaders as BaseRequestHeaders; use Zipkin\Propagation\RemoteSetter; diff --git a/src/Zipkin/Instrumentation/Http/Server/DefaultParser.php b/src/Zipkin/Instrumentation/Http/Server/DefaultParser.php deleted file mode 100644 index 1c429206..00000000 --- a/src/Zipkin/Instrumentation/Http/Server/DefaultParser.php +++ /dev/null @@ -1,34 +0,0 @@ -getMethod(); - } - - public function request(ServerRequestInterface $request, TraceContext $context, SpanCustomizer $span): void - { - $span->tag(Tags\HTTP_METHOD, $request->getMethod()); - $span->tag(Tags\HTTP_PATH, $request->getUri()->getPath() ?: "/"); - } - - public function response(ResponseInterface $response, TraceContext $context, SpanCustomizer $span): void - { - $span->tag(Tags\HTTP_STATUS_CODE, (string) $response->getStatusCode()); - if ($response->getStatusCode() > 399) { - $span->tag(Tags\ERROR, (string) $response->getStatusCode()); - } - } -} diff --git a/src/Zipkin/Instrumentation/Http/Server/ServerTracing.php b/src/Zipkin/Instrumentation/Http/Server/HttpServerTracing.php similarity index 70% rename from src/Zipkin/Instrumentation/Http/Server/ServerTracing.php rename to src/Zipkin/Instrumentation/Http/Server/HttpServerTracing.php index 72e98adc..a609026a 100644 --- a/src/Zipkin/Instrumentation/Http/Server/ServerTracing.php +++ b/src/Zipkin/Instrumentation/Http/Server/HttpServerTracing.php @@ -10,7 +10,7 @@ * Tracing includes all the elements needed to trace a HTTP server * middleware or request handler. */ -class ServerTracing +class HttpServerTracing { /** * @var Tracing @@ -24,23 +24,19 @@ class ServerTracing /** * function that decides to sample or not an unsampled - * request. The signature is: + * request. * - *
-     * function (RequestInterface $request): ?bool {}
-     * 
- * - * @var callable|null + * @var (callable(mixed):?bool)|null */ private $requestSampler; public function __construct( Tracing $tracing, - Parser $parser = null, + Parser $parser, callable $requestSampler = null ) { $this->tracing = $tracing; - $this->parser = $parser ?? new DefaultParser; + $this->parser = $parser; $this->requestSampler = $requestSampler; } @@ -50,11 +46,7 @@ public function getTracing(): Tracing } /** - * @return callable|null with the signature: - * - *
-     * function (RequestInterface $request): ?bool
-     * 
+ * @return (callable(mixed):?bool)|null */ public function getRequestSampler(): ?callable { diff --git a/src/Zipkin/Instrumentation/Http/Server/Parser.php b/src/Zipkin/Instrumentation/Http/Server/Parser.php index 5f9a1df7..051246d0 100644 --- a/src/Zipkin/Instrumentation/Http/Server/Parser.php +++ b/src/Zipkin/Instrumentation/Http/Server/Parser.php @@ -6,14 +6,17 @@ use Zipkin\SpanCustomizer; use Zipkin\Propagation\TraceContext; -use Throwable; -use Psr\Http\Message\ServerRequestInterface; -use Psr\Http\Message\ResponseInterface; -use Psr\Http\Message\RequestInterface; /** * Parser includes the methods to obtain meaningful span information * out of HTTP request/response elements. + * + * $request and $response objects are not typed on purpose as different + * frameworks can use different request models (e.g. symfony uses a list + * parameters including method, url and options). Since PHP does not support + * generics this is the only way to make this interface reusable but + * implementors have to make sure the request/response objects have the right + * type. */ interface Parser { @@ -22,7 +25,7 @@ interface Parser * usually the HTTP method is enough (e.g GET or POST) but ideally * the http.route is desired (e.g. /user/{user_id}). */ - public function spanName(ServerRequestInterface $request): string; + public function spanName($request): string; /** * request parses the incoming data related to a request in order to add @@ -31,7 +34,7 @@ public function spanName(ServerRequestInterface $request): string; * Basic data being tagged is HTTP method, HTTP path but other information * such as query parameters can be added to enrich the span information. */ - public function request(ServerRequestInterface $request, TraceContext $context, SpanCustomizer $span): void; + public function request($request, TraceContext $context, SpanCustomizer $span): void; /** @@ -42,5 +45,5 @@ public function request(ServerRequestInterface $request, TraceContext $context, * as any response header or more granular information based on the response * payload can be added. */ - public function response(ResponseInterface $response, TraceContext $context, SpanCustomizer $span): void; + public function response($response, TraceContext $context, SpanCustomizer $span): void; } diff --git a/src/Zipkin/Instrumentation/Http/Server/Psr15/DefaultParser.php b/src/Zipkin/Instrumentation/Http/Server/Psr15/DefaultParser.php new file mode 100644 index 00000000..ba6d9257 --- /dev/null +++ b/src/Zipkin/Instrumentation/Http/Server/Psr15/DefaultParser.php @@ -0,0 +1,52 @@ +getMethod(); + } + + public function request($request, TraceContext $context, SpanCustomizer $span): void + { + self::assertRequestType($request); + $span->tag(Tags\HTTP_METHOD, $request->getMethod()); + $span->tag(Tags\HTTP_PATH, $request->getUri()->getPath() ?: "/"); + } + + public function response($response, TraceContext $context, SpanCustomizer $span): void + { + self::assertResponseType($response); + $span->tag(Tags\HTTP_STATUS_CODE, (string) $response->getStatusCode()); + if ($response->getStatusCode() > 399) { + $span->tag(Tags\ERROR, (string) $response->getStatusCode()); + } + } + + // Makes sure the type for request object is correct. + private static function assertRequestType(ServerRequestInterface $request): void + { + } + + // Makes sure the type for response object is correct. + private static function assertResponseType(ResponseInterface $response): void + { + } +} diff --git a/src/Zipkin/Instrumentation/Http/Server/Middleware.php b/src/Zipkin/Instrumentation/Http/Server/Psr15/Middleware.php similarity index 91% rename from src/Zipkin/Instrumentation/Http/Server/Middleware.php rename to src/Zipkin/Instrumentation/Http/Server/Psr15/Middleware.php index b9da73b6..68d36022 100644 --- a/src/Zipkin/Instrumentation/Http/Server/Middleware.php +++ b/src/Zipkin/Instrumentation/Http/Server/Psr15/Middleware.php @@ -2,12 +2,14 @@ declare(strict_types=1); -namespace Zipkin\Instrumentation\Http\Server; +namespace Zipkin\Instrumentation\Http\Server\Psr15; use Zipkin\Tracer; use Zipkin\SpanCustomizerShield; use Zipkin\Propagation\TraceContext; use Zipkin\Kind; +use Zipkin\Instrumentation\Http\Server\Parser; +use Zipkin\Instrumentation\Http\Server\HttpServerTracing; use Throwable; use Psr\Http\Server\RequestHandlerInterface; use Psr\Http\Server\MiddlewareInterface; @@ -36,7 +38,7 @@ final class Middleware implements MiddlewareInterface */ private $requestSampler; - public function __construct(ServerTracing $tracing) + public function __construct(HttpServerTracing $tracing) { $this->tracer = $tracing->getTracing()->getTracer(); $this->extractor = $tracing->getTracing()->getPropagation()->getExtractor(new RequestHeaders()); diff --git a/src/Zipkin/Instrumentation/Http/Server/README.md b/src/Zipkin/Instrumentation/Http/Server/Psr15/README.md similarity index 81% rename from src/Zipkin/Instrumentation/Http/Server/README.md rename to src/Zipkin/Instrumentation/Http/Server/Psr15/README.md index 902b8d8c..785fe0f6 100644 --- a/src/Zipkin/Instrumentation/Http/Server/README.md +++ b/src/Zipkin/Instrumentation/Http/Server/Psr15/README.md @@ -15,8 +15,9 @@ composer require psr/http-server-middleware In this example we use [fast-route](https://github.com/middlewares/fast-route) and [request-handler](https://github.com/middlewares/request-handler) middlewares but any HTTP server middleware supporting PSR15 middlewares will work. ```php -use Zipkin\Instrumentation\Http\Server\Middleware as ZipkinMiddleware; -use Zipkin\Instrumentation\Http\Server\ServerTracing; +use Zipkin\Instrumentation\Http\Server\Psr15\DefaultParser; +use Zipkin\Instrumentation\Http\Server\Psr15\Middleware as ZipkinMiddleware; +use Zipkin\Instrumentation\Http\Server\HttpServerTracing; // Create the routing dispatcher $fastRouteDispatcher = FastRoute\simpleDispatcher(function (FastRoute\RouteCollector $r) { @@ -28,7 +29,7 @@ $tracing = TracingBuilder::create() ->havingLocalServiceName('my_service') ->build(); -$httpClientTracing = new ServerTracing($tracing); +$httpClientTracing = new ServerTracing($tracing, new DefaultParser); $dispatcher = new Dispatcher([ new Middlewares\FastRoute($fastRouteDispatcher), diff --git a/src/Zipkin/Instrumentation/Http/Server/RequestHeaders.php b/src/Zipkin/Instrumentation/Http/Server/Psr15/RequestHeaders.php similarity index 85% rename from src/Zipkin/Instrumentation/Http/Server/RequestHeaders.php rename to src/Zipkin/Instrumentation/Http/Server/Psr15/RequestHeaders.php index 2cc6c213..5a6311d9 100644 --- a/src/Zipkin/Instrumentation/Http/Server/RequestHeaders.php +++ b/src/Zipkin/Instrumentation/Http/Server/Psr15/RequestHeaders.php @@ -2,7 +2,7 @@ declare(strict_types=1); -namespace Zipkin\Instrumentation\Http\Server; +namespace Zipkin\Instrumentation\Http\Server\Psr15; use Zipkin\Propagation\RequestHeaders as BaseRequestHeaders; use Zipkin\Propagation\RemoteSetter; diff --git a/tests/Integration/Instrumentation/Http/Server/ServerTest.php b/tests/Integration/Instrumentation/Http/Server/ServerTest.php index ac84750d..4adc48ec 100644 --- a/tests/Integration/Instrumentation/Http/Server/ServerTest.php +++ b/tests/Integration/Instrumentation/Http/Server/ServerTest.php @@ -11,10 +11,10 @@ use Zipkin\Reporters\InMemory; use Zipkin\Propagation\TraceContext; -use Zipkin\Instrumentation\Http\Server\ServerTracing; +use Zipkin\Instrumentation\Http\Server\Psr15\Middleware; +use Zipkin\Instrumentation\Http\Server\Psr15\DefaultParser; use Zipkin\Instrumentation\Http\Server\Parser; -use Zipkin\Instrumentation\Http\Server\Middleware; -use Zipkin\Instrumentation\Http\Server\DefaultParser; +use Zipkin\Instrumentation\Http\Server\HttpServerTracing; use RingCentral\Psr7\Response; use Psr\Http\Message\ServerRequestInterface; use PHPUnit\Framework\TestCase; @@ -36,7 +36,7 @@ private static function createTracing(Parser $parser): array $tracer = $tracing->getTracer(); return [ - new ServerTracing($tracing, $parser), + new HttpServerTracing($tracing, $parser), static function () use ($tracer, $reporter): array { $tracer->flush(); return $reporter->flush(); @@ -47,8 +47,9 @@ static function () use ($tracer, $reporter): array { public function testMiddleware() { $parser = new class() extends DefaultParser { - public function request(ServerRequestInterface $request, TraceContext $context, SpanCustomizer $span): void + public function request($request, TraceContext $context, SpanCustomizer $span): void { + assert($request instanceof ServerRequestInterface); // This parser retrieves the user_id from the request and add // is a tag. $userId = $request->getAttribute('user_id'); diff --git a/tests/Unit/Instrumentation/Http/Client/ClientTest.php b/tests/Unit/Instrumentation/Http/Client/Psr18/ClientTest.php similarity index 94% rename from tests/Unit/Instrumentation/Http/Client/ClientTest.php rename to tests/Unit/Instrumentation/Http/Client/Psr18/ClientTest.php index 7586c8c6..8f44ea0f 100644 --- a/tests/Unit/Instrumentation/Http/Client/ClientTest.php +++ b/tests/Unit/Instrumentation/Http/Client/Psr18/ClientTest.php @@ -2,13 +2,14 @@ declare(strict_types=1); -namespace ZipkinTests\Unit\Instrumentation\Http\Client; +namespace ZipkinTests\Unit\Instrumentation\Http\Client\Psr18; use Zipkin\TracingBuilder; use Zipkin\Samplers\BinarySampler; use Zipkin\Reporters\InMemory; -use Zipkin\Instrumentation\Http\Client\ClientTracing; -use Zipkin\Instrumentation\Http\Client\Client; +use Zipkin\Instrumentation\Http\Client\Psr18\DefaultParser; +use Zipkin\Instrumentation\Http\Client\Psr18\Client; +use Zipkin\Instrumentation\Http\Client\HttpClientTracing; use RuntimeException; use Psr\Http\Message\ResponseInterface; use Psr\Http\Message\RequestInterface; @@ -30,7 +31,7 @@ private static function createTracing(): array $tracer = $tracing->getTracer(); return [ - new ClientTracing($tracing), + new HttpClientTracing($tracing, new DefaultParser), static function () use ($tracer, $reporter): array { $tracer->flush(); return $reporter->flush(); diff --git a/tests/Unit/Instrumentation/Http/Server/ServerTest.php b/tests/Unit/Instrumentation/Http/Server/Psr15/ServerTest.php similarity index 91% rename from tests/Unit/Instrumentation/Http/Server/ServerTest.php rename to tests/Unit/Instrumentation/Http/Server/Psr15/ServerTest.php index f37d356c..db3417c4 100644 --- a/tests/Unit/Instrumentation/Http/Server/ServerTest.php +++ b/tests/Unit/Instrumentation/Http/Server/Psr15/ServerTest.php @@ -2,13 +2,14 @@ declare(strict_types=1); -namespace ZipkinTests\Unit\Instrumentation\Http\Server; +namespace ZipkinTests\Unit\Instrumentation\Http\Server\Psr15; use Zipkin\TracingBuilder; use Zipkin\Samplers\BinarySampler; use Zipkin\Reporters\InMemory; -use Zipkin\Instrumentation\Http\Server\ServerTracing; -use Zipkin\Instrumentation\Http\Server\Middleware; +use Zipkin\Instrumentation\Http\Server\Psr15\Middleware; +use Zipkin\Instrumentation\Http\Server\Psr15\DefaultParser; +use Zipkin\Instrumentation\Http\Server\HttpServerTracing; use RingCentral\Psr7\Response as Psr7Response; use Psr\Http\Server\RequestHandlerInterface; use Psr\Http\Message\ServerRequestInterface; @@ -30,7 +31,7 @@ private static function createTracing(): array $tracer = $tracing->getTracer(); return [ - new ServerTracing($tracing), + new HttpServerTracing($tracing, new DefaultParser), static function () use ($tracer, $reporter): array { $tracer->flush(); return $reporter->flush(); From 64cb9f9ddfebd9f22dcba9b3195780cb401f45c2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Carlos=20Ch=C3=A1vez?= Date: Mon, 13 Jul 2020 19:56:21 +0200 Subject: [PATCH 08/20] tests(http-server): adds test for next span in the PSR15 server middleware. --- README.md | 4 +- .../Http/Server/Psr15/Middleware.php | 33 ++++++---- .../{ServerTest.php => MiddlewareTest.php} | 64 ++++++++++++++++++- 3 files changed, 86 insertions(+), 15 deletions(-) rename tests/Unit/Instrumentation/Http/Server/Psr15/{ServerTest.php => MiddlewareTest.php} (59%) diff --git a/README.md b/README.md index 01d876c2..7b4f2b9a 100644 --- a/README.md +++ b/README.md @@ -294,8 +294,8 @@ request. To do this, simply pass null to `openScope`. ## Instrumentation -- [PSR18 HTTP Client](src/Zipkin/Instrumentation/Http/Client) -- [PSR15 HTTP Server](src/Zipkin/Instrumentation/Http/Server) +- [PSR18 HTTP Client](src/Zipkin/Instrumentation/Http/Client/Psr18) +- [PSR15 HTTP Server](src/Zipkin/Instrumentation/Http/Server/Psr15) ## Tests diff --git a/src/Zipkin/Instrumentation/Http/Server/Psr15/Middleware.php b/src/Zipkin/Instrumentation/Http/Server/Psr15/Middleware.php index 68d36022..0fb168d7 100644 --- a/src/Zipkin/Instrumentation/Http/Server/Psr15/Middleware.php +++ b/src/Zipkin/Instrumentation/Http/Server/Psr15/Middleware.php @@ -6,7 +6,10 @@ use Zipkin\Tracer; use Zipkin\SpanCustomizerShield; +use Zipkin\Span; use Zipkin\Propagation\TraceContext; +use Zipkin\Propagation\SamplingFlags; +use Zipkin\Propagation\DefaultSamplingFlags; use Zipkin\Kind; use Zipkin\Instrumentation\Http\Server\Parser; use Zipkin\Instrumentation\Http\Server\HttpServerTracing; @@ -50,17 +53,7 @@ public function process(ServerRequestInterface $request, RequestHandlerInterface { $extractedContext = ($this->extractor)($request); - if ($extractedContext instanceof TraceContext) { - $span = $this->tracer->joinSpan($extractedContext); - } elseif ($this->requestSampler === null) { - $span = $this->tracer->nextSpan($extractedContext); - } else { - $span = $this->tracer->nextSpanWithSampler( - $this->requestSampler, - [$request], - $extractedContext - ); - } + $span = $this->nextSpan($extractedContext, $request); $scopeCloser = $this->tracer->openScope($span); if ($span->isNoop()) { @@ -89,4 +82,22 @@ public function process(ServerRequestInterface $request, RequestHandlerInterface $scopeCloser(); } } + + private function nextSpan(?SamplingFlags $extractedContext, ServerRequestInterface $request): Span + { + if ($extractedContext instanceof TraceContext) { + return $this->tracer->joinSpan($extractedContext); + } + + $extractedContext = $extractedContext ?? DefaultSamplingFlags::createAsEmpty(); + if ($this->requestSampler === null) { + return $this->tracer->nextSpan($extractedContext); + } + + return $this->tracer->nextSpanWithSampler( + $this->requestSampler, + [$request], + $extractedContext + ); + } } diff --git a/tests/Unit/Instrumentation/Http/Server/Psr15/ServerTest.php b/tests/Unit/Instrumentation/Http/Server/Psr15/MiddlewareTest.php similarity index 59% rename from tests/Unit/Instrumentation/Http/Server/Psr15/ServerTest.php rename to tests/Unit/Instrumentation/Http/Server/Psr15/MiddlewareTest.php index db3417c4..86af8813 100644 --- a/tests/Unit/Instrumentation/Http/Server/Psr15/ServerTest.php +++ b/tests/Unit/Instrumentation/Http/Server/Psr15/MiddlewareTest.php @@ -5,8 +5,11 @@ namespace ZipkinTests\Unit\Instrumentation\Http\Server\Psr15; use Zipkin\TracingBuilder; +use Zipkin\Span; use Zipkin\Samplers\BinarySampler; use Zipkin\Reporters\InMemory; +use Zipkin\Propagation\TraceContext; +use Zipkin\Propagation\DefaultSamplingFlags; use Zipkin\Instrumentation\Http\Server\Psr15\Middleware; use Zipkin\Instrumentation\Http\Server\Psr15\DefaultParser; use Zipkin\Instrumentation\Http\Server\HttpServerTracing; @@ -20,7 +23,7 @@ final class ServerTest extends TestCase { - private static function createTracing(): array + private static function createTracing(callable $requestSampler = null): array { $reporter = new InMemory(); $tracing = @@ -31,7 +34,7 @@ private static function createTracing(): array $tracer = $tracing->getTracer(); return [ - new HttpServerTracing($tracing, new DefaultParser), + new HttpServerTracing($tracing, new DefaultParser, $requestSampler), static function () use ($tracer, $reporter): array { $tracer->flush(); return $reporter->flush(); @@ -116,4 +119,61 @@ public function testMiddlewareParsesRequestSuccessfullyWithNon2xx() 'error' => '404' ], $span['tags']); } + + public function testMiddlewareKeepsContextForJoinSpan() + { + $request = new ServerRequest('GET', 'http://mytest'); + + $extractedContext = TraceContext::createAsRoot(DefaultSamplingFlags::createAsSampled()); + + list($tracing) = self::createTracing(); + $middleware = new Middleware($tracing); + + /** + * @var Span $span + */ + $span = $this->invokePrivateMethod($middleware, 'nextSpan', [$extractedContext, $request]); + $this->assertSame($extractedContext->getTraceId(), $span->getContext()->getTraceId()); + } + + /** + * @dataProvider middlewareNextSpanProvider + */ + public function testMiddlewareNextSpanResolvesSampling( + $extractedContext, + callable $requestSampler, + ?bool $expectedSampling + ) { + $request = new ServerRequest('GET', 'http://mytest'); + + list($tracing) = self::createTracing($requestSampler); + $middleware = new Middleware($tracing); + + $span = $this->invokePrivateMethod($middleware, 'nextSpan', [$extractedContext, $request]); + $this->assertEquals($expectedSampling, $span->getContext()->isSampled()); + } + + public function middlewareNextSpanProvider(): array + { + return [ + //[$extractedContext, $requestSampler, $expectedSampling] + 'no context becomes sampled' => [null, function () { + return true; + }, true], + 'not sampled becomes sampled' => [DefaultSamplingFlags::createAsNotSampled(), function () { + return true; + }, true], + 'sampled remains the same' => [DefaultSamplingFlags::createAsSampled(), function () { + return false; + }, true], + ]; + } + + private function invokePrivateMethod(&$object, $methodName, array $parameters = array()) + { + $reflection = new \ReflectionClass(get_class($object)); + $method = $reflection->getMethod($methodName); + $method->setAccessible(true); + return $method->invokeArgs($object, $parameters); + } } From d737d34a79626ef59d0eb1593fc309235d4773f3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Carlos=20Ch=C3=A1vez?= Date: Tue, 14 Jul 2020 09:51:59 +0200 Subject: [PATCH 09/20] chore(http-server): small tweaks to tests. --- src/Zipkin/Instrumentation/Http/Server/HttpServerTracing.php | 2 +- .../Http/Server/{ServerTest.php => MiddlewareTest.php} | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) rename tests/Integration/Instrumentation/Http/Server/{ServerTest.php => MiddlewareTest.php} (98%) diff --git a/src/Zipkin/Instrumentation/Http/Server/HttpServerTracing.php b/src/Zipkin/Instrumentation/Http/Server/HttpServerTracing.php index a609026a..b0ab8232 100644 --- a/src/Zipkin/Instrumentation/Http/Server/HttpServerTracing.php +++ b/src/Zipkin/Instrumentation/Http/Server/HttpServerTracing.php @@ -7,7 +7,7 @@ use Zipkin\Tracing; /** - * Tracing includes all the elements needed to trace a HTTP server + * HttpServerTracing includes all the elements needed to trace a HTTP server * middleware or request handler. */ class HttpServerTracing diff --git a/tests/Integration/Instrumentation/Http/Server/ServerTest.php b/tests/Integration/Instrumentation/Http/Server/MiddlewareTest.php similarity index 98% rename from tests/Integration/Instrumentation/Http/Server/ServerTest.php rename to tests/Integration/Instrumentation/Http/Server/MiddlewareTest.php index 4adc48ec..7dca1bb0 100644 --- a/tests/Integration/Instrumentation/Http/Server/ServerTest.php +++ b/tests/Integration/Instrumentation/Http/Server/MiddlewareTest.php @@ -23,7 +23,7 @@ use Middlewares; use FastRoute\RouteCollector; -final class ServerTest extends TestCase +final class MiddlewareTest extends TestCase { private static function createTracing(Parser $parser): array { From d15581c37f6deb7bc494043227d4fc8803259774 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Carlos=20Ch=C3=A1vez?= Date: Wed, 15 Jul 2020 22:32:12 +0200 Subject: [PATCH 10/20] tests(http-server): improves test naming for client and middleware. --- src/Zipkin/Instrumentation/Http/Client/Psr18/Client.php | 8 +++----- .../Instrumentation/Http/Server/Psr15/Middleware.php | 4 ++-- .../Instrumentation/Http/Server/MiddlewareTest.php | 2 +- 3 files changed, 6 insertions(+), 8 deletions(-) diff --git a/src/Zipkin/Instrumentation/Http/Client/Psr18/Client.php b/src/Zipkin/Instrumentation/Http/Client/Psr18/Client.php index c8b2450d..e5328801 100644 --- a/src/Zipkin/Instrumentation/Http/Client/Psr18/Client.php +++ b/src/Zipkin/Instrumentation/Http/Client/Psr18/Client.php @@ -42,11 +42,9 @@ final class Client implements ClientInterface */ private $requestSampler; - public function __construct( - ClientInterface $delegate, - HttpClientTracing $tracing - ) { - $this->delegate = $delegate; + public function __construct(ClientInterface $client, HttpClientTracing $tracing) + { + $this->delegate = $client; $this->injector = $tracing->getTracing()->getPropagation()->getInjector(new RequestHeaders()); $this->tracer = $tracing->getTracing()->getTracer(); $this->parser = $tracing->getParser(); diff --git a/src/Zipkin/Instrumentation/Http/Server/Psr15/Middleware.php b/src/Zipkin/Instrumentation/Http/Server/Psr15/Middleware.php index 0fb168d7..44b06207 100644 --- a/src/Zipkin/Instrumentation/Http/Server/Psr15/Middleware.php +++ b/src/Zipkin/Instrumentation/Http/Server/Psr15/Middleware.php @@ -27,7 +27,7 @@ final class Middleware implements MiddlewareInterface private $tracer; /** - * @var callable + * @var callable(ServerRequestInterface):SamplingFlags */ private $extractor; @@ -37,7 +37,7 @@ final class Middleware implements MiddlewareInterface private $parser; /** - * @var callable|null + * @var (callable(ServerRequestInterface):?bool)|null */ private $requestSampler; diff --git a/tests/Integration/Instrumentation/Http/Server/MiddlewareTest.php b/tests/Integration/Instrumentation/Http/Server/MiddlewareTest.php index 7dca1bb0..d3c4d6f4 100644 --- a/tests/Integration/Instrumentation/Http/Server/MiddlewareTest.php +++ b/tests/Integration/Instrumentation/Http/Server/MiddlewareTest.php @@ -44,7 +44,7 @@ static function () use ($tracer, $reporter): array { ]; } - public function testMiddleware() + public function testMiddlewareRecordsRequestSuccessfully() { $parser = new class() extends DefaultParser { public function request($request, TraceContext $context, SpanCustomizer $span): void From b3866a9ad4e0acf77dcd2e6fecbd6c273f924797 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Carlos=20Ch=C3=A1vez?= Date: Wed, 15 Jul 2020 23:01:37 +0200 Subject: [PATCH 11/20] chore: moves RequestHeaders into propagation folder, also adds typing to the default parsers. --- src/Zipkin/Instrumentation/Http/Client/Psr18/Client.php | 1 + .../Instrumentation/Http/Client/Psr18/DefaultParser.php | 9 +++++++++ .../Client/Psr18/{ => Propagation}/RequestHeaders.php | 2 +- .../Instrumentation/Http/Server/Psr15/DefaultParser.php | 9 +++++++++ .../Instrumentation/Http/Server/Psr15/Middleware.php | 1 + .../Server/Psr15/{ => Propagation}/RequestHeaders.php | 2 +- 6 files changed, 22 insertions(+), 2 deletions(-) rename src/Zipkin/Instrumentation/Http/Client/Psr18/{ => Propagation}/RequestHeaders.php (82%) rename src/Zipkin/Instrumentation/Http/Server/Psr15/{ => Propagation}/RequestHeaders.php (82%) diff --git a/src/Zipkin/Instrumentation/Http/Client/Psr18/Client.php b/src/Zipkin/Instrumentation/Http/Client/Psr18/Client.php index e5328801..3b9f56ae 100644 --- a/src/Zipkin/Instrumentation/Http/Client/Psr18/Client.php +++ b/src/Zipkin/Instrumentation/Http/Client/Psr18/Client.php @@ -8,6 +8,7 @@ use Zipkin\SpanCustomizerShield; use Zipkin\Propagation\TraceContext; use Zipkin\Kind; +use Zipkin\Instrumentation\Http\Client\Psr18\Propagation\RequestHeaders; use Zipkin\Instrumentation\Http\Client\Parser; use Zipkin\Instrumentation\Http\Client\HttpClientTracing; use Throwable; diff --git a/src/Zipkin/Instrumentation/Http/Client/Psr18/DefaultParser.php b/src/Zipkin/Instrumentation/Http/Client/Psr18/DefaultParser.php index d23389de..57a601b4 100644 --- a/src/Zipkin/Instrumentation/Http/Client/Psr18/DefaultParser.php +++ b/src/Zipkin/Instrumentation/Http/Client/Psr18/DefaultParser.php @@ -18,12 +18,18 @@ */ class DefaultParser implements Parser { + /** + * @param RequestInterface $request + */ public function spanName($request): string { self::assertRequestType($request); return $request->getMethod(); } + /** + * @param RequestInterface $request + */ public function request($request, TraceContext $context, SpanCustomizer $span): void { self::assertRequestType($request); @@ -31,6 +37,9 @@ public function request($request, TraceContext $context, SpanCustomizer $span): $span->tag(Tags\HTTP_PATH, $request->getUri()->getPath() ?: "/"); } + /** + * @param ResponseInterface $response + */ public function response($response, TraceContext $context, SpanCustomizer $span): void { self::assertResponseType($response); diff --git a/src/Zipkin/Instrumentation/Http/Client/Psr18/RequestHeaders.php b/src/Zipkin/Instrumentation/Http/Client/Psr18/Propagation/RequestHeaders.php similarity index 82% rename from src/Zipkin/Instrumentation/Http/Client/Psr18/RequestHeaders.php rename to src/Zipkin/Instrumentation/Http/Client/Psr18/Propagation/RequestHeaders.php index fab06551..fa4088da 100644 --- a/src/Zipkin/Instrumentation/Http/Client/Psr18/RequestHeaders.php +++ b/src/Zipkin/Instrumentation/Http/Client/Psr18/Propagation/RequestHeaders.php @@ -2,7 +2,7 @@ declare(strict_types=1); -namespace Zipkin\Instrumentation\Http\Client\Psr18; +namespace Zipkin\Instrumentation\Http\Client\Psr18\Propagation; use Zipkin\Propagation\RequestHeaders as BaseRequestHeaders; use Zipkin\Propagation\RemoteSetter; diff --git a/src/Zipkin/Instrumentation/Http/Server/Psr15/DefaultParser.php b/src/Zipkin/Instrumentation/Http/Server/Psr15/DefaultParser.php index ba6d9257..345d53bc 100644 --- a/src/Zipkin/Instrumentation/Http/Server/Psr15/DefaultParser.php +++ b/src/Zipkin/Instrumentation/Http/Server/Psr15/DefaultParser.php @@ -18,12 +18,18 @@ */ class DefaultParser implements Parser { + /** + * @param ServerRequestInterface $request + */ public function spanName($request): string { self::assertRequestType($request); return $request->getMethod(); } + /** + * @param ServerRequestInterface $request + */ public function request($request, TraceContext $context, SpanCustomizer $span): void { self::assertRequestType($request); @@ -31,6 +37,9 @@ public function request($request, TraceContext $context, SpanCustomizer $span): $span->tag(Tags\HTTP_PATH, $request->getUri()->getPath() ?: "/"); } + /** + * @param ResponseInterface $response + */ public function response($response, TraceContext $context, SpanCustomizer $span): void { self::assertResponseType($response); diff --git a/src/Zipkin/Instrumentation/Http/Server/Psr15/Middleware.php b/src/Zipkin/Instrumentation/Http/Server/Psr15/Middleware.php index 44b06207..a833811b 100644 --- a/src/Zipkin/Instrumentation/Http/Server/Psr15/Middleware.php +++ b/src/Zipkin/Instrumentation/Http/Server/Psr15/Middleware.php @@ -11,6 +11,7 @@ use Zipkin\Propagation\SamplingFlags; use Zipkin\Propagation\DefaultSamplingFlags; use Zipkin\Kind; +use Zipkin\Instrumentation\Http\Server\Psr15\Propagation\RequestHeaders; use Zipkin\Instrumentation\Http\Server\Parser; use Zipkin\Instrumentation\Http\Server\HttpServerTracing; use Throwable; diff --git a/src/Zipkin/Instrumentation/Http/Server/Psr15/RequestHeaders.php b/src/Zipkin/Instrumentation/Http/Server/Psr15/Propagation/RequestHeaders.php similarity index 82% rename from src/Zipkin/Instrumentation/Http/Server/Psr15/RequestHeaders.php rename to src/Zipkin/Instrumentation/Http/Server/Psr15/Propagation/RequestHeaders.php index 5a6311d9..88957395 100644 --- a/src/Zipkin/Instrumentation/Http/Server/Psr15/RequestHeaders.php +++ b/src/Zipkin/Instrumentation/Http/Server/Psr15/Propagation/RequestHeaders.php @@ -2,7 +2,7 @@ declare(strict_types=1); -namespace Zipkin\Instrumentation\Http\Server\Psr15; +namespace Zipkin\Instrumentation\Http\Server\Psr15\Propagation; use Zipkin\Propagation\RequestHeaders as BaseRequestHeaders; use Zipkin\Propagation\RemoteSetter; From 398d4cb1e08c480b6f54212dc852c2859ab3e551 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Carlos=20Ch=C3=A1vez?= Date: Mon, 20 Jul 2020 14:27:40 +0200 Subject: [PATCH 12/20] feat: adds request/response objects to HTTP instrumentation. --- phpstan.neon | 10 --- .../Http/Client/DefaultParser.php | 40 +++++++++++ .../Http/Client/HttpClientTracing.php | 10 +-- .../Instrumentation/Http/Client/Parser.php | 15 ++-- .../Http/Client/Psr18/Client.php | 11 +-- .../Http/Client/Psr18/DefaultParser.php | 61 ----------------- .../Http/Client/Psr18/Request.php | 63 +++++++++++++++++ .../Http/Client/Psr18/Response.php | 56 +++++++++++++++ .../Instrumentation/Http/Client/Request.php | 11 +++ .../Instrumentation/Http/Client/Response.php | 11 +++ src/Zipkin/Instrumentation/Http/Request.php | 61 +++++++++++++++++ src/Zipkin/Instrumentation/Http/Response.php | 27 ++++++++ .../Http/Server/DefaultParser.php | 41 +++++++++++ .../Http/Server/HttpServerTracing.php | 4 +- .../Instrumentation/Http/Server/Parser.php | 13 +--- .../Http/Server/Psr15/DefaultParser.php | 61 ----------------- .../Http/Server/Psr15/Middleware.php | 11 +-- .../Http/Server/Psr15/Request.php | 68 +++++++++++++++++++ .../Http/Server/Psr15/Response.php | 64 +++++++++++++++++ .../Instrumentation/Http/Server/Request.php | 25 +++++++ .../Instrumentation/Http/Server/Response.php | 25 +++++++ .../Http/Server/MiddlewareTest.php | 9 ++- .../Http/Client/Psr18/ClientTest.php | 2 +- .../Http/Server/Psr15/MiddlewareTest.php | 2 +- 24 files changed, 526 insertions(+), 175 deletions(-) create mode 100644 src/Zipkin/Instrumentation/Http/Client/DefaultParser.php delete mode 100644 src/Zipkin/Instrumentation/Http/Client/Psr18/DefaultParser.php create mode 100644 src/Zipkin/Instrumentation/Http/Client/Psr18/Request.php create mode 100644 src/Zipkin/Instrumentation/Http/Client/Psr18/Response.php create mode 100644 src/Zipkin/Instrumentation/Http/Client/Request.php create mode 100644 src/Zipkin/Instrumentation/Http/Client/Response.php create mode 100644 src/Zipkin/Instrumentation/Http/Request.php create mode 100644 src/Zipkin/Instrumentation/Http/Response.php create mode 100644 src/Zipkin/Instrumentation/Http/Server/DefaultParser.php delete mode 100644 src/Zipkin/Instrumentation/Http/Server/Psr15/DefaultParser.php create mode 100644 src/Zipkin/Instrumentation/Http/Server/Psr15/Request.php create mode 100644 src/Zipkin/Instrumentation/Http/Server/Psr15/Response.php create mode 100644 src/Zipkin/Instrumentation/Http/Server/Request.php create mode 100644 src/Zipkin/Instrumentation/Http/Server/Response.php diff --git a/phpstan.neon b/phpstan.neon index 19258744..8a28f522 100644 --- a/phpstan.neon +++ b/phpstan.neon @@ -29,17 +29,7 @@ parameters: # In general types are desired but carrier is an special case message: '#has parameter \$carrier with no typehint specified#' path: src/Zipkin/* - - - # Somehow this started failing - message: '#Constant .* already defined#' - path: src/Zipkin - # SpanCustomizer is definitively not null message: '#Parameter \#3 \$ of callable callable\(.*, Zipkin\\Propagation\\TraceContext, Zipkin\\SpanCustomizer\): void expects Zipkin\\SpanCustomizer, Zipkin\\SpanCustomizerShield\|null given.#' path: src/Zipkin/Tracer - - - message: '#Method .* with no typehint specified.#' - path: src/Zipkin/Instrumentation/Http/Server - - - message: '#Method .* with no typehint specified.#' - path: src/Zipkin/Instrumentation/Http/Client \ No newline at end of file diff --git a/src/Zipkin/Instrumentation/Http/Client/DefaultParser.php b/src/Zipkin/Instrumentation/Http/Client/DefaultParser.php new file mode 100644 index 00000000..44f774fd --- /dev/null +++ b/src/Zipkin/Instrumentation/Http/Client/DefaultParser.php @@ -0,0 +1,40 @@ +getMethod(); + } + + public function request(Request $request, TraceContext $context, SpanCustomizer $span): void + { + $span->tag(Tags\HTTP_METHOD, $request->getMethod()); + $span->tag(Tags\HTTP_PATH, $request->getPath() ?: "/"); + } + + public function response(Response $response, TraceContext $context, SpanCustomizer $span): void + { + $statusCode = $response->getStatusCode(); + $span->tag(Tags\HTTP_STATUS_CODE, (string) $statusCode); + if ($statusCode > 399) { + $span->tag(Tags\ERROR, (string) $statusCode); + } + } +} diff --git a/src/Zipkin/Instrumentation/Http/Client/HttpClientTracing.php b/src/Zipkin/Instrumentation/Http/Client/HttpClientTracing.php index 4cbb6f31..b9a03aba 100644 --- a/src/Zipkin/Instrumentation/Http/Client/HttpClientTracing.php +++ b/src/Zipkin/Instrumentation/Http/Client/HttpClientTracing.php @@ -5,7 +5,7 @@ namespace Zipkin\Instrumentation\Http\Client; use Zipkin\Tracing; -use Psr\Http\Message\RequestInterface; +use Zipkin\Instrumentation\Http\Request; /** * ClientTracing includes all the elements needed to instrument a @@ -27,17 +27,17 @@ class HttpClientTracing * function that decides to sample or not an unsampled * request. * - * @var callable(RequestInterface):?bool|null + * @var callable(Request):?bool|null */ private $requestSampler; public function __construct( Tracing $tracing, - Parser $parser, + Parser $parser = null, callable $requestSampler = null ) { $this->tracing = $tracing; - $this->parser = $parser; + $this->parser = $parser ?? new DefaultParser; $this->requestSampler = $requestSampler; } @@ -47,7 +47,7 @@ public function getTracing(): Tracing } /** - * @return (callable(RequestInterface):?bool)|null + * @return (callable(Request):?bool)|null */ public function getRequestSampler(): ?callable { diff --git a/src/Zipkin/Instrumentation/Http/Client/Parser.php b/src/Zipkin/Instrumentation/Http/Client/Parser.php index af8462fc..ec29c5c0 100644 --- a/src/Zipkin/Instrumentation/Http/Client/Parser.php +++ b/src/Zipkin/Instrumentation/Http/Client/Parser.php @@ -6,17 +6,12 @@ use Zipkin\SpanCustomizer; use Zipkin\Propagation\TraceContext; +use Zipkin\Instrumentation\Http\Response; +use Zipkin\Instrumentation\Http\Request; /** * Parser includes the methods to obtain meaningful span information * out of HTTP request/response elements. - * - * $request and $response objects are not typed on purpose as different - * frameworks can use different request models (e.g. symfony uses a list - * parameters including method, url and options). Since PHP does not support - * generics this is the only way to make this interface reusable but - * implementors have to make sure the request/response objects have the right - * type. */ interface Parser { @@ -24,7 +19,7 @@ interface Parser * spanName returns an appropiate span name based on the request, * usually the HTTP method is good enough (e.g GET or POST). */ - public function spanName($request): string; + public function spanName(Request $request): string; /** * request parses the incoming data related to a request in order to add @@ -33,7 +28,7 @@ public function spanName($request): string; * Basic data being tagged is HTTP method, HTTP path but other information * such as query parameters can be added to enrich the span information. */ - public function request($request, TraceContext $context, SpanCustomizer $span): void; + public function request(Request $request, TraceContext $context, SpanCustomizer $span): void; /** @@ -44,5 +39,5 @@ public function request($request, TraceContext $context, SpanCustomizer $span): * as any response header or more granular information based on the response * payload can be added. */ - public function response($response, TraceContext $context, SpanCustomizer $span): void; + public function response(Response $response, TraceContext $context, SpanCustomizer $span): void; } diff --git a/src/Zipkin/Instrumentation/Http/Client/Psr18/Client.php b/src/Zipkin/Instrumentation/Http/Client/Psr18/Client.php index 3b9f56ae..0010aced 100644 --- a/src/Zipkin/Instrumentation/Http/Client/Psr18/Client.php +++ b/src/Zipkin/Instrumentation/Http/Client/Psr18/Client.php @@ -39,7 +39,7 @@ final class Client implements ClientInterface private $parser; /** - * @var (callable(RequestInterface):?bool)|null + * @var (callable(Request):?bool)|null */ private $requestSampler; @@ -54,12 +54,13 @@ public function __construct(ClientInterface $client, HttpClientTracing $tracing) public function sendRequest(RequestInterface $request): ResponseInterface { + $parsedRequest = new Request($request); if ($this->requestSampler === null) { $span = $this->tracer->nextSpan(); } else { $span = $this->tracer->nextSpanWithSampler( $this->requestSampler, - [$request] + [$parsedRequest] ); } @@ -77,13 +78,13 @@ public function sendRequest(RequestInterface $request): ResponseInterface // If span is NOOP it does not make sense to add customizations // to it like tags or annotations. $spanCustomizer = new SpanCustomizerShield($span); - $span->setName($this->parser->spanName($request)); - $this->parser->request($request, $span->getContext(), $spanCustomizer); + $span->setName($this->parser->spanName($parsedRequest)); + $this->parser->request($parsedRequest, $span->getContext(), $spanCustomizer); try { $span->start(); $response = $this->delegate->sendRequest($request); - $this->parser->response($response, $span->getContext(), $spanCustomizer); + $this->parser->response(new Response($response, $parsedRequest), $span->getContext(), $spanCustomizer); return $response; } catch (Throwable $e) { $span->setError($e); diff --git a/src/Zipkin/Instrumentation/Http/Client/Psr18/DefaultParser.php b/src/Zipkin/Instrumentation/Http/Client/Psr18/DefaultParser.php deleted file mode 100644 index 57a601b4..00000000 --- a/src/Zipkin/Instrumentation/Http/Client/Psr18/DefaultParser.php +++ /dev/null @@ -1,61 +0,0 @@ -getMethod(); - } - - /** - * @param RequestInterface $request - */ - public function request($request, TraceContext $context, SpanCustomizer $span): void - { - self::assertRequestType($request); - $span->tag(Tags\HTTP_METHOD, $request->getMethod()); - $span->tag(Tags\HTTP_PATH, $request->getUri()->getPath() ?: "/"); - } - - /** - * @param ResponseInterface $response - */ - public function response($response, TraceContext $context, SpanCustomizer $span): void - { - self::assertResponseType($response); - $span->tag(Tags\HTTP_STATUS_CODE, (string) $response->getStatusCode()); - if ($response->getStatusCode() > 399) { - $span->tag(Tags\ERROR, (string) $response->getStatusCode()); - } - } - - // Makes sure the type for request object is correct. - private static function assertRequestType(RequestInterface $request): void - { - } - - // Makes sure the type for response object is correct. - private static function assertResponseType(ResponseInterface $response): void - { - } -} diff --git a/src/Zipkin/Instrumentation/Http/Client/Psr18/Request.php b/src/Zipkin/Instrumentation/Http/Client/Psr18/Request.php new file mode 100644 index 00000000..4b8049c7 --- /dev/null +++ b/src/Zipkin/Instrumentation/Http/Client/Psr18/Request.php @@ -0,0 +1,63 @@ +delegate = $delegate; + } + + /** + * {@inheritdoc} + */ + public function getMethod(): string + { + return $this->delegate->getMethod(); + } + + /** + * {@inheritdoc} + */ + public function getPath(): ?string + { + return $this->delegate->getUri()->getPath(); + } + + /** + * {@inheritdoc} + */ + public function getUrl(): string + { + return $this->delegate->getUri()->__toString(); + } + + /** + * {@inheritdoc} + */ + public function getHeader(string $name): string + { + return $this->delegate->getHeaderLine($name); + } + + /** + * {@inheritdoc} + * + * @return RequestInterface + */ + public function unwrap() + { + return $this->delegate; + } +} diff --git a/src/Zipkin/Instrumentation/Http/Client/Psr18/Response.php b/src/Zipkin/Instrumentation/Http/Client/Psr18/Response.php new file mode 100644 index 00000000..2802a81c --- /dev/null +++ b/src/Zipkin/Instrumentation/Http/Client/Psr18/Response.php @@ -0,0 +1,56 @@ +delegate = $delegate; + $this->request = $request; + } + + /** + * {@inheritdoc} + */ + public function getRequest(): ?HttpRequest + { + return $this->request; + } + + /** + * {@inheritdoc} + */ + public function getStatusCode(): int + { + return $this->delegate->getStatusCode(); + } + + /** + * {@inheritdoc} + * + * @return ResponseInterface + */ + public function unwrap() + { + return $this->delegate; + } +} diff --git a/src/Zipkin/Instrumentation/Http/Client/Request.php b/src/Zipkin/Instrumentation/Http/Client/Request.php new file mode 100644 index 00000000..e2c821e2 --- /dev/null +++ b/src/Zipkin/Instrumentation/Http/Client/Request.php @@ -0,0 +1,11 @@ +Conventionally associated with the key "http.method" + * + *

Note

+ *

It is part of the HTTP RFC + * that an HTTP method is case-sensitive. Do not downcase results. If you do, not only will + * integration tests fail, but you will surprise any consumers who expect compliant results. + * + * @see Zipkin\Tags\HTTP_METHOD + */ + public function getMethod(): string; + + /** + * The absolute http path, without any query parameters. Ex. "/objects/abcd-ff" + * + *

Conventionally associated with the key "http.path" + * + *

{@code null} could mean not applicable to the HTTP method (ex CONNECT). + * + *

Implementation notes

+ * Some HTTP client abstractions, return the input as opposed to + * the absolute path. One common problem is a path requested as "", not "/". When that's the case, + * normalize "" to "/". This ensures values are consistent with wire-level clients and behaviour + * consistent with RFC 7230 Section 2.7.3. + * + * @see Tags\HTTP_PATH + */ + public function getPath(): ?string; + + /** + * The entire URL, including the scheme, host and query parameters if available. + * + *

Conventionally associated with the key "http.url" + * + * @see Tags#HTTP_URL + */ + public function getUrl(): string; + + /** + * Returns one value corresponding to the specified header, or null. + */ + public function getHeader(string $name): string; + + /** + * @return mixed the underlying request object or {@code null} if there is none. + */ + public function unwrap(); +} diff --git a/src/Zipkin/Instrumentation/Http/Response.php b/src/Zipkin/Instrumentation/Http/Response.php new file mode 100644 index 00000000..f924d127 --- /dev/null +++ b/src/Zipkin/Instrumentation/Http/Response.php @@ -0,0 +1,27 @@ +Conventionally associated with the key "http.status_code" + * + * @since 5.10 + */ + public function getStatusCode(): int; + + /** + * @return mixed the underlying response object or {@code null} if there is none. + */ + public function unwrap(); +} diff --git a/src/Zipkin/Instrumentation/Http/Server/DefaultParser.php b/src/Zipkin/Instrumentation/Http/Server/DefaultParser.php new file mode 100644 index 00000000..8b1dcc53 --- /dev/null +++ b/src/Zipkin/Instrumentation/Http/Server/DefaultParser.php @@ -0,0 +1,41 @@ +getMethod() . ($request->getRoute() === null ? '' : ' ' . $request->getRoute()); + } + + public function request(Request $request, TraceContext $context, SpanCustomizer $span): void + { + $span->tag(Tags\HTTP_METHOD, $request->getMethod()); + $span->tag(Tags\HTTP_PATH, $request->getPath() ?: '/'); + } + + public function response(Response $response, TraceContext $context, SpanCustomizer $span): void + { + $span->tag(Tags\HTTP_STATUS_CODE, (string) $response->getStatusCode()); + if ($response->getStatusCode() > 399) { + $span->tag(Tags\ERROR, (string) $response->getStatusCode()); + } + + if ($response->getRoute() !== null && $response->getRequest() !== null) { + $span->setName($response->getRequest()->getMethod() . ' ' . $response->getRoute()); + } + } +} diff --git a/src/Zipkin/Instrumentation/Http/Server/HttpServerTracing.php b/src/Zipkin/Instrumentation/Http/Server/HttpServerTracing.php index b0ab8232..f350dddd 100644 --- a/src/Zipkin/Instrumentation/Http/Server/HttpServerTracing.php +++ b/src/Zipkin/Instrumentation/Http/Server/HttpServerTracing.php @@ -26,7 +26,7 @@ class HttpServerTracing * function that decides to sample or not an unsampled * request. * - * @var (callable(mixed):?bool)|null + * @var (callable(Request):?bool)|null */ private $requestSampler; @@ -46,7 +46,7 @@ public function getTracing(): Tracing } /** - * @return (callable(mixed):?bool)|null + * @return (callable(Request):?bool)|null */ public function getRequestSampler(): ?callable { diff --git a/src/Zipkin/Instrumentation/Http/Server/Parser.php b/src/Zipkin/Instrumentation/Http/Server/Parser.php index 051246d0..ddfac330 100644 --- a/src/Zipkin/Instrumentation/Http/Server/Parser.php +++ b/src/Zipkin/Instrumentation/Http/Server/Parser.php @@ -10,13 +10,6 @@ /** * Parser includes the methods to obtain meaningful span information * out of HTTP request/response elements. - * - * $request and $response objects are not typed on purpose as different - * frameworks can use different request models (e.g. symfony uses a list - * parameters including method, url and options). Since PHP does not support - * generics this is the only way to make this interface reusable but - * implementors have to make sure the request/response objects have the right - * type. */ interface Parser { @@ -25,7 +18,7 @@ interface Parser * usually the HTTP method is enough (e.g GET or POST) but ideally * the http.route is desired (e.g. /user/{user_id}). */ - public function spanName($request): string; + public function spanName(Request $request): string; /** * request parses the incoming data related to a request in order to add @@ -34,7 +27,7 @@ public function spanName($request): string; * Basic data being tagged is HTTP method, HTTP path but other information * such as query parameters can be added to enrich the span information. */ - public function request($request, TraceContext $context, SpanCustomizer $span): void; + public function request(Request $request, TraceContext $context, SpanCustomizer $span): void; /** @@ -45,5 +38,5 @@ public function request($request, TraceContext $context, SpanCustomizer $span): * as any response header or more granular information based on the response * payload can be added. */ - public function response($response, TraceContext $context, SpanCustomizer $span): void; + public function response(Response $response, TraceContext $context, SpanCustomizer $span): void; } diff --git a/src/Zipkin/Instrumentation/Http/Server/Psr15/DefaultParser.php b/src/Zipkin/Instrumentation/Http/Server/Psr15/DefaultParser.php deleted file mode 100644 index 345d53bc..00000000 --- a/src/Zipkin/Instrumentation/Http/Server/Psr15/DefaultParser.php +++ /dev/null @@ -1,61 +0,0 @@ -getMethod(); - } - - /** - * @param ServerRequestInterface $request - */ - public function request($request, TraceContext $context, SpanCustomizer $span): void - { - self::assertRequestType($request); - $span->tag(Tags\HTTP_METHOD, $request->getMethod()); - $span->tag(Tags\HTTP_PATH, $request->getUri()->getPath() ?: "/"); - } - - /** - * @param ResponseInterface $response - */ - public function response($response, TraceContext $context, SpanCustomizer $span): void - { - self::assertResponseType($response); - $span->tag(Tags\HTTP_STATUS_CODE, (string) $response->getStatusCode()); - if ($response->getStatusCode() > 399) { - $span->tag(Tags\ERROR, (string) $response->getStatusCode()); - } - } - - // Makes sure the type for request object is correct. - private static function assertRequestType(ServerRequestInterface $request): void - { - } - - // Makes sure the type for response object is correct. - private static function assertResponseType(ResponseInterface $response): void - { - } -} diff --git a/src/Zipkin/Instrumentation/Http/Server/Psr15/Middleware.php b/src/Zipkin/Instrumentation/Http/Server/Psr15/Middleware.php index a833811b..65669ba8 100644 --- a/src/Zipkin/Instrumentation/Http/Server/Psr15/Middleware.php +++ b/src/Zipkin/Instrumentation/Http/Server/Psr15/Middleware.php @@ -11,6 +11,7 @@ use Zipkin\Propagation\SamplingFlags; use Zipkin\Propagation\DefaultSamplingFlags; use Zipkin\Kind; +use Zipkin\Instrumentation\Http\Server\Request as ServerRequest; use Zipkin\Instrumentation\Http\Server\Psr15\Propagation\RequestHeaders; use Zipkin\Instrumentation\Http\Server\Parser; use Zipkin\Instrumentation\Http\Server\HttpServerTracing; @@ -38,7 +39,7 @@ final class Middleware implements MiddlewareInterface private $parser; /** - * @var (callable(ServerRequestInterface):?bool)|null + * @var (callable(ServerRequest):?bool)|null */ private $requestSampler; @@ -66,14 +67,16 @@ public function process(ServerRequestInterface $request, RequestHandlerInterface } } + $parsedRequest = new Request($request); + $span->setKind(Kind\SERVER); $spanCustomizer = new SpanCustomizerShield($span); - $span->setName($this->parser->spanName($request)); - $this->parser->request($request, $span->getContext(), $spanCustomizer); + $span->setName($this->parser->spanName($parsedRequest)); + $this->parser->request($parsedRequest, $span->getContext(), $spanCustomizer); try { $response = $handler->handle($request); - $this->parser->response($response, $span->getContext(), $spanCustomizer); + $this->parser->response(new Response($response, $parsedRequest), $span->getContext(), $spanCustomizer); return $response; } catch (Throwable $e) { $span->setError($e); diff --git a/src/Zipkin/Instrumentation/Http/Server/Psr15/Request.php b/src/Zipkin/Instrumentation/Http/Server/Psr15/Request.php new file mode 100644 index 00000000..51d7439e --- /dev/null +++ b/src/Zipkin/Instrumentation/Http/Server/Psr15/Request.php @@ -0,0 +1,68 @@ +delegate = $delegate; + } + + /** + * {@inheritdoc} + */ + public function getMethod(): string + { + return $this->delegate->getMethod(); + } + + /** + * {@inheritdoc} + */ + public function getPath(): ?string + { + return $this->delegate->getUri()->getPath(); + } + + /** + * {@inheritdoc} + */ + public function getUrl(): string + { + return $this->delegate->getUri()->__toString(); + } + + /** + * {@inheritdoc} + */ + public function getHeader(string $name): string + { + return $this->delegate->getHeaderLine($name); + } + + public function getRoute(): ?string + { + return null; + } + + /** + * {@inheritdoc} + * + * @return RequestInterface + */ + public function unwrap() + { + return $this->delegate; + } +} diff --git a/src/Zipkin/Instrumentation/Http/Server/Psr15/Response.php b/src/Zipkin/Instrumentation/Http/Server/Psr15/Response.php new file mode 100644 index 00000000..ad031890 --- /dev/null +++ b/src/Zipkin/Instrumentation/Http/Server/Psr15/Response.php @@ -0,0 +1,64 @@ +delegate = $delegate; + $this->request = $request; + } + + /** + * {@inheritdoc} + */ + public function getRequest(): ?Request + { + return $this->request; + } + + /** + * {@inheritdoc} + */ + public function getStatusCode(): int + { + return $this->delegate->getStatusCode(); + } + + /** + * {@inheritdoc} + */ + public function getRoute(): ?string + { + return null; + } + + /** + * {@inheritdoc} + * + * @return ResponseInterface + */ + public function unwrap() + { + return $this->delegate; + } +} diff --git a/src/Zipkin/Instrumentation/Http/Server/Request.php b/src/Zipkin/Instrumentation/Http/Server/Request.php new file mode 100644 index 00000000..59f11823 --- /dev/null +++ b/src/Zipkin/Instrumentation/Http/Server/Request.php @@ -0,0 +1,25 @@ +The route is associated with the request, but it may not be visible until response + * processing. The reasons is that many server implementations process the request before they can + * identify the route. Parsing should expect this and look at {@link HttpResponse#route()} as + * needed. + * + * @see Tags#HTTP_ROUTE + * @since 5.10 + */ + public function getRoute(): ?string; +} diff --git a/src/Zipkin/Instrumentation/Http/Server/Response.php b/src/Zipkin/Instrumentation/Http/Server/Response.php new file mode 100644 index 00000000..915cdb4b --- /dev/null +++ b/src/Zipkin/Instrumentation/Http/Server/Response.php @@ -0,0 +1,25 @@ +The route is associated with the request, but it may not be visible until response + * processing. The reasons is that many server implementations process the request before they can + * identify the route. Parsing should expect this and look at {@link HttpResponse#route()} as + * needed. + * + * @see Tags#HTTP_ROUTE + * @since 5.10 + */ + public function getRoute(): ?string; +} diff --git a/tests/Integration/Instrumentation/Http/Server/MiddlewareTest.php b/tests/Integration/Instrumentation/Http/Server/MiddlewareTest.php index d3c4d6f4..05366cfa 100644 --- a/tests/Integration/Instrumentation/Http/Server/MiddlewareTest.php +++ b/tests/Integration/Instrumentation/Http/Server/MiddlewareTest.php @@ -11,12 +11,12 @@ use Zipkin\Reporters\InMemory; use Zipkin\Propagation\TraceContext; +use Zipkin\Instrumentation\Http\Server\Request; use Zipkin\Instrumentation\Http\Server\Psr15\Middleware; -use Zipkin\Instrumentation\Http\Server\Psr15\DefaultParser; use Zipkin\Instrumentation\Http\Server\Parser; use Zipkin\Instrumentation\Http\Server\HttpServerTracing; +use Zipkin\Instrumentation\Http\Server\DefaultParser; use RingCentral\Psr7\Response; -use Psr\Http\Message\ServerRequestInterface; use PHPUnit\Framework\TestCase; use Middlewares\Utils\Factory; use Middlewares\Utils\Dispatcher; @@ -47,12 +47,11 @@ static function () use ($tracer, $reporter): array { public function testMiddlewareRecordsRequestSuccessfully() { $parser = new class() extends DefaultParser { - public function request($request, TraceContext $context, SpanCustomizer $span): void + public function request(Request $request, TraceContext $context, SpanCustomizer $span): void { - assert($request instanceof ServerRequestInterface); // This parser retrieves the user_id from the request and add // is a tag. - $userId = $request->getAttribute('user_id'); + $userId = $request->unwrap()->getAttribute('user_id'); $span->tag('user_id', $userId); parent::request($request, $context, $span); } diff --git a/tests/Unit/Instrumentation/Http/Client/Psr18/ClientTest.php b/tests/Unit/Instrumentation/Http/Client/Psr18/ClientTest.php index 8f44ea0f..a653d0b1 100644 --- a/tests/Unit/Instrumentation/Http/Client/Psr18/ClientTest.php +++ b/tests/Unit/Instrumentation/Http/Client/Psr18/ClientTest.php @@ -7,9 +7,9 @@ use Zipkin\TracingBuilder; use Zipkin\Samplers\BinarySampler; use Zipkin\Reporters\InMemory; -use Zipkin\Instrumentation\Http\Client\Psr18\DefaultParser; use Zipkin\Instrumentation\Http\Client\Psr18\Client; use Zipkin\Instrumentation\Http\Client\HttpClientTracing; +use Zipkin\Instrumentation\Http\Client\DefaultParser; use RuntimeException; use Psr\Http\Message\ResponseInterface; use Psr\Http\Message\RequestInterface; diff --git a/tests/Unit/Instrumentation/Http/Server/Psr15/MiddlewareTest.php b/tests/Unit/Instrumentation/Http/Server/Psr15/MiddlewareTest.php index 86af8813..5a98cd39 100644 --- a/tests/Unit/Instrumentation/Http/Server/Psr15/MiddlewareTest.php +++ b/tests/Unit/Instrumentation/Http/Server/Psr15/MiddlewareTest.php @@ -11,8 +11,8 @@ use Zipkin\Propagation\TraceContext; use Zipkin\Propagation\DefaultSamplingFlags; use Zipkin\Instrumentation\Http\Server\Psr15\Middleware; -use Zipkin\Instrumentation\Http\Server\Psr15\DefaultParser; use Zipkin\Instrumentation\Http\Server\HttpServerTracing; +use Zipkin\Instrumentation\Http\Server\DefaultParser; use RingCentral\Psr7\Response as Psr7Response; use Psr\Http\Server\RequestHandlerInterface; use Psr\Http\Message\ServerRequestInterface; From 71275e29eaeb2ebd5171b5cb891992b57cc14d98 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Carlos=20Ch=C3=A1vez?= Date: Mon, 20 Jul 2020 22:39:21 +0200 Subject: [PATCH 13/20] docs(http-server): improves docs. --- src/Zipkin/Instrumentation/Http/Response.php | 2 -- src/Zipkin/Instrumentation/Http/Server/Request.php | 3 +-- src/Zipkin/Instrumentation/Http/Server/Response.php | 3 +-- 3 files changed, 2 insertions(+), 6 deletions(-) diff --git a/src/Zipkin/Instrumentation/Http/Response.php b/src/Zipkin/Instrumentation/Http/Response.php index f924d127..fcc19caa 100644 --- a/src/Zipkin/Instrumentation/Http/Response.php +++ b/src/Zipkin/Instrumentation/Http/Response.php @@ -15,8 +15,6 @@ public function getRequest(): ?Request; * The HTTP status code or zero if unreadable. * *

Conventionally associated with the key "http.status_code" - * - * @since 5.10 */ public function getStatusCode(): int; diff --git a/src/Zipkin/Instrumentation/Http/Server/Request.php b/src/Zipkin/Instrumentation/Http/Server/Request.php index 59f11823..59dbbf5e 100644 --- a/src/Zipkin/Instrumentation/Http/Server/Request.php +++ b/src/Zipkin/Instrumentation/Http/Server/Request.php @@ -18,8 +18,7 @@ interface Request extends HttpRequest * identify the route. Parsing should expect this and look at {@link HttpResponse#route()} as * needed. * - * @see Tags#HTTP_ROUTE - * @since 5.10 + * @see Tags\HTTP_ROUTE */ public function getRoute(): ?string; } diff --git a/src/Zipkin/Instrumentation/Http/Server/Response.php b/src/Zipkin/Instrumentation/Http/Server/Response.php index 915cdb4b..6fbfe67b 100644 --- a/src/Zipkin/Instrumentation/Http/Server/Response.php +++ b/src/Zipkin/Instrumentation/Http/Server/Response.php @@ -18,8 +18,7 @@ interface Response extends HttpResponse * identify the route. Parsing should expect this and look at {@link HttpResponse#route()} as * needed. * - * @see Tags#HTTP_ROUTE - * @since 5.10 + * @see Tags\HTTP_ROUTE */ public function getRoute(): ?string; } From a38152ecf3ccd6e882056d5a79c7c1c8b5a383ec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Carlos=20Ch=C3=A1vez?= Date: Tue, 21 Jul 2020 10:35:32 +0200 Subject: [PATCH 14/20] chore(http-server): makes Request/Response interfaces abstract classes to make sure the introduction of new methods won't break implementations. --- .../Instrumentation/Http/Client/Psr18/Request.php | 6 +++--- .../Instrumentation/Http/Client/Psr18/Response.php | 2 +- src/Zipkin/Instrumentation/Http/Client/Request.php | 2 +- src/Zipkin/Instrumentation/Http/Client/Response.php | 2 +- src/Zipkin/Instrumentation/Http/Request.php | 12 ++++++------ src/Zipkin/Instrumentation/Http/Response.php | 8 ++++---- .../Instrumentation/Http/Server/Psr15/Request.php | 6 +++--- .../Instrumentation/Http/Server/Psr15/Response.php | 2 +- src/Zipkin/Instrumentation/Http/Server/Request.php | 4 ++-- src/Zipkin/Instrumentation/Http/Server/Response.php | 4 ++-- 10 files changed, 24 insertions(+), 24 deletions(-) diff --git a/src/Zipkin/Instrumentation/Http/Client/Psr18/Request.php b/src/Zipkin/Instrumentation/Http/Client/Psr18/Request.php index 4b8049c7..b8912cd8 100644 --- a/src/Zipkin/Instrumentation/Http/Client/Psr18/Request.php +++ b/src/Zipkin/Instrumentation/Http/Client/Psr18/Request.php @@ -7,7 +7,7 @@ use Zipkin\Instrumentation\Http\Client\Request as ClientRequest; use Psr\Http\Message\RequestInterface; -final class Request implements ClientRequest +final class Request extends ClientRequest { /** * @var RequestInterface @@ -46,9 +46,9 @@ public function getUrl(): string /** * {@inheritdoc} */ - public function getHeader(string $name): string + public function getHeader(string $name): ?string { - return $this->delegate->getHeaderLine($name); + return $this->delegate->getHeaderLine($name) ?: null; } /** diff --git a/src/Zipkin/Instrumentation/Http/Client/Psr18/Response.php b/src/Zipkin/Instrumentation/Http/Client/Psr18/Response.php index 2802a81c..5c4e402c 100644 --- a/src/Zipkin/Instrumentation/Http/Client/Psr18/Response.php +++ b/src/Zipkin/Instrumentation/Http/Client/Psr18/Response.php @@ -8,7 +8,7 @@ use Zipkin\Instrumentation\Http\Client\Response as ClientResponse; use Psr\Http\Message\ResponseInterface; -final class Response implements ClientResponse +final class Response extends ClientResponse { /** * @var ResponseInterface diff --git a/src/Zipkin/Instrumentation/Http/Client/Request.php b/src/Zipkin/Instrumentation/Http/Client/Request.php index e2c821e2..5a47f815 100644 --- a/src/Zipkin/Instrumentation/Http/Client/Request.php +++ b/src/Zipkin/Instrumentation/Http/Client/Request.php @@ -6,6 +6,6 @@ use Zipkin\Instrumentation\Http\Request as HttpRequest; -interface Request extends HttpRequest +abstract class Request extends HttpRequest { } diff --git a/src/Zipkin/Instrumentation/Http/Client/Response.php b/src/Zipkin/Instrumentation/Http/Client/Response.php index 695bbf70..201adbcf 100644 --- a/src/Zipkin/Instrumentation/Http/Client/Response.php +++ b/src/Zipkin/Instrumentation/Http/Client/Response.php @@ -6,6 +6,6 @@ use Zipkin\Instrumentation\Http\Response as HttpResponse; -interface Response extends HttpResponse +abstract class Response extends HttpResponse { } diff --git a/src/Zipkin/Instrumentation/Http/Request.php b/src/Zipkin/Instrumentation/Http/Request.php index 759bb740..3f973908 100644 --- a/src/Zipkin/Instrumentation/Http/Request.php +++ b/src/Zipkin/Instrumentation/Http/Request.php @@ -7,7 +7,7 @@ /** * Abstract request type used for parsing and sampling of http clients and servers. */ -interface Request +abstract class Request { /** * The HTTP method, or verb, such as "GET" or "POST". @@ -21,7 +21,7 @@ interface Request * * @see Zipkin\Tags\HTTP_METHOD */ - public function getMethod(): string; + abstract public function getMethod(): string; /** * The absolute http path, without any query parameters. Ex. "/objects/abcd-ff" @@ -38,7 +38,7 @@ public function getMethod(): string; * * @see Tags\HTTP_PATH */ - public function getPath(): ?string; + abstract public function getPath(): ?string; /** * The entire URL, including the scheme, host and query parameters if available. @@ -47,15 +47,15 @@ public function getPath(): ?string; * * @see Tags#HTTP_URL */ - public function getUrl(): string; + abstract public function getUrl(): string; /** * Returns one value corresponding to the specified header, or null. */ - public function getHeader(string $name): string; + abstract public function getHeader(string $name): ?string; /** * @return mixed the underlying request object or {@code null} if there is none. */ - public function unwrap(); + abstract public function unwrap(); } diff --git a/src/Zipkin/Instrumentation/Http/Response.php b/src/Zipkin/Instrumentation/Http/Response.php index fcc19caa..154ab395 100644 --- a/src/Zipkin/Instrumentation/Http/Response.php +++ b/src/Zipkin/Instrumentation/Http/Response.php @@ -4,22 +4,22 @@ namespace Zipkin\Instrumentation\Http; -interface Response +abstract class Response { /** * The request that initiated this response or {@code null} if unknown. */ - public function getRequest(): ?Request; + abstract public function getRequest(): ?Request; /** * The HTTP status code or zero if unreadable. * *

Conventionally associated with the key "http.status_code" */ - public function getStatusCode(): int; + abstract public function getStatusCode(): int; /** * @return mixed the underlying response object or {@code null} if there is none. */ - public function unwrap(); + abstract public function unwrap(); } diff --git a/src/Zipkin/Instrumentation/Http/Server/Psr15/Request.php b/src/Zipkin/Instrumentation/Http/Server/Psr15/Request.php index 51d7439e..89ff5eb1 100644 --- a/src/Zipkin/Instrumentation/Http/Server/Psr15/Request.php +++ b/src/Zipkin/Instrumentation/Http/Server/Psr15/Request.php @@ -7,7 +7,7 @@ use Zipkin\Instrumentation\Http\Server\Request as ServerRequest; use Psr\Http\Message\RequestInterface; -final class Request implements ServerRequest +final class Request extends ServerRequest { /** * @var RequestInterface @@ -46,9 +46,9 @@ public function getUrl(): string /** * {@inheritdoc} */ - public function getHeader(string $name): string + public function getHeader(string $name): ?string { - return $this->delegate->getHeaderLine($name); + return $this->delegate->getHeaderLine($name) ?: null; } public function getRoute(): ?string diff --git a/src/Zipkin/Instrumentation/Http/Server/Psr15/Response.php b/src/Zipkin/Instrumentation/Http/Server/Psr15/Response.php index ad031890..1f84b1a6 100644 --- a/src/Zipkin/Instrumentation/Http/Server/Psr15/Response.php +++ b/src/Zipkin/Instrumentation/Http/Server/Psr15/Response.php @@ -8,7 +8,7 @@ use Zipkin\Instrumentation\Http\Request; use Psr\Http\Message\ResponseInterface; -final class Response implements ServerResponse +final class Response extends ServerResponse { /** * @var ResponseInterface diff --git a/src/Zipkin/Instrumentation/Http/Server/Request.php b/src/Zipkin/Instrumentation/Http/Server/Request.php index 59dbbf5e..0f175238 100644 --- a/src/Zipkin/Instrumentation/Http/Server/Request.php +++ b/src/Zipkin/Instrumentation/Http/Server/Request.php @@ -6,7 +6,7 @@ use Zipkin\Instrumentation\Http\Request as HttpRequest; -interface Request extends HttpRequest +abstract class Request extends HttpRequest { /** * Returns an expression such as "/items/:itemId" representing an application endpoint, @@ -20,5 +20,5 @@ interface Request extends HttpRequest * * @see Tags\HTTP_ROUTE */ - public function getRoute(): ?string; + abstract public function getRoute(): ?string; } diff --git a/src/Zipkin/Instrumentation/Http/Server/Response.php b/src/Zipkin/Instrumentation/Http/Server/Response.php index 6fbfe67b..be0bf83c 100644 --- a/src/Zipkin/Instrumentation/Http/Server/Response.php +++ b/src/Zipkin/Instrumentation/Http/Server/Response.php @@ -6,7 +6,7 @@ use Zipkin\Instrumentation\Http\Response as HttpResponse; -interface Response extends HttpResponse +abstract class Response extends HttpResponse { /** * Returns an expression such as "/items/:itemId" representing an application endpoint, @@ -20,5 +20,5 @@ interface Response extends HttpResponse * * @see Tags\HTTP_ROUTE */ - public function getRoute(): ?string; + abstract public function getRoute(): ?string; } From d51b5110b1fd77e0997b7fbcb2c0e1b2bebaedbc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Carlos=20Ch=C3=A1vez?= Date: Tue, 21 Jul 2020 10:47:17 +0200 Subject: [PATCH 15/20] chore(http-server): removes Parser::spanName to reduce the API surface. --- .../Instrumentation/Http/Client/DefaultParser.php | 3 ++- src/Zipkin/Instrumentation/Http/Client/Parser.php | 6 ------ .../Instrumentation/Http/Client/Psr18/Client.php | 1 - .../Instrumentation/Http/Server/DefaultParser.php | 14 ++++++++------ src/Zipkin/Instrumentation/Http/Server/Parser.php | 7 ------- .../Http/Server/Psr15/Middleware.php | 1 - 6 files changed, 10 insertions(+), 22 deletions(-) diff --git a/src/Zipkin/Instrumentation/Http/Client/DefaultParser.php b/src/Zipkin/Instrumentation/Http/Client/DefaultParser.php index 44f774fd..4bc695d3 100644 --- a/src/Zipkin/Instrumentation/Http/Client/DefaultParser.php +++ b/src/Zipkin/Instrumentation/Http/Client/DefaultParser.php @@ -18,13 +18,14 @@ */ class DefaultParser implements Parser { - public function spanName(Request $request): string + protected function spanName(Request $request): string { return $request->getMethod(); } public function request(Request $request, TraceContext $context, SpanCustomizer $span): void { + $span->setName($this->spanName($request)); $span->tag(Tags\HTTP_METHOD, $request->getMethod()); $span->tag(Tags\HTTP_PATH, $request->getPath() ?: "/"); } diff --git a/src/Zipkin/Instrumentation/Http/Client/Parser.php b/src/Zipkin/Instrumentation/Http/Client/Parser.php index ec29c5c0..25a43b1f 100644 --- a/src/Zipkin/Instrumentation/Http/Client/Parser.php +++ b/src/Zipkin/Instrumentation/Http/Client/Parser.php @@ -15,12 +15,6 @@ */ interface Parser { - /** - * spanName returns an appropiate span name based on the request, - * usually the HTTP method is good enough (e.g GET or POST). - */ - public function spanName(Request $request): string; - /** * request parses the incoming data related to a request in order to add * relevant information to the span under the SpanCustomizer interface. diff --git a/src/Zipkin/Instrumentation/Http/Client/Psr18/Client.php b/src/Zipkin/Instrumentation/Http/Client/Psr18/Client.php index 0010aced..efcffafe 100644 --- a/src/Zipkin/Instrumentation/Http/Client/Psr18/Client.php +++ b/src/Zipkin/Instrumentation/Http/Client/Psr18/Client.php @@ -78,7 +78,6 @@ public function sendRequest(RequestInterface $request): ResponseInterface // If span is NOOP it does not make sense to add customizations // to it like tags or annotations. $spanCustomizer = new SpanCustomizerShield($span); - $span->setName($this->parser->spanName($parsedRequest)); $this->parser->request($parsedRequest, $span->getContext(), $spanCustomizer); try { diff --git a/src/Zipkin/Instrumentation/Http/Server/DefaultParser.php b/src/Zipkin/Instrumentation/Http/Server/DefaultParser.php index 8b1dcc53..72b39d2a 100644 --- a/src/Zipkin/Instrumentation/Http/Server/DefaultParser.php +++ b/src/Zipkin/Instrumentation/Http/Server/DefaultParser.php @@ -16,13 +16,19 @@ */ class DefaultParser implements Parser { - public function spanName(Request $request): string + /** + * spanName returns an appropiate span name based on the request, + * usually the HTTP method is enough (e.g GET or POST) but ideally + * the http.route is desired (e.g. /user/{user_id}). + */ + protected function spanName(Request $request): string { - return $request->getMethod() . ($request->getRoute() === null ? '' : ' ' . $request->getRoute()); + return $request->getMethod(); } public function request(Request $request, TraceContext $context, SpanCustomizer $span): void { + $span->setName($this->spanName($request)); $span->tag(Tags\HTTP_METHOD, $request->getMethod()); $span->tag(Tags\HTTP_PATH, $request->getPath() ?: '/'); } @@ -33,9 +39,5 @@ public function response(Response $response, TraceContext $context, SpanCustomiz if ($response->getStatusCode() > 399) { $span->tag(Tags\ERROR, (string) $response->getStatusCode()); } - - if ($response->getRoute() !== null && $response->getRequest() !== null) { - $span->setName($response->getRequest()->getMethod() . ' ' . $response->getRoute()); - } } } diff --git a/src/Zipkin/Instrumentation/Http/Server/Parser.php b/src/Zipkin/Instrumentation/Http/Server/Parser.php index ddfac330..bf048807 100644 --- a/src/Zipkin/Instrumentation/Http/Server/Parser.php +++ b/src/Zipkin/Instrumentation/Http/Server/Parser.php @@ -13,13 +13,6 @@ */ interface Parser { - /** - * spanName returns an appropiate span name based on the request, - * usually the HTTP method is enough (e.g GET or POST) but ideally - * the http.route is desired (e.g. /user/{user_id}). - */ - public function spanName(Request $request): string; - /** * request parses the incoming data related to a request in order to add * relevant information to the span under the SpanCustomizer interface. diff --git a/src/Zipkin/Instrumentation/Http/Server/Psr15/Middleware.php b/src/Zipkin/Instrumentation/Http/Server/Psr15/Middleware.php index 65669ba8..a4d4bf72 100644 --- a/src/Zipkin/Instrumentation/Http/Server/Psr15/Middleware.php +++ b/src/Zipkin/Instrumentation/Http/Server/Psr15/Middleware.php @@ -71,7 +71,6 @@ public function process(ServerRequestInterface $request, RequestHandlerInterface $span->setKind(Kind\SERVER); $spanCustomizer = new SpanCustomizerShield($span); - $span->setName($this->parser->spanName($parsedRequest)); $this->parser->request($parsedRequest, $span->getContext(), $spanCustomizer); try { From bc571dcfbd45af34283ff935f743ef031d9a30e1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Carlos=20Ch=C3=A1vez?= Date: Tue, 21 Jul 2020 14:11:56 +0200 Subject: [PATCH 16/20] chore(http-server): do not tag success status code. --- .../Instrumentation/Http/Client/DefaultParser.php | 5 ++++- .../Instrumentation/Http/Server/DefaultParser.php | 10 +++++++--- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/src/Zipkin/Instrumentation/Http/Client/DefaultParser.php b/src/Zipkin/Instrumentation/Http/Client/DefaultParser.php index 4bc695d3..0f2e4ed2 100644 --- a/src/Zipkin/Instrumentation/Http/Client/DefaultParser.php +++ b/src/Zipkin/Instrumentation/Http/Client/DefaultParser.php @@ -33,7 +33,10 @@ public function request(Request $request, TraceContext $context, SpanCustomizer public function response(Response $response, TraceContext $context, SpanCustomizer $span): void { $statusCode = $response->getStatusCode(); - $span->tag(Tags\HTTP_STATUS_CODE, (string) $statusCode); + if ($statusCode < 200 || $statusCode > 299) { + $span->tag(Tags\HTTP_STATUS_CODE, (string) $statusCode); + } + if ($statusCode > 399) { $span->tag(Tags\ERROR, (string) $statusCode); } diff --git a/src/Zipkin/Instrumentation/Http/Server/DefaultParser.php b/src/Zipkin/Instrumentation/Http/Server/DefaultParser.php index 72b39d2a..18a1b9b9 100644 --- a/src/Zipkin/Instrumentation/Http/Server/DefaultParser.php +++ b/src/Zipkin/Instrumentation/Http/Server/DefaultParser.php @@ -35,9 +35,13 @@ public function request(Request $request, TraceContext $context, SpanCustomizer public function response(Response $response, TraceContext $context, SpanCustomizer $span): void { - $span->tag(Tags\HTTP_STATUS_CODE, (string) $response->getStatusCode()); - if ($response->getStatusCode() > 399) { - $span->tag(Tags\ERROR, (string) $response->getStatusCode()); + $statusCode = $response->getStatusCode(); + if ($statusCode < 200 || $statusCode > 299) { + $span->tag(Tags\HTTP_STATUS_CODE, (string) $statusCode); + } + + if ($statusCode > 399) { + $span->tag(Tags\ERROR, (string) $statusCode); } } } From 2b0c3a2773cbbc14adc59d0024f6fad906367040 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Carlos=20Ch=C3=A1vez?= Date: Tue, 21 Jul 2020 14:12:34 +0200 Subject: [PATCH 17/20] chore(http-server): removed the need to pass a parser when intended to use the default one. --- src/Zipkin/Instrumentation/Http/Client/Psr18/README.md | 3 +-- src/Zipkin/Instrumentation/Http/Server/HttpServerTracing.php | 4 ++-- src/Zipkin/Instrumentation/Http/Server/Psr15/README.md | 3 +-- 3 files changed, 4 insertions(+), 6 deletions(-) diff --git a/src/Zipkin/Instrumentation/Http/Client/Psr18/README.md b/src/Zipkin/Instrumentation/Http/Client/Psr18/README.md index 8b0dc4d3..89c080c6 100644 --- a/src/Zipkin/Instrumentation/Http/Client/Psr18/README.md +++ b/src/Zipkin/Instrumentation/Http/Client/Psr18/README.md @@ -19,13 +19,12 @@ use GuzzleHttp\Client; use GuzzleHttp\Psr7\Request; use Zipkin\Instrumentation\Http\Client\HttpClientTracing; use Zipkin\Instrumentation\Http\Client\Psr\Client as ZipkinClient; -use Zipkin\Instrumentation\Http\Client\Psr\DefaultParser; $tracing = TracingBuilder::create() ->havingLocalServiceName('my_service') ->build(); -$httpClientTracing = new HttpClientTracing($tracing, new DefaultParser); +$httpClientTracing = new HttpClientTracing($tracing); ... $httpClient = new ZipkinClient(new Client, $httpClientTracing); diff --git a/src/Zipkin/Instrumentation/Http/Server/HttpServerTracing.php b/src/Zipkin/Instrumentation/Http/Server/HttpServerTracing.php index f350dddd..f0bcd595 100644 --- a/src/Zipkin/Instrumentation/Http/Server/HttpServerTracing.php +++ b/src/Zipkin/Instrumentation/Http/Server/HttpServerTracing.php @@ -32,11 +32,11 @@ class HttpServerTracing public function __construct( Tracing $tracing, - Parser $parser, + Parser $parser = null, callable $requestSampler = null ) { $this->tracing = $tracing; - $this->parser = $parser; + $this->parser = $parser ?? new DefaultParser; $this->requestSampler = $requestSampler; } diff --git a/src/Zipkin/Instrumentation/Http/Server/Psr15/README.md b/src/Zipkin/Instrumentation/Http/Server/Psr15/README.md index 785fe0f6..9916302d 100644 --- a/src/Zipkin/Instrumentation/Http/Server/Psr15/README.md +++ b/src/Zipkin/Instrumentation/Http/Server/Psr15/README.md @@ -15,7 +15,6 @@ composer require psr/http-server-middleware In this example we use [fast-route](https://github.com/middlewares/fast-route) and [request-handler](https://github.com/middlewares/request-handler) middlewares but any HTTP server middleware supporting PSR15 middlewares will work. ```php -use Zipkin\Instrumentation\Http\Server\Psr15\DefaultParser; use Zipkin\Instrumentation\Http\Server\Psr15\Middleware as ZipkinMiddleware; use Zipkin\Instrumentation\Http\Server\HttpServerTracing; @@ -29,7 +28,7 @@ $tracing = TracingBuilder::create() ->havingLocalServiceName('my_service') ->build(); -$httpClientTracing = new ServerTracing($tracing, new DefaultParser); +$httpClientTracing = new HttpServerTracing($tracing); $dispatcher = new Dispatcher([ new Middlewares\FastRoute($fastRouteDispatcher), From 8b09a4486c12ea6eadf6fc3ae76c2b523813315f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Carlos=20Ch=C3=A1vez?= Date: Tue, 21 Jul 2020 18:32:32 +0200 Subject: [PATCH 18/20] chore(http-server): adds default implementation for request/response methods that are likely to have a default value. --- .../Instrumentation/Http/Client/DefaultParser.php | 10 ++++++++++ src/Zipkin/Instrumentation/Http/Response.php | 5 ++++- .../Instrumentation/Http/Server/DefaultParser.php | 13 ++++++++++++- .../Instrumentation/Http/Server/Psr15/Request.php | 5 ----- .../Instrumentation/Http/Server/Psr15/Response.php | 8 -------- src/Zipkin/Instrumentation/Http/Server/Request.php | 5 ++++- src/Zipkin/Instrumentation/Http/Server/Response.php | 5 ++++- .../Instrumentation/Http/Server/MiddlewareTest.php | 1 - .../Http/Client/Psr18/ClientTest.php | 1 - .../Http/Server/Psr15/MiddlewareTest.php | 1 - 10 files changed, 34 insertions(+), 20 deletions(-) diff --git a/src/Zipkin/Instrumentation/Http/Client/DefaultParser.php b/src/Zipkin/Instrumentation/Http/Client/DefaultParser.php index 0f2e4ed2..7c1dc4c1 100644 --- a/src/Zipkin/Instrumentation/Http/Client/DefaultParser.php +++ b/src/Zipkin/Instrumentation/Http/Client/DefaultParser.php @@ -18,11 +18,18 @@ */ class DefaultParser implements Parser { + /** + * spanName returns an appropiate span name based on the request, + * usually the HTTP method is enough (e.g GET or POST). + */ protected function spanName(Request $request): string { return $request->getMethod(); } + /** + * {@inhertidoc} + */ public function request(Request $request, TraceContext $context, SpanCustomizer $span): void { $span->setName($this->spanName($request)); @@ -30,6 +37,9 @@ public function request(Request $request, TraceContext $context, SpanCustomizer $span->tag(Tags\HTTP_PATH, $request->getPath() ?: "/"); } + /** + * {@inhertidoc} + */ public function response(Response $response, TraceContext $context, SpanCustomizer $span): void { $statusCode = $response->getStatusCode(); diff --git a/src/Zipkin/Instrumentation/Http/Response.php b/src/Zipkin/Instrumentation/Http/Response.php index 154ab395..1fd4e51a 100644 --- a/src/Zipkin/Instrumentation/Http/Response.php +++ b/src/Zipkin/Instrumentation/Http/Response.php @@ -9,7 +9,10 @@ abstract class Response /** * The request that initiated this response or {@code null} if unknown. */ - abstract public function getRequest(): ?Request; + public function getRequest(): ?Request + { + return null; + } /** * The HTTP status code or zero if unreadable. diff --git a/src/Zipkin/Instrumentation/Http/Server/DefaultParser.php b/src/Zipkin/Instrumentation/Http/Server/DefaultParser.php index 18a1b9b9..45f10c97 100644 --- a/src/Zipkin/Instrumentation/Http/Server/DefaultParser.php +++ b/src/Zipkin/Instrumentation/Http/Server/DefaultParser.php @@ -23,9 +23,13 @@ class DefaultParser implements Parser */ protected function spanName(Request $request): string { - return $request->getMethod(); + return $request->getMethod() + . ($request->getRoute() === null ? '' : ' ' . $request->getRoute()); } + /** + * {@inhertidoc} + */ public function request(Request $request, TraceContext $context, SpanCustomizer $span): void { $span->setName($this->spanName($request)); @@ -33,8 +37,15 @@ public function request(Request $request, TraceContext $context, SpanCustomizer $span->tag(Tags\HTTP_PATH, $request->getPath() ?: '/'); } + /** + * {@inhertidoc} + */ public function response(Response $response, TraceContext $context, SpanCustomizer $span): void { + if (null !== ($route = $response->getRoute())) { + $span->setName($response->getRequest()->getMethod() . ' ' . $route); + } + $statusCode = $response->getStatusCode(); if ($statusCode < 200 || $statusCode > 299) { $span->tag(Tags\HTTP_STATUS_CODE, (string) $statusCode); diff --git a/src/Zipkin/Instrumentation/Http/Server/Psr15/Request.php b/src/Zipkin/Instrumentation/Http/Server/Psr15/Request.php index 89ff5eb1..79cf618c 100644 --- a/src/Zipkin/Instrumentation/Http/Server/Psr15/Request.php +++ b/src/Zipkin/Instrumentation/Http/Server/Psr15/Request.php @@ -51,11 +51,6 @@ public function getHeader(string $name): ?string return $this->delegate->getHeaderLine($name) ?: null; } - public function getRoute(): ?string - { - return null; - } - /** * {@inheritdoc} * diff --git a/src/Zipkin/Instrumentation/Http/Server/Psr15/Response.php b/src/Zipkin/Instrumentation/Http/Server/Psr15/Response.php index 1f84b1a6..76ffba18 100644 --- a/src/Zipkin/Instrumentation/Http/Server/Psr15/Response.php +++ b/src/Zipkin/Instrumentation/Http/Server/Psr15/Response.php @@ -44,14 +44,6 @@ public function getStatusCode(): int return $this->delegate->getStatusCode(); } - /** - * {@inheritdoc} - */ - public function getRoute(): ?string - { - return null; - } - /** * {@inheritdoc} * diff --git a/src/Zipkin/Instrumentation/Http/Server/Request.php b/src/Zipkin/Instrumentation/Http/Server/Request.php index 0f175238..1d6f2dae 100644 --- a/src/Zipkin/Instrumentation/Http/Server/Request.php +++ b/src/Zipkin/Instrumentation/Http/Server/Request.php @@ -20,5 +20,8 @@ abstract class Request extends HttpRequest * * @see Tags\HTTP_ROUTE */ - abstract public function getRoute(): ?string; + public function getRoute(): ?string + { + return null; + } } diff --git a/src/Zipkin/Instrumentation/Http/Server/Response.php b/src/Zipkin/Instrumentation/Http/Server/Response.php index be0bf83c..2a07e74a 100644 --- a/src/Zipkin/Instrumentation/Http/Server/Response.php +++ b/src/Zipkin/Instrumentation/Http/Server/Response.php @@ -20,5 +20,8 @@ abstract class Response extends HttpResponse * * @see Tags\HTTP_ROUTE */ - abstract public function getRoute(): ?string; + public function getRoute(): ?string + { + return null; + } } diff --git a/tests/Integration/Instrumentation/Http/Server/MiddlewareTest.php b/tests/Integration/Instrumentation/Http/Server/MiddlewareTest.php index 05366cfa..a305f85d 100644 --- a/tests/Integration/Instrumentation/Http/Server/MiddlewareTest.php +++ b/tests/Integration/Instrumentation/Http/Server/MiddlewareTest.php @@ -85,7 +85,6 @@ public function request(Request $request, TraceContext $context, SpanCustomizer $this->assertEquals([ 'http.method' => 'GET', 'http.path' => '/users/abc123', - 'http.status_code' => '201', 'user_id' => 'abc123', ], $span['tags']); } diff --git a/tests/Unit/Instrumentation/Http/Client/Psr18/ClientTest.php b/tests/Unit/Instrumentation/Http/Client/Psr18/ClientTest.php index a653d0b1..6af9d4bc 100644 --- a/tests/Unit/Instrumentation/Http/Client/Psr18/ClientTest.php +++ b/tests/Unit/Instrumentation/Http/Client/Psr18/ClientTest.php @@ -78,7 +78,6 @@ public function getLastRequest(): ?RequestInterface $this->assertEquals([ 'http.method' => 'GET', 'http.path' => '/', - 'http.status_code' => '200', ], $span->getTags()); } diff --git a/tests/Unit/Instrumentation/Http/Server/Psr15/MiddlewareTest.php b/tests/Unit/Instrumentation/Http/Server/Psr15/MiddlewareTest.php index 5a98cd39..2bd8dbbe 100644 --- a/tests/Unit/Instrumentation/Http/Server/Psr15/MiddlewareTest.php +++ b/tests/Unit/Instrumentation/Http/Server/Psr15/MiddlewareTest.php @@ -89,7 +89,6 @@ public function testMiddlewareHandlesRequestSuccessfully() $this->assertEquals([ 'http.method' => 'GET', 'http.path' => '/', - 'http.status_code' => '200', ], $span['tags']); } From 31bfdedbc3c7a1a75a4bccc9510d51da598561d1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Carlos=20Ch=C3=A1vez?= Date: Fri, 24 Jul 2020 10:03:18 +0200 Subject: [PATCH 19/20] chore(http-server): removes getRoute as now we use abstract classes and hence method can be added later (when needed) without being a breaking change. --- .../Instrumentation/Http/Server/Response.php | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/src/Zipkin/Instrumentation/Http/Server/Response.php b/src/Zipkin/Instrumentation/Http/Server/Response.php index 2a07e74a..db9cbb83 100644 --- a/src/Zipkin/Instrumentation/Http/Server/Response.php +++ b/src/Zipkin/Instrumentation/Http/Server/Response.php @@ -8,20 +8,4 @@ abstract class Response extends HttpResponse { - /** - * Returns an expression such as "/items/:itemId" representing an application endpoint, - * conventionally associated with the tag key "http.route". If no route matched, "" (empty string) - * is returned. Null indicates this instrumentation doesn't understand http routes. - * - *

The route is associated with the request, but it may not be visible until response - * processing. The reasons is that many server implementations process the request before they can - * identify the route. Parsing should expect this and look at {@link HttpResponse#route()} as - * needed. - * - * @see Tags\HTTP_ROUTE - */ - public function getRoute(): ?string - { - return null; - } } From 0406c520f516f09682b947d0ba96a444e84163bb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Carlos=20Ch=C3=A1vez?= Date: Fri, 24 Jul 2020 10:04:36 +0200 Subject: [PATCH 20/20] docs(http-server): last tweaks to documentation. --- src/Zipkin/Instrumentation/Http/Request.php | 7 +++---- src/Zipkin/Instrumentation/Http/Server/DefaultParser.php | 4 ---- src/Zipkin/Instrumentation/Http/Server/Request.php | 2 +- 3 files changed, 4 insertions(+), 9 deletions(-) diff --git a/src/Zipkin/Instrumentation/Http/Request.php b/src/Zipkin/Instrumentation/Http/Request.php index 3f973908..4407e0fc 100644 --- a/src/Zipkin/Instrumentation/Http/Request.php +++ b/src/Zipkin/Instrumentation/Http/Request.php @@ -16,8 +16,7 @@ abstract class Request * *

Note

*

It is part of the HTTP RFC - * that an HTTP method is case-sensitive. Do not downcase results. If you do, not only will - * integration tests fail, but you will surprise any consumers who expect compliant results. + * that an HTTP method is case-sensitive. Do not downcase results. * * @see Zipkin\Tags\HTTP_METHOD */ @@ -36,7 +35,7 @@ abstract public function getMethod(): string; * normalize "" to "/". This ensures values are consistent with wire-level clients and behaviour * consistent with RFC 7230 Section 2.7.3. * - * @see Tags\HTTP_PATH + * @see Zipkin\Tags\HTTP_PATH */ abstract public function getPath(): ?string; @@ -45,7 +44,7 @@ abstract public function getPath(): ?string; * *

Conventionally associated with the key "http.url" * - * @see Tags#HTTP_URL + * @see Zipkin\Tags\HTTP_URL */ abstract public function getUrl(): string; diff --git a/src/Zipkin/Instrumentation/Http/Server/DefaultParser.php b/src/Zipkin/Instrumentation/Http/Server/DefaultParser.php index 45f10c97..e36505f5 100644 --- a/src/Zipkin/Instrumentation/Http/Server/DefaultParser.php +++ b/src/Zipkin/Instrumentation/Http/Server/DefaultParser.php @@ -42,10 +42,6 @@ public function request(Request $request, TraceContext $context, SpanCustomizer */ public function response(Response $response, TraceContext $context, SpanCustomizer $span): void { - if (null !== ($route = $response->getRoute())) { - $span->setName($response->getRequest()->getMethod() . ' ' . $route); - } - $statusCode = $response->getStatusCode(); if ($statusCode < 200 || $statusCode > 299) { $span->tag(Tags\HTTP_STATUS_CODE, (string) $statusCode); diff --git a/src/Zipkin/Instrumentation/Http/Server/Request.php b/src/Zipkin/Instrumentation/Http/Server/Request.php index 1d6f2dae..1f33c9e9 100644 --- a/src/Zipkin/Instrumentation/Http/Server/Request.php +++ b/src/Zipkin/Instrumentation/Http/Server/Request.php @@ -18,7 +18,7 @@ abstract class Request extends HttpRequest * identify the route. Parsing should expect this and look at {@link HttpResponse#route()} as * needed. * - * @see Tags\HTTP_ROUTE + * @see Zipkin\Tags\HTTP_ROUTE */ public function getRoute(): ?string {