Allow method chaining for Span

[ellisv]

This allows the usage of

$span
    ->log([...])
    ->setTag(Tags\ERROR, true);

This behavior is also found in other OpenTracing libraries.

[jcchavezs]

Hi Eligijus! Personally I am against fluent methods on interfaces. There is a very nice post from @ocramius about this: https://ocramius.github.io/blog/fluent-interfaces-are-evil/

Since Span is in fact an interface I think this will make the API more complex for implementors and users.

In the other hand I don't see what is the problem this is solving. However I am open to discuss it.

Thanks!

Ping @yurishkuro @beberlei

[Ocramius]

Indeed: there is no real need to chain methods this way, so I suggest avoiding it unless necessary

[ellisv]

The reason I suggest a change is that I'd like to keep this library as similar as possible with other language libraries. This is found on:

• Python: https://github.com/opentracing/opentracing-python/blob/master/opentracing/span.py#L95
• Java: https://github.com/opentracing/opentracing-java/blob/master/opentracing-api/src/main/java/io/opentracing/Span.java#L39

[Ocramius]

@ellisv the same issues described (linked) above are applicable to Python/Java in the same way

[jcchavezs]

Maybe @carlosalberto or @palazzem have something to say here too. However I don't think it is an option to change anything in java.

[ellisv]

@Ocramius great article that makes you think and I really appreciate expressed opinion in there.

In article you identified such problems with fluent interfaces:

• Fluent Interfaces break Encapsulation
• Fluent Interfaces break Decorators (and sometimes Composition)
• Fluent Interfaces are harder to Mock
• Fluent Interfaces make diffs harder to read
• Fluent Interfaces are less readable (personal feeling)
• Fluent Interfaces cause BC breaks during early development stages

Fluent Interfaces break Encapsulation

The point expressed there is that you do not know whenever a method returns a new instance or the same instance. Breaking encapsulation is quite a harsh statement to be saying. And we do have a way of communicating this

/** @return $this returns the same object instance */
/** @return self returns the same type of object that may not be the same instance */

Fluent Interfaces break Encapsulation (and Composition)

I do not agree with that statement again. So you gave the example of

class SpanDecorator implements Span
{
    // ...
    public function log(array $fields, $timestamp = null)
    {
        return $this->original->log($fields, $timestamp);
    }
    // ...
}

But that is not a correct way of doing a decorator whenever you have @return $this documented on the interface. To actually fulfill the interface you would have to do:

class SpanDecorator implements Span
{
    // ...
    public function log(array $fields, $timestamp = null)
    {
        $this->original->log($fields, $timestamp);

        return $this;
    }
    // ...
}

So this no longer hold to be true.

Fluent Interfaces are harder to Mock

I do agree on this remark!

Fluent Interfaces make diffs harder to read

On that I quote a comment in your page:

Making diffs harder to read is a red herring, IMHO, I often lack context in a diff even with non-fluent interfaces -- that's a failing of the code review tool (e.g. not using "git diff -U" to provide more context) not the interface.

Also this is for you as the user to decide whenever you want to use chaining or not in your code.

Fluent Interfaces are less readable

Is a personal opinion so no discussions there.

Fluent Interfaces cause BC breaks during early development stages

You mention there that changing from void return type to any type is backwards compatibile. Imagine this scenario:

• We return void on Span::log()
• Every single OpenTracing implementation does return void on Span::log()
• Now we decide that Span::log() returns bool whenever call succeeded and release this under a patch or minor version (because we assume it's backwards compatible)
• Now in the project I composer update and get a new version of Span interface
• I write my code in the project based on assumption that Span::log() returns a bool (because it says so in the interface) and do create conditional checks based on that
• Because this was released as a patch or minor version OpenTracing implementations still return void hence this is backwards incompatible

And this is especially not true since php 7.1 as we explicitly specify return type of void.

[Ocramius]

Right, I'm gonna pull up something reeeeeeally simple and then bail out of the thread (regardless of outcome):

• It takes a lot of discussion to introduce something like this
• there are well known issues with it, some of which clearly to be discussed
• the change is not necessary at all, as the library works just fine without

In such a scenario, unless there is a clear cut on the added value, as a maintainer I'd rather not introduce the change.

[yurishkuro]

TBH in all the instrumentations I've written with OpenTracing in different languages, I don't recall using fluent API. I may have done it but it didn't register.

[carlosalberto]

Similarly to @yurishkuro, I've written some Python instrumentation and haven't used the fluent API much.

[jcchavezs]

thoughts @ellisv?

[ellisv]

In my opinion fluent interface is not necessary and in most cases you do not have many calls on a Span itself in a single block of code. But because every other opentracing library does have that I thought I'd launch at least a discussion to have this in php library too.

[piotrooo]

As @ellisv wrote:

But because every other opentracing library does have that I thought I'd launch at least a discussion to have this in php library too.

That is only one reasons that convinces me, so maybe we also should have fluent interface (but I'm a Java dev, so this is only my opinion).

[a-dyakov-mercuryo]

I agree with @ellisv. Fluent interfaces more convenient and more readable.