-
Notifications
You must be signed in to change notification settings - Fork 59
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(http-server): adds middleware instrumentation. #162
Changes from 13 commits
1d18a24
bd11c76
14fcc62
1f462b0
31faf67
754340d
9fa99f1
64cb9f9
d737d34
d15581c
b3866a9
398d4cb
71275e2
a38152e
d51b511
bc571dc
2b0c3a2
8b09a44
31bfded
0406c52
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,28 +4,36 @@ | |
"description": "A Zipkin instrumentation for PHP", | ||
"keywords": [ | ||
"zipkin", | ||
"distributed-tracing", | ||
"tracing", | ||
"openzipkin" | ||
], | ||
"license": "MIT", | ||
"license": "Apache-2.0", | ||
"authors": [ | ||
{ | ||
"name": "José Carlos Chávez", | ||
"email": "[email protected]" | ||
} | ||
], | ||
"homepage": "https://github.com/openzipkin/zipkin-php", | ||
"support": { | ||
"issues": "https://github.com/openzipkin/zipkin-php/issues" | ||
}, | ||
"require": { | ||
"php": "^7.1", | ||
"ext-curl": "*", | ||
"psr/http-message": "~1.0", | ||
"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", | ||
"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-middleware": "Allows to instrument HTTP servers via middlewares following PSR15." | ||
}, | ||
"extra": { | ||
"branch-alias": { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,28 +7,34 @@ | |
use Zipkin\Tags; | ||
use Zipkin\SpanCustomizer; | ||
use Zipkin\Propagation\TraceContext; | ||
use Throwable; | ||
use Psr\Http\Message\ResponseInterface; | ||
use Psr\Http\Message\RequestInterface; | ||
use Zipkin\Instrumentation\Http\Response; | ||
use Zipkin\Instrumentation\Http\Request; | ||
use Zipkin\Instrumentation\Http\Client\Parser; | ||
|
||
/** | ||
* DefaultParser contains the basic logic for turning request/response information | ||
* into span name and tags. Implementors can use this as a base parser to reduce | ||
* boilerplate. | ||
*/ | ||
class DefaultParser implements Parser | ||
{ | ||
public function spanName(RequestInterface $request): string | ||
public function spanName(Request $request): string | ||
{ | ||
return $request->getMethod(); | ||
} | ||
|
||
public function request(RequestInterface $request, TraceContext $context, SpanCustomizer $span): void | ||
public function request(Request $request, TraceContext $context, SpanCustomizer $span): void | ||
{ | ||
$span->tag(Tags\HTTP_METHOD, $request->getMethod()); | ||
$span->tag(Tags\HTTP_PATH, $request->getUri()->getPath() ?: "/"); | ||
$span->tag(Tags\HTTP_PATH, $request->getPath() ?: "/"); | ||
} | ||
|
||
public function response(ResponseInterface $response, TraceContext $context, SpanCustomizer $span): void | ||
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(); | ||
$span->tag(Tags\HTTP_STATUS_CODE, (string) $statusCode); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. status code is a little more nuanced than this: maybe TODO if you don't want to address now, but we only tag if not in success range There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good reminder. I wanted to introduce this before but I wasn't sure this applied for both client and server, by that time I wouldn't dive into the http instrumentation design so many doubts and I did not want to introduce a change I did not understand. Anyways good time to introduce it. |
||
if ($statusCode > 399) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. need also to not accidentally tag 100 as error |
||
$span->tag(Tags\ERROR, (string) $statusCode); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,13 +5,13 @@ | |
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 | ||
* HTTP client. | ||
*/ | ||
class ClientTracing | ||
class HttpClientTracing | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changed the name @adriancole |
||
{ | ||
/** | ||
* @var Tracing | ||
|
@@ -25,13 +25,9 @@ class ClientTracing | |
|
||
/** | ||
* function that decides to sample or not an unsampled | ||
* request. The signature is: | ||
* request. | ||
* | ||
* <pre> | ||
* function (RequestInterface $request): ?bool {} | ||
* </pre> | ||
* | ||
* @var callable(RequestInterface):?bool|null | ||
* @var callable(Request):?bool|null | ||
*/ | ||
private $requestSampler; | ||
|
||
|
@@ -51,7 +47,7 @@ public function getTracing(): Tracing | |
} | ||
|
||
/** | ||
* @return (callable(RequestInterface):?bool)|null | ||
* @return (callable(Request):?bool)|null | ||
*/ | ||
public function getRequestSampler(): ?callable | ||
{ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,9 +6,8 @@ | |
|
||
use Zipkin\SpanCustomizer; | ||
use Zipkin\Propagation\TraceContext; | ||
use Throwable; | ||
use Psr\Http\Message\ResponseInterface; | ||
use Psr\Http\Message\RequestInterface; | ||
use Zipkin\Instrumentation\Http\Response; | ||
use Zipkin\Instrumentation\Http\Request; | ||
|
||
/** | ||
* Parser includes the methods to obtain meaningful span information | ||
|
@@ -20,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(RequestInterface $request): string; | ||
public function spanName(Request $request): string; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah see other comment about. as spanName can be set anytime, and is typically reset in the response based on http.route tag. might be better to not add this method to the base type |
||
|
||
/** | ||
* request parses the incoming data related to a request in order to add | ||
|
@@ -29,7 +28,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 $request, TraceContext $context, SpanCustomizer $span): void; | ||
|
||
|
||
/** | ||
|
@@ -40,5 +39,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 $response, TraceContext $context, SpanCustomizer $span): void; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. drift? I presume default impl works now, right (ex don't have Psr specific parser) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, totally. Also when using default parser we don't need to pass it. Honestly having to pass the |
||
... | ||
|
||
$httpClient = new ZipkinClient(new Client, $httpClientTracing); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,63 @@ | ||
<?php | ||
|
||
declare(strict_types=1); | ||
|
||
namespace Zipkin\Instrumentation\Http\Client\Psr18; | ||
|
||
use Zipkin\Instrumentation\Http\Client\Request as ClientRequest; | ||
use Psr\Http\Message\RequestInterface; | ||
|
||
final class Request implements ClientRequest | ||
{ | ||
/** | ||
* @var RequestInterface | ||
*/ | ||
private $delegate; | ||
|
||
public function __construct(RequestInterface $delegate) | ||
{ | ||
$this->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; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as span name can also be set (overridden) at response time, we don't expose a public function like this in brave. Just focus on request/response parsing vs suggesting where the span name is set. In the default impl we set the span name. Note: only in the default impl do we define the spanName method. it isn't in the generic interface anymore for this reason (that it doesn't belong to the request.. that name is just one example of something parsed) https://github.com/openzipkin/brave/blob/master/instrumentation/http/src/main/java/brave/http/HttpRequestParser.java#L79
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. Actually I was about to change this as using the API felt weird.