opentracing-php full tree

[wladekb]

Place for feedback for initial opentracing implementation in PHP.

PROOF OF CONCEPT

[wladekb]

@bensigelman wrote: the short version of my high-level feedback in case it helps reduce a round-trip between us:

• thanks. already almost all of the way there IMO, great work.
• https://github.com/Wikia/opentracing-php/blob/master/src/OpenTracing/Span.php#L119 doesn’t need to be part of the Span interface in the other languages… it can be a (static) global function that takes the parent span and the span.tracer() combined with Tracer.startSpan() get the job done.
• the OT interface needs to define a few standard required formats and their respective carriers, much like https://github.com/opentracing/opentracing-go/blob/master/propagation.go#L60 and below
• it’s great that you took a stab at a standard tracer impl, but we are planning to move such helpers out of the main OT repos… so that code may live on, but probably not as a subdir.

oh: and if we move this into the opentracing org it would be good to make it “Copyright 2016: The OpenTracing Authors”. Is the MIT license “standard” for PHP OSS, btw? (Fine by me, but we are trying to choose whatever “reasonable” (MIT, Apache, etc) license is most popular for the particular platform)

[bhs]

@wladekb: let me know if you want more detail about any of the above bullets... in general it really pretty reasonable to me and most of the feedback will probably have to come from someone who understands PHP best practices better than I do.

[wladekb]

@gabrys @macbre @mixth-sense @kvas-damian wanna take a look in your spare time?

[bhs]

Two other things while I'm thinking of them:

• the various OT libraries have provided access to a global singleton Tracer instance... I don't see that here but we need some sort of story on that front
• I don't know much about PHP, but my understanding is that it's very difficult to retain state across requests... not sure if that will affect the way that Tracer/Span are designed, but it might be useful to look at other instrumentation libs in php for example patterns

[yurishkuro]

Agree with @bensigelman that standardtracer should not be part of this repo. Could rename it to basictracer and move to another repo.

It's important to have a no-op as default implementation, which is what stub package is doing. To validate API adherence I would recommend adding an API test harness like this one: https://github.com/opentracing/opentracing-python/blob/master/opentracing/harness/api_check.py

[yurishkuro]

One more thing - in both Go and Python we ended up defining explicit types for the two standard carriers. I am not seeing them here.

[wladekb]

@yurishkuro

1. I plan to add tests after the API gets a bit more stable.
2. I'll move the Basic tracer to separate repo later.
3. What's the reason for explicit types for carriers? @bensigelman yes, in PHP every request starts from scratch unless you create a C extension that has different life cycle I could add a TracerSingleton class with getInstance (automatically creates a Stub instance if none was set before) and setInstance methods that would emulate a singleton, but on the other hand you would still need to load the chosen Tracer implementation during bootstrap of your app.

[beberlei]

Global Tracer could mimic this: https://github.com/opentracing/opentracing-go/blob/master/globaltracer.go

Having a class OpenTracing\GlobalTracer with public static function init(Tracer $tracer), public static function get() and public static startSpan($operationName);.

[beberlei]

About Span::startChild, I see only three ways here:

• Make Span an abstact class and add the startChild implementation as static final function.
• Move startChild into a function in OpenTracing namespace and use Composer's file autoload strategy to include the file with the function during every request (not recommended).
• Introduce a static class with utility functions, maybe even the code for GlobalTracer as well, OpenTracing\Utils for example. But this would introduce a naming problem.

[wladekb]

@beberlei I started with creating an abstract class initially but then decided to go with interfaces since you don't have multiple inheritance in PHP and I didn't want to limit implementors. However given those 3 options i would call the abstract class approach as the most clean. How about defining a trait that would implement just this single method?

[beberlei]

@wladekb I like abstract class the most as well, a trait cannot be called statically and it has no interface, so users cannot trust objects to implement methods of a trait. This makes it bad to use in a generic library imho. If we have:

abstract class Span
{
    public static final function startChild(Span $parent, $operationName)
    {
        return $parent->getTracer()->startSpan($operationName, $parent);
    }
}

Or we do it as a dynamic method:

abstract class Span
{
    public final function startChild($operationName)
    {
        return $this->getTracer()->startSpan($operationName, $this);
    }
}

I prefer the second, it deviates a little from Go and Python, but is more idomatic in PHP.

In this case we could apply the same to Tracer interface and introduce the global tracer methods on an abstract Tracer class instead of introducing a second.

I don't think multiple inheritance is needed in implementations here, they are usually just adapters anyways, i dont see the value for implementors having interfaces over abstract classes here.

[wladekb]

@beberlei I think modern PHP is OO-based and thus to create a new Span object you need to derive it from a Tracer or other Span object so both of them should have (probably identical) non-static methods to achieve that. I can't fit a static method/function to this mindset so I'd go with creating a dynamic method. Makes sense?

[beberlei]

@wladekb My second example had a copy paste error, the static was wrong there. So i agree yes, we should have $span->startChild($operationName);, second example.

[yurishkuro]

1. What's the reason for explicit types for carriers?

so that both the end user and tracer implementor know what they are dealing with. At the moment split text propagator treats the carrier as a flat dictionary - that means it's not really split, the end user has no way of deciding which keys from that dictionary represent trace context vs. baggage.

[wladekb]

@yurishkuro would an array be enough for specifying the carrier type? (array is like a map in Go but as PHP is weak-typed lang you don't need to specify types of keys and values)

[yurishkuro]

If you look at Go/Python impls, the carrier type is defined conceptually as a struct with two maps (or two binary blobs). Is there a reason why using an (associative) array in PHP is better than an explicit type?

[wladekb]

@yurishkuro In PHP you usually go with plain arrays or full-blown classes that do something. Anyway for the sake of consistency I updated to have specific carrier types for SplitText and SplitBinary.

[bhs]

@wladekb:

@bensigelman

yes, in PHP every request starts from scratch unless you create a C extension that has different life cycle

I could add a TracerSingleton class with getInstance (automatically creates a Stub instance if none was set before) and setInstance methods that would emulate a singleton, but on the other hand you would still need to load the chosen Tracer implementation during bootstrap of your app.

@bcronin would you mind taking a quick look at this and comparing against your PHP instrumentation trauma[s]?

[wladekb]

@bensigelman @bcronin FYI, we're a few hours of work later and there's a Tracer::getGlobalTracer and Tracer::setGlobalTracer but it currently doesn't autospawn a Stub implementation

[bhs]

@wladekb can you summarize the status here? Are you just waiting for an LGTM or are there significant open issues? Thanks.

[bhs]

@wladekb I've been talking with a PHP-using org that is interesting in OT and I wasn't sure of your timeline for picking this up again. Anything to share? And/or would you be irritated/annoyed if it got forked and continued elsewhere from a dev standpoint? You have done some great work here and it looks like it's getting close to where it needs to be.

[wladekb]

@bensigelman

I'd say this is a fake PR as it tried to merge from master to an arbitrary empty branch. I created it to make browsing the code easier and have all files in a single place - the "Files changed" tab and make sure we have a common place to place general comments.

As I had to move my full-time focus away from opentracing I thought to resync the API in batches. I've been keeping track briefly and have seen the evolution is ongoing so I still waited for a best moment. If you think that now is the moment just let me know and I'll update it within a couple of days.

That's a very good news that some other companies are getting interested in opentracing. I fully believe in the goals and I'm happy to have seen the progress in the project over the last couple of weeks. However I lost an ability to focus on it full-time as we decided not to use the opentracing API yet I think it makes sense to fork/move it to opentracing org. Let me know if you need any cooperation from me.

[bhs]

@wladekb thanks for the update! We are getting close to stabilization with respect to the propagation specification. The only other important issues in my mind are info/error logging details (which you've brought up yourself IIRC) and how to set something like an error bit on a Span instance, but both of those will likely be strict additions to the API, not changes.