opentracing / opentracing-php

OpenTracing API for PHP
Apache License 2.0
505 stars 56 forks source link

PHP implementation #1

Closed yurishkuro closed 7 years ago

yurishkuro commented 7 years ago

Placeholder issue to list current work on PHP implementations.

beberlei commented 7 years ago

There is also https://github.com/tideways/opentracing-php

beberlei commented 7 years ago

The current state from my POV is:

From my POV @jcchavezs implementation is borrowing API from Go and Java, while mine (@tideways) is leaning more towards the python/ruby libraries, so which one we use is a question of style. I obviously prefer mine ;-)

Not sure how to proceed form here.

jcchavezs commented 7 years ago

I am not sure if I am borrowing (while yours is learning) the API from Golang and Java, at the very beginning the implementation was inspired in the Go one but after some feedback from @felixfbecker, you at some point and the python library I built a more idiomatic/easy-to-use library so you see, mine also learns.

I only want to talk about my library so I'd say that it is a conscientious representation of the opentracing specification, focusing on details (that is why it includes extra classes for VOs) in order to reflect what the specification says. I also want to mention that even in the first stage it worked pretty well for an DataDog implementation (which will be updated once we decide the library) so I am eager to see how it works with the new changes).

I prefer mine because I believe it follows the specification in a way I feel comfortable with and that implementors will feel their back covered in terms of details.

beberlei commented 7 years ago

@jcchavezs I should have quoted "borrowing", not sure what a better term would be, I don't specifically mean that the Go/Java perspective is bad.

I prefer smaller APIs over bigger one and that is one of the differences between our two (less classes and methods) and I mentioned before yours requires several external dependencies (symfony pollyfil, tracing-context) that are both concrete libraries and I fell they should not be used by code providing a generic specification interface.

yurishkuro commented 7 years ago

adding OTSC @bhs @pavolloffay @basvanbeek @cce

felixfbecker commented 7 years ago

Maybe lets list the goals of an official implementation so we can decide and weight the implementations. Here are traits I think a good implementation should have, where possible:

We don't have to decide to take one of the existing implementations as-is in its current form, maybe we can find a compromise that everyone is happy with (and thanks to everyone who put work into this so far). All implementations tick most of these boxes.

For example, I dislike @beberlei's static OpenTracing class that serves a lot of different purposes and the static Tags class that just holds constants. There are also a lot of docblocks missing which makes it unclear what type e.g. carriers have. The readme shows an example with basic curl and header arrays, but nothing for more advanced message representations like Symfony or PSR-7 Request objects. @jcchavezs's feels a lot more polished and structured in these regards. But at other areas like logs and tags, where types are truly dynamic, it feels like it is fighting PHP's dynamic nature instead of embracing it - @beberlei's implementation is simpler here, it feels more natural for PHP.

If I had to decide I would take @jcchavezs's implementation, but trim the API a bit.

jcchavezs commented 7 years ago

After some back and forth with @beberlei and based on @felixfbecker I am open to remove what concerns you in terms of complexity. I still would prefer Tags VOs but as I already mentioned by gitter chat, I am open to change them.

Regarding the dependencies, I will remove the tracing context (and change SpanContext to an interface) and the polifyll (since it is only being used for logs).

Once that is done I will ping you all to have a second look.

tedsuo commented 7 years ago

Looks like there is consensus to go with https://github.com/jcchavezs/opentracing-php. We'd like to transfer the repo in it's current state, and continue working on it here in tandem with the Java API. Not sure if we need to delete the placeholder to do this? @yurishkuro I think you hold the keys to this repo.

jcchavezs commented 7 years ago

I am ready to transfer the repo.

yurishkuro commented 7 years ago

added admins to the repo

jcchavezs commented 7 years ago

DONE https://github.com/opentracing/opentracing-php

tedsuo commented 7 years ago

Success!

bhs commented 7 years ago

🎉