feat(http-server): adds middleware instrumentation. #162

jcchavezs · 2020-06-24T19:54:25Z

This PR adds support for instrumentation in PSR middlewares.

codefromthecrypt · 2020-06-25T00:18:11Z

src/Zipkin/Instrumentation/Http/Server/DefaultParser.php

+        }
+    }
+
+    public function error(Throwable $e, TraceContext $context, SpanCustomizer $span): void


fyi in brave we don't do this at the abstraction of http etc as often the throwable is generically parsable. Also in case it goes to another system, we don't set it at all (as it could be used in another field called exception for example)
ex we opt out like this:

if (error != null) return; // the call site used Span.error

https://github.com/openzipkin/brave/blob/master/instrumentation/http/src/main/java/brave/http/HttpResponseParser.java#L115

And defer to zipkin handler to use an error parser which internally does this:

static String parse(Throwable error) { if (error == null) throw new NullPointerException("error == null"); String message = error.getMessage(); if (message != null) return message; if (error.getClass().isAnonymousClass()) { // avoids "" return error.getClass().getSuperclass().getSimpleName(); } return error.getClass().getSimpleName(); }

In the zipkin variant of the error reporter we generically convent errors using that parser:

https://github.com/openzipkin/zipkin-reporter-java/blob/master/brave/src/main/java/zipkin2/reporter/brave/ConvertingSpanReporter.java#L89-L93

In other words we used to put $span->tag(Tags\ERROR, $e->getMessage()) in different code, but then had to later centralize it, and then later defer it as when you use other tools like x-ray the tag shape is different.

So if I understand correctly you don't treat error as something being part of an http tracing but instead you have an error parser that transform the error into something customized? I see that being feasible because Tags are objects in brave, in zipkin-php they are strings (I remember I wanted them to be stringable objects first but objects in PHP are not as cheap as in Java). To be honest, we are still in time to do such change (accept anything for tags and stringify them afterwards) because we are just introducing stronger typing and even more, if we don't do that change we won't be able to do it in the future but I am not entirely sure of the implications of such a change.

the jist of the change is you store the Throwable and only parse it into a String tag at the last moment (converting to json for example). So our field used is MutableSpan.error which is separate from MutableSpan.tags.

here's the notes about that field which I think says all there is to be said (at least that I know :)) https://github.com/openzipkin/brave/blob/master/brave/src/main/java/brave/handler/MutableSpan.java#L49-L86

I see. I did not know Span.error was a thing in brave. Let me dig into it.

Done: https://github.com/openzipkin/zipkin-php/pull/162/files#diff-7f86d716dfda08180da2ce29601ff41bR83

codefromthecrypt · 2020-06-25T00:27:27Z

src/Zipkin/Instrumentation/Http/Server/Middleware.php

+        } elseif ($this->requestSampler === null) {
+            $span = $this->tracer->nextSpan($extractedContext);
+        } else {
+            $span = $this->tracer->nextSpanWithSampler(


In brave, we do this logic differently as it keeps the tracer api small which is helpful as sometimes there are many "if statements" inside, and keeping all inside nextSpan(extracted) helps sanity a little.

Span nextSpan(TraceContextOrSamplingFlags extracted, HttpServerRequest request) { Boolean sampled = extracted.sampled(); // only recreate the context if the http sampler made a decision if (sampled == null && (sampled = sampler.trySample(request)) != null) { extracted = extracted.sampled(sampled.booleanValue()); } return extracted.context() != null ? tracer.joinSpan(extracted.context()) : tracer.nextSpan(extracted); }

Check eb5bee4 where I added the nextSpanHandler into the ServerTracing.

codefromthecrypt · 2020-06-25T00:28:24Z

src/Zipkin/Instrumentation/Http/Server/Middleware.php

+                $extractedContext
+            );
+        }
+        $spanCustomizer = null;


instead of all the null checking, why not check if noop and early return $handler->handle($request);?

Totally. Let me change it.

codefromthecrypt · 2020-06-25T00:32:13Z

src/Zipkin/Instrumentation/Http/Server/Parser.php

+ * Parser includes the methods to obtain meaningful span information
+ * out of HTTP request/response elements.
+ */
+interface Parser


design note:

we used to have a design like this, but we don't any more. I already mentioned about the error handling in an earlier comment.

instead we have lambda compatible (single method) functions for the request and response side.

HttpRequestParser -> void parse(HttpRequest request, TraceContext context, SpanCustomizer span)
HttpResponseParser -> void parse(HttpResponse response, TraceContext context, SpanCustomizer span)

At least in Java, this allows easier composition. Ex you can rely on the default for span name etc and add a tag like so

.clientRequestParser((req, context, span) -> { HttpClientRequestParser.DEFAULT.parse(req, context, span); HttpTags.URL.tag(req, context, span); // add the url in addition to defaults })

Most people need to change request not response tags anyway so it has little impact to split them, though the rationale may not be the same here.

I'd keep them together as IMHO it is easier to rationale about them together and also because I don't want to create too much classes around this.

src/Zipkin/Instrumentation/Http/Server/RequestHandler.php

codefromthecrypt

I made some notes and it appears to me that maybe due to the amount of deprecation we had in brave, it could have been less clear to follow vs rpc which is almost the same as our http handlers except it doesn't have all the deprecated things.

https://github.com/openzipkin/brave/blob/master/instrumentation/rpc

I made a bunch of comments I think maybe take a stab and think which are relevant here and let me know when you want another look.

jcchavezs · 2020-07-07T20:11:05Z

@adriancole PTAL

jcchavezs · 2020-07-07T20:15:46Z

@adriancole I addressed most of your comments. I turned the requestSampler into the nextSpanHandler in the serverTracing and I am pretty happy about it. If this ends up being the final design, I will replicate it in my symfony instrumentation.

codefromthecrypt · 2020-07-07T23:26:20Z

src/Zipkin/Instrumentation/Http/Server/README.md

+
+```php
+use Zipkin\Instrumentation\Http\Server\Middleware as ZipkinMiddleware;
+use Zipkin\Instrumentation\Http\Server\ServerTracing;


I see this is to avoid clash with normal Tracing. since RpcTracing may come next and that would have a client and server side.. and be configured possibly in the same file.. does it make more sense to use HttpTracing instead?

codefromthecrypt · 2020-07-07T23:31:01Z

src/Zipkin/Instrumentation/Http/Server/ServerTracing.php

+        return $this->parser;
+    }
+
+    public function getNextSpanHandler(): callable


if this is only used once maybe it can live at its call site? having it here requires you to say why and how it is used

jcchavezs · 2020-07-13T09:41:38Z

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


Changed the name @adriancole

jcchavezs · 2020-07-13T09:42:48Z

src/Zipkin/Instrumentation/Http/Server/Psr15/Middleware.php

+    {
+        $extractedContext = ($this->extractor)($request);
+
+        if ($extractedContext instanceof TraceContext) {


For now I am going to move this into a private method with a meaningful name.

jcchavezs · 2020-07-13T09:43:04Z

src/Zipkin/Instrumentation/Http/Server/Psr15/Middleware.php

+use Psr\Http\Message\ServerRequestInterface;
+use Psr\Http\Message\ResponseInterface;
+
+final class Middleware implements MiddlewareInterface


This is the PSR15 implementation using the server tracing.

jcchavezs · 2020-07-13T13:19:45Z

src/Zipkin/Instrumentation/Http/Client/HttpClientTracing.php

     *
     * @var callable(RequestInterface):?bool|null
     */
    private $requestSampler;

    public function __construct(
        Tracing $tracing,
-        Parser $parser = null,
+        Parser $parser,


Removing the null as it was intended for the HttpClientTracing to use a default one but since the default one requires a request type to be defined we require it as a parameter.

…r tracing.

…tation.

…eware.

… to the default parsers.

jcchavezs · 2020-07-15T21:10:50Z

@adriancole please take another look. I addressed most of your comments, I postponed the nextSpan thing and started this PR #169 for starting the laravel instrumentation using this API (additional to the PSR one).

codefromthecrypt · 2020-07-16T00:56:43Z

src/Zipkin/Instrumentation/Http/Server/Psr15/DefaultParser.php

+ * into span name and tags. Implementors can use this as a base parser to reduce
+ * boilerplate.
+ */
+class DefaultParser implements Parser


this makes me curious. in brave, the parser is intended to be defined in a way that is not library specific. This allows it to be generated from static configuration that doesn't know anything about the types that would be used. What I'm seeing here is that DefaultParser and the data policy is being independently defined in different libraries.

I would recommend instead to keep parser language agnostic. similar to brave, make the caller of HttpTracing localize the Request interface. ex. Zipkin\Instrumentation\Http\HttpRequest is the parameter of parser, and Zipkin\Instrumentation\Http\Server\Psr15\SomethingRequest which can be package private, implements it.

that different libraries treat data differently is normal, but if the "parser of http requests" requires knowing which library is in use, we lose the ability to change tagging policy independent of library, ex from yaml.

ex notice here that "how" an http header is parsed is not something the user should care about.. it is just an implementation detail.

https://github.com/openzipkin/brave/blob/master/instrumentation/okhttp3/src/main/java/brave/okhttp3/TracingInterceptor.java#L93-L129

doing so allows you to configure ITs at global level, such as I think we also do in zipkin-js etc
https://github.com/openzipkin/brave/blob/master/instrumentation/http-tests/src/main/java/brave/test/http/ITHttpClient.java#L234-L237

that allows configuration or global config to change the data policy. Ex if X-Ray needs url you add code for that without caring knowing or linking to library is PSR etc... to do that.. make sense?

jcchavezs · 2020-07-20T20:36:00Z

@adriancole I just added the Request/Response types and aligned the parsers. PTAL.

codefromthecrypt

shaping up.. getting almost there!

codefromthecrypt · 2020-07-21T00:50:39Z

src/Zipkin/Instrumentation/Http/Client/DefaultParser.php

 class DefaultParser implements Parser
 {
-    public function spanName(RequestInterface $request): string
+    public function spanName(Request $request): string


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

Makes sense. Actually I was about to change this as using the API felt weird.

codefromthecrypt · 2020-07-21T00:56:23Z

src/Zipkin/Instrumentation/Http/Client/DefaultParser.php

-        if ($response->getStatusCode() > 399) {
-            $span->tag(Tags\ERROR, (string) $response->getStatusCode());
+        $statusCode = $response->getStatusCode();
+        $span->tag(Tags\HTTP_STATUS_CODE, (string) $statusCode);


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
https://github.com/openzipkin/brave/blob/master/instrumentation/http/src/main/java/brave/http/HttpResponseParser.java#L88
https://github.com/openzipkin/brave/blob/master/instrumentation/http/src/main/java/brave/http/HttpTags.java#L155-L162

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.

codefromthecrypt · 2020-07-21T00:57:04Z

src/Zipkin/Instrumentation/Http/Client/DefaultParser.php

-            $span->tag(Tags\ERROR, (string) $response->getStatusCode());
+        $statusCode = $response->getStatusCode();
+        $span->tag(Tags\HTTP_STATUS_CODE, (string) $statusCode);
+        if ($statusCode > 399) {


need also to not accidentally tag 100 as error

https://github.com/openzipkin/brave/blob/master/instrumentation/http/src/main/java/brave/http/HttpResponseParser.java#L123-L128

codefromthecrypt · 2020-07-21T00:58:20Z

src/Zipkin/Instrumentation/Http/Client/Parser.php

@@ -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;


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

codefromthecrypt · 2020-07-21T00:59:22Z

src/Zipkin/Instrumentation/Http/Client/Psr18/README.md


 $tracing = TracingBuilder::create()
            ->havingLocalServiceName('my_service')
            ->build();

-$httpClientTracing = new ClientTracing($tracing);
+$httpClientTracing = new HttpClientTracing($tracing, new DefaultParser);


drift? I presume default impl works now, right (ex don't have Psr specific parser)

Yeah, totally. Also when using default parser we don't need to pass it. Honestly having to pass the DefaultParser per library was akward.

codefromthecrypt · 2020-07-21T01:02:49Z

src/Zipkin/Instrumentation/Http/Server/Parser.php

+     * 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;


similar comment, that the client and server side have two different types that do the same things, means there are some opportunity for copy/paste problems.

codefromthecrypt · 2020-07-21T01:03:46Z

src/Zipkin/Instrumentation/Http/Server/Psr15/README.md

+            ->havingLocalServiceName('my_service')
+            ->build();
+
+$httpClientTracing = new ServerTracing($tracing, new DefaultParser);


again user shouldn't have to instantiate a default parser anymore

codefromthecrypt · 2020-07-21T01:04:47Z

src/Zipkin/Instrumentation/Http/Server/Request.php

+     *
+     * @see Tags\HTTP_ROUTE
+     */
+    public function getRoute(): ?string;


route is also possible on some clients, not sure in PHP. It makes things easier to assume everything can see it and just handle if it is not visible vs 2 different parser hierarchies.

I don't think it is possible to pass router in PHP clients, I haven't seen any example. Since we know use abstract classes I think it is OK to not to add route yet and readd it later as it won't be a breaking change.

codefromthecrypt · 2020-07-21T01:06:09Z

src/Zipkin/Instrumentation/Http/Server/Response.php

+     *
+     * @see Tags\HTTP_ROUTE
+     */
+    public function getRoute(): ?string;


technically eventhough this is visible in the response, it is a part of request data. once we added response.request() we were able to keep this data in the more logical place https://github.com/openzipkin/brave/blob/master/instrumentation/http/src/main/java/brave/http/HttpRequest.java#L92-L106

My concern about that is that if route is not there yet at request parsing, having the ability to parse later means we mutated the request. In any case I will check first what happens in laravel or symfony before settling on this.

In the end I removed it as now we use abstract classes. I will readd it when the time comes.

codefromthecrypt · 2020-07-21T01:06:29Z

tests/Unit/Instrumentation/Http/Client/Psr18/ClientTest.php

@@ -30,7 +31,7 @@ private static function createTracing(): array
        $tracer = $tracing->getTracer();

        return [
-            new ClientTracing($tracing),
+            new HttpClientTracing($tracing, new DefaultParser),


same comment

…s to make sure the introduction of new methods won't break implementations.

…o use the default one.

…methods that are likely to have a default value.

…nd hence method can be added later (when needed) without being a breaking change.

jcchavezs · 2020-07-24T08:20:41Z

@adriancole I think I addressed all the feedback so please take another look and feel free to merge if you agree with all.

codefromthecrypt reviewed Jun 25, 2020

View reviewed changes

src/Zipkin/Instrumentation/Http/Server/RequestHandler.php Outdated Show resolved Hide resolved

codefromthecrypt reviewed Jun 25, 2020

View reviewed changes

jcchavezs force-pushed the http_server branch from 9177f63 to 4fc98ca Compare July 6, 2020 15:11

codefromthecrypt reviewed Jul 7, 2020

View reviewed changes

jcchavezs mentioned this pull request Jul 13, 2020

chore: decouple client/server tracing from PSR implementation and consolidate nextSpanResolver. #166

Closed

jcchavezs force-pushed the http_server branch from 4fc98ca to 9287d3a Compare July 13, 2020 09:39

jcchavezs commented Jul 13, 2020

View reviewed changes

jcchavezs force-pushed the http_server branch from 9287d3a to 2f08a7b Compare July 13, 2020 13:18

jcchavezs commented Jul 13, 2020

View reviewed changes

jcchavezs changed the title ~~feat(http-server): adds middleware and request handler instrumentation.~~ feat(http-server): adds middleware instrumentation. Jul 15, 2020

jcchavezs added 11 commits July 15, 2020 23:07

feat(http-server): adds middleware and request handler instrumentation.

1d18a24

tests(http-server): adds integration test for the Middleware.

bd11c76

docs(http-server): improves documentation.

14fcc62

chore(http-server): makes linter happy.

1f462b0

chore(http-server): removes RequestHandler as middleware is enough fo…

31faf67

…r tracing.

chore: uses span error API.

754340d

chore(http-server): decouples client/server tracing from PSR implemen…

9fa99f1

…tation.

tests(http-server): adds test for next span in the PSR15 server middl…

64cb9f9

…eware.

chore(http-server): small tweaks to tests.

d737d34

tests(http-server): improves test naming for client and middleware.

d15581c

chore: moves RequestHeaders into propagation folder, also adds typing…

b3866a9

… to the default parsers.

jcchavezs force-pushed the http_server branch from e7dd308 to b3866a9 Compare July 15, 2020 21:08

codefromthecrypt reviewed Jul 16, 2020

View reviewed changes

feat: adds request/response objects to HTTP instrumentation.

398d4cb

docs(http-server): improves docs.

71275e2

codefromthecrypt reviewed Jul 21, 2020

View reviewed changes

jcchavezs added 7 commits July 21, 2020 10:35

chore(http-server): makes Request/Response interfaces abstract classe…

a38152e

…s to make sure the introduction of new methods won't break implementations.

chore(http-server): removes Parser::spanName to reduce the API surface.

d51b511

chore(http-server): do not tag success status code.

bc571dc

chore(http-server): removed the need to pass a parser when intended t…

2b0c3a2

…o use the default one.

chore(http-server): adds default implementation for request/response …

8b09a44

…methods that are likely to have a default value.

chore(http-server): removes getRoute as now we use abstract classes a…

31bfded

…nd hence method can be added later (when needed) without being a breaking change.

docs(http-server): last tweaks to documentation.

0406c52

jcchavezs merged commit 3486acc into master Jul 29, 2020

jcchavezs deleted the http_server branch July 29, 2020 12:24

feat(http-server): adds middleware instrumentation. #162

feat(http-server): adds middleware instrumentation. #162

Conversation

jcchavezs commented Jun 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codefromthecrypt left a comment

Choose a reason for hiding this comment

jcchavezs commented Jul 7, 2020

jcchavezs commented Jul 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcchavezs commented Jul 15, 2020

codefromthecrypt Jul 16, 2020 • edited Loading

Choose a reason for hiding this comment

jcchavezs commented Jul 20, 2020

codefromthecrypt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcchavezs Jul 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcchavezs commented Jul 24, 2020

jcchavezs commented Jun 24, 2020 •

edited

Loading

codefromthecrypt Jul 16, 2020 •

edited

Loading

jcchavezs Jul 21, 2020 •

edited

Loading