chore: decouple client/server tracing from PSR implementation and consolidate nextSpanResolver. by jcchavezs · Pull Request #166 · openzipkin/zipkin-php

jcchavezs · 2020-07-12T21:30:28Z

This PR decouples the PSR implementation of HTTP tracing so the http abstraction can be used also with non PSR libraries.

It also consolidates the nextSpanResolver as a way to remove the responsability in the client or middleware to deal with the complexity of the next span:

In server side, this is the join span and the request sampler.
In client side, the ability to obtain tracing information from a request (like explicit context in zipkin-js) when doing event loop (and hence no global current context works) and the request sampler.

Once the design is finished I will add more test coverage.

Ping @adriancole

…r tracing.

jcchavezs · 2020-07-12T21:31:12Z

phpstan.neon

-			# Somehow this started failing
-			message: '#Constant .* already defined#'
-			path: src/Zipkin
+		-


Ignore this changes.

jcchavezs · 2020-07-12T21:33:06Z

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

-    public function getParser(): Parser
-    {
-        return $this->parser;
+    private static function buildNextSpanResolver(


This is important change.

jcchavezs · 2020-07-12T21:37:01Z

src/Zipkin/Propagation/B3.php

     */
    private const TRACE_ID_NAME = 'X-B3-TraceId';
-    
+


Ignore all changes in this class. It is just lint.

jcchavezs · 2020-07-12T21:37:54Z

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

+use Zipkin\Propagation\TraceContext;
+
+/**
+ * NoopParser is a noop implementation of a parser that parses nothing as it does not know anything about the request.


not sure I should provide this but at least it is convenient for tests.

if not sure keep it in tests directory :P

…tation and consolidates nextSpanResolver.

codefromthecrypt · 2020-07-12T23:33:27Z

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

+use Zipkin\Propagation\TraceContext;
+
+/**
+ * NoopParser is a noop implementation of a parser that parses nothing as it does not know anything about the request.


if not sure keep it in tests directory :P

codefromthecrypt · 2020-07-12T23:33:44Z

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

+{
+    public function spanName($request): string
+    {
+        return '';


is this because null isn't a thing?

codefromthecrypt · 2020-07-12T23:41:25Z

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

+     * @return callable(TraceContext, $request): Span the next span handler which creates an appropriate span based
+     * on the extracted context.
+     */
+    public function getNextSpanResolver(): callable


gut feel is there are too many variants of functions like this. I still don't fully understand why they have to be exposed separately as a function provider, as opposed to just using a handler.

I don't remember what these do, and it is odd that we have something typically called extractor which this is similar to, but maybe not the same as?

It smells like there are too many types of abstractions, like 1 per framework :P

before reviewing anything else, especially as the common answer is so often that X is for something not in this repo.. please make a model of things not in this repo that require the different types, as for example in java we don't require so many types of helper functions that have similar but different names. It is very confusing and not the best change model to drift here for something not here, and with no model tests to show why.

ex you can litersally make a test called PSR and even have that in the test dependency making s small version which shows why all these things are needed.

currently, the cognitive burden is very high due to so many things that are in many cases arbitrarily different than brave for me to review. When all things including nameing, class organization, lack of example code, etc are different it is very hard for me to help as each change is like a new baby and quest to figure out what its parents are.

here's an example.. in brave we dno't have OT in the same repo. To explain why some features exist (ex to give visibility so we can refactor them later etc, and if OT drifts we know what we initially designed to), we have a partial copy of the actual OT code https://github.com/openzipkin/brave/tree/master/brave/src/test/java/brave/features/opentracing

I've used this as how that's implemented has been revised and we'd easily break the hooks otherwise. For someone looking at change right now I have to look at different repos or ask you questions to try to understand why seems like 3 web frameworks need 3 variants of nextSpan :D I'd like to see these things here, the parts that require all these bits somehow and know this isn't a mono-repo. Another way is to locally put some of the parts that drive the design here literally marked as PSR etc.

That way it is easier to understand the design impact. Concretely, there are functions that are being slightly modified for this or that. sometimes composition is a better choice as it is less new words to invent and explain. Like "extract and call tracer" requires naming a word while just doing the 2 or 3 lines locally to a framework does not. It is easy to over-abstract in other words, and that sets up not just new vocab to learn that isn't anything like other vocab, but in certain perspectives it is completely unnecessary.

Ex even if the same 2 or 3 lines are combined in 3 projects (which is the minimal to do such a thing :P) it doesn't mean the composition of that 2 functions is worth naming as a higher order function. even if it might be.. the models of PSR etc here could prove the case that they actually add value enough to have to explain why X is not the same as Y, and neither is it like U T and Z.

So concretely, I'm thinking localize small integrations of all things you've done so far. whatever the several frameworks are since this is not a mono-repo.. into the test tree. Use these "dummy projects" to have a test case for the actual abstraction working.. this could make it easier to see where several new functions are worthwhile or not worthwhile.

Meanwhile, bear in mind if statement nesting inside a higher order function is a smell that it isn't quite clean as a separate function. Keep these to a minimal. Ex Tracer.joinSpan in brave, it exists in the tracer as disabling it lives there. only use twice and only on server side (HTTP and RPC), so just inlined them locally to the places that call it. I'd prefer to explain more the choices of what is done there if something is unfamiliar than have to explain a few more nextSpan variant functions. Ex it may not be obvious, but we also have to explicitly pass trace context in a request sometimes.. we can have this conversation and design impact easily if where these things are needed exist in the same codebase as the design is being developed and implemented.

hope this helps!

Thanks for the feedback @adriancole, actually PSR implementation is in this PR: https://github.com/openzipkin/zipkin-php/pull/166/files#diff-480132932af55e40a0941e973a46d2afR48-R85

public function process(ServerRequestInterface $request, RequestHandlerInterface $handler): ResponseInterface { $extractedContext = ($this->extractor)($request); $span = ($this->nextSpanResolver)($extractedContext, $request); $scopeCloser = $this->tracer->openScope($span); ...

Indeed nextSpanResolver was inspired by the nextSpanHandler as described in #162 (comment). The idea was to abstract the complexity for getting a new span in a server request/response situation. The way it is structured works for PSR (the standard for PHP) and any other server framework, that was the whole idea on separating the server instrumentation and PSR and keep PSR as one implementation because this logic of joining span or creating a new one from the current scope or use the requestSampler is being repeated in all the servers.

I still don't fully understand why they have to be exposed separately as a function provider, as opposed to just using a handler.

I don't fully understand this, probably because I don't have a clear idea on how a handler look like, is it that we would prefer a pluggable object that resolves or an object that resolves this without the need of being part of the server tracing?

when I say handler I usually mean https://github.com/openzipkin/brave/blob/master/instrumentation/http/src/main/java/brave/http/HttpServerHandler.java filtered on non-deprecated methods

for example, I've not found a case yet where you want to create a span and not parse data from its request or response. that's why the "nextSpan" type logic is a part of the handler.

jcchavezs · 2020-07-13T09:40:52Z

I am closing this as there are many concerns from @adriancole and I don't feel too comfortable making some decisions now that I would not be able to revert in the future when tackling the use cases.

jcchavezs added 7 commits July 6, 2020 17:09

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

9986886

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

dd5de99

docs(http-server): improves documentation.

02ccf75

chore(http-server): makes linter happy.

18b1e71

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

eb76e08

…r tracing.

chore: uses span error API.

4fc98ca

chore: creates nextSpanHandler in ServerTracing.

eb5bee4

jcchavezs commented Jul 12, 2020

View reviewed changes

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

bd75590

…tation and consolidates nextSpanResolver.

jcchavezs force-pushed the next_span_resolver branch from 450c0be to bd75590 Compare July 12, 2020 21:40

codefromthecrypt reviewed Jul 12, 2020

View reviewed changes

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

jcchavezs closed this Jul 13, 2020

jcchavezs deleted the next_span_resolver branch July 31, 2020 18:16

Conversation

jcchavezs commented Jul 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcchavezs commented Jul 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jcchavezs commented Jul 12, 2020 •

edited

Loading

jcchavezs commented Jul 13, 2020 •

edited

Loading