[#60] Changes the API for the In process context propagation

[jcchavezs]

Read #61 for more context

Short description of what this PR does:

• Changes the Tracer::startActiveSpan signature to return a scope.
• Changes the ScopeManager to not to be aware of spans

This is a breaking change but since we are still in beta, it is OK.

Checklist

• I have made a material change to the repo (functionality, testing, spelling, grammar)
• I have read the [Contribution Guide] and my PR follows them.
• I updated my branch with the master branch.
• I have added tests that prove my fix is effective or that my feature works
• I have added necessary documentation about the functionality in the appropriate .md file
• I have added in line documentation to the code I modified

Fixes #61

Ping @beberlei @yurishkuro @ellisv @tedsuo

[yurishkuro]

overall looks reasoanable

[yurishkuro]

I think this example is promoting bad naming practice. root and http should really be called scope, and they should be in different syntax scopes (not sure if php allows that - if not, best to introduce a function in the example)

[yurishkuro]

move "or not" after "finished"

[yurishkuro]

not familiar with php syntax - what does this mean? looks like assigning scalar to array.

[ellisv]

This means to append entry to an array

[ellisv]

ScopedSpan class does not exist. It should return a Span.

[jcchavezs]

Totally. I was playing around with the idea of having such a thing but I decided to not to in the end.

[jcchavezs]

@yurishkuro @ellisv I fixed your comments.

[beberlei]

Sorry for commenting so late. I actually proposed the previous API, because i feel returning the scope makes for a bad API. You always grab the span from it to call additional stuff and its a constant law of demeter violation with complicated code.

PHP is single threaded, request based shared nothing. This is different to all other OT languages so far. So i felt it warranted a different API. In addition PHP doesn't have a with keyword like python/java, making the scope so weird, and $scope->close() is not called automatically.

The default, 90% use case for clients of this library would be the API:

$span = $tracer->startActiveSpan(...);
$span->annotate(...);
//..
$span->annotate(...);
$span->finish($closeOnFinish = true);

now its going to be:

$scope = $tracer->startActiveSpan(...);
$scope->getSpan()->annotate(...);
//..
$scope->getSpan()->annotate(...);
$scope->close($finishOnClose = true);

[ellisv]

In previous case having a scope was unecessary at all in my opinion as it was never exposed to the user and may confuse implementors. That is why I opened #55. Instead we could have removed scope concept at all and left Tracer::getActiveSpan() for implementors to implement in which ever way they want. Or something like

interface SpanManager
{
    public function activate(Span $span): void;
    public function getActive(): Span;
}

Or put activateSpan() on Tracer itself.

So in my opinion we can turn two ways:

• Implement the way it is done on Java and Python by exposing Scope to the user
• Do not have Scopes at all

[jcchavezs]

So I did this implementation three times under three different vendors (mock tracer, datadog and zipkin) I can share my experiences with this:

1. The previous API (that I endorsed) had a circular dependency between Span and Scope (both being aware of each other).
2. It was confusing who had the responsibility to finish a span after a scope.
3. ScopeManager was quite useless.

In the other hand, I agree with most of @beberlei's points as returning a Scope has some inconveniences.

At some point I came up with this approach of having a ScopedSpan which would be wrapping both a Scope and Span and implement both interfaces. That approach felt quite good until you did ScopeManager::activate which forced you to create a new object:

$scopedSpan = $tracer->getScopeManager()->activate($span);
$scopedSpan->tag(....)

So I backed off. To be honest this is not an inconvenience because ScopeSpan would carry the existing span + when doing activating you always need to keep track of the scope if you want to avoid the circular dependency.

I would go for the ScopedSpan, feels more natural for me.

Thoughts @beberlei @yurishkuro @ellisv

[yurishkuro]

I just opened this issue as it was on my mind recently. If it's decided in favor of ScopeContext then the notion of the Scope will probably need to be preserved.