Adds activeSpanSource #22

jcchavezs · 2017-06-15T10:46:38Z

CONTINUE THE DISCUSSION in opentracing/opentracing-php#2

This is related to the in_process_propagation from: opentracing/opentracing-python#52

The specification context is "In-process propagation" discussed in this issue opentracing/specification#23, already for Java and in PR for Python.

Ping @jcchavezs @felixfbecker @tedsuo @beberlei

jcchavezs · 2017-06-15T11:03:37Z

src/OpenTracing/ActiveSpanSource.php

+     * @param string|array $threads Use only in case of async operations. Allows you to keep
+     * an span as active in different threads.
+     */
+    public function activate(Span $span, $threads = self::THREAD_DEFAULT);


I was not so sure about this. I thought about having this interface and then two implementations: AsyncActiveSpanSource and ActiveSpanSource but they are conceptually two different use cases and consequently two different method signatures which is not allowed for interfaces.

We can of course cheat by using func_get_args but I am not so sure about that being a good idea. I'd like the code to be more descriptive.

Putting the concepts of threads into this sounds weird given that PHP is completely single-threaded and there is no reliable cross-platform thread extension as far as I know.

$thread is actually an identificator, nothing to do with runtime concept. My initial idea was to use something like tag but that concept is already in use.

@jcchavezs but how do independent third party libraries coordinate on what identification to use? That is unsolved, but it is actually solved by the ActiveSpanSource itself, providing $threads from outside into API just breaks that encapsulation.

beberlei · 2017-06-15T11:03:39Z

src/OpenTracing/NoopTracer.php

+    /**
+     * {@inheritdoc}
+     */
+    public function startActiveSpan(
        $operationName,
        SpanReference $parentReference = null,


Active Span needs no reference anymore, and this being the second argument will lead to users having startActiveSpan('op1', null, ...);

Why is there no array of options anymore? Ruby and Python have a list of arguments as well, but both languages support named arguments, so they could do startActiveSpan('op1', tags=['foo' => 'bar']); which PHP cannot do.

beberlei · 2017-06-15T11:05:49Z

src/OpenTracing/ActiveSpanSource.php

+     * @param string|array $threads Use only in case of async operations. Allows you to keep
+     * an span as active in different threads.
+     */
+    public function activate(Span $span, $threads = self::THREAD_DEFAULT);


The use of $threads make no sense to me, because you assume that at every location in the code, it knows what its own thread id is. You cannot guarantee this.

In addition it puts the responsibility for the thread selection to the client, but the client doesn't necessary know what kind of threading context it is in. The ActiveSpanSource should now and the python implementation PR has examples how custom ActiveSpanSources handle this internally, not expose this to the client.

I am against this approach.

$thread is actually an identificator, nothing to do with runtime concept. My initial idea was to use something like tag but that concept is already in use. Anyways this was just a try to deal with asynchronous difficulties. Thanks for the feedback tho.

beberlei · 2017-06-15T11:07:37Z

src/OpenTracing/SpanOptions.php

@@ -33,6 +43,14 @@ public static function create(array $options)

        foreach ($options as $key => $value) {
            switch ($key) {
+                case 'is_active':


The SpanOptions are for creating spans from the client POV. In that context is active or not is implicitly set by the call to either startActiveSpan or startManualSPan, so having this flag on options is redundant.

beberlei · 2017-06-15T11:09:29Z

src/OpenTracing/Tracer.php

+    /**
+     * @return ActiveSpanSource
+     */
+    public function activeSpanSource();


the coding style is inconsistent, some places use get*, this place here does not. It looks like a direct translation from python, where this is a coding convention.

I tried to avoid all gets in the code. If there is any around I will open a PR changing them.

@jcchavezs you mentioned you consider get/set anti patterns, I'm curious why? It certainly is common in PHP libraries

Based on the changes on #30 I will change the methods here to use get prefixes. About setters, as I expressed before (somewhere) I prefer meaningful methods like $object->doSomethingYouUnderstand() rather than $object->setYourStateTo($whatever) (for example, ->activate() over ->setStatus('active').

yeah, totally agree with meaningful method names like activate(), as long as they are verbs. E.g. activeSpanSource is not a verb and therefor gives no real indication what it does (does it return the value? or does it set it? or both, like jQuery?)

jcchavezs · 2017-06-15T14:36:31Z

@beberlei @felixfbecker I just addressed your comments.

jcchavezs · 2017-06-15T18:20:17Z

src/OpenTracing/Span.php

@@ -15,10 +15,13 @@ public function operationName();
    public function context();

    /**
+     * Finishes the span and locks it so it becomes immutable. As an implementor, make


I believe this is a necessary recommendation, does it make sense?
@beberlei @felixfbecker

I don't think it has to "lock" it (as in all methods now throw exceptions), but I read that after finish() you should not use the Span anymore.

That was the intention, how would you reword it?

felixfbecker · 2017-07-06T07:18:56Z

README.md

-    use OpenTracing\SpanReference\ChildOf;
+	use OpenTracing\SpanReference\ChildOf;
+
+	$parent = GlobalTracer::globalTracer()->startManualSpan('parent');	$child = GlobalTracer::globalTracer()->startManualSpan('child', [


theres a weird line break here

felixfbecker · 2017-07-06T07:19:28Z

README.md

+	...
+	$child->finish();
+	...
+	$parent->finish();


shouldn't finish() always be called in a finally?

felixfbecker · 2017-07-06T07:19:45Z

README.md

+Every new span will take the active span as parent and it will take its spot.
+
+```php    
+	$parent = GlobalTracer::globalTracer()->startManualSpan('parent');        ...


indentation

felixfbecker · 2017-07-06T07:20:48Z

src/OpenTracing/ActiveSpanSource.php

+     *
+     * @return Span
+     */
+    public function activeSpan();


should be getActiveSpan()

…cer.

felixfbecker · 2017-07-12T09:57:05Z

src/OpenTracing/Tracer.php

@@ -25,6 +25,13 @@
    const FORMAT_HTTP_HEADERS = 3;

    /**
+     * @deprecated use either startActiveSpan or startManualSpan instead.


Imo the initial official version shouldn't have anything deprecated.

The problem is that we'd prefer the python one to be released before merging this one so either we wait or either we release the library without the active span record.

what is the reason for that?

tedsuo · 2017-07-15T01:14:36Z

There is some discussion of ActiveSpan API in Java right now, might want to hold off and coordinate with where that is going: opentracing/specification#80

felixfbecker · 2017-07-15T08:14:38Z

I don't fully understand what is referred to by "refcounting". Does that apply to PHP?

jcchavezs · 2017-07-15T09:42:45Z

As pointed out by @tedsuo there are some discussions about this feature in opentracing/specification#80. After a long read through the issue and its replies, my thoughts are that the reference counting is not an issue for us because in PHP everything happens in one request and there is no way to modify the context by other requests so whether we implement refcounting for auto-finishing or not, that won't be a big problem for us. What is true is that auto-finishing is something desirable by some implementors.

In the other hand I do believe there is not a consensus about this among implementations. @bhs might come up with a solution for this (according to opentracing/specification#80 (comment)) but my feeling is that this concept is not defined enough so I would hold for that meaning that I'd launch the library without including ActiveSpanSource for now.

What do you think @felixfbecker, @beberlei & @tedsuo?

PS: I am not sure if we might mention the Continuation and capture concepts that come up in the Java and python implementation. This is something we don't need (IMO) since there is only one thread and the span lifecycle is scoped on the request but I think that should be written somewhere so a developer coming from one of this libraries is aware of that.\

felixfbecker · 2017-07-15T09:49:28Z

in PHP everything happens in one request and there is no way to modify the context by other requests

that is not entirely true, please keep in mind that PHP can be used in more contexts than just as a SAPI module (i.e. not just as an Apache/nginx request handler). PHP can be used to build long-running RPC server processes, for example with an event loop. However, the event loop use case can be solved by integrating into the event loop. PHP is single-threaded, so there is no chance of two pieces of code running at the same time and modifying the active span.

Not sure if that is what "refcounting" does, but PHP has destructors. I.e. Span can have a method __destruct() that can call finish() when the GC destroys the span cause there are no references to it anymore. Not sure whether we would want that cause the time GC run is not guaranteed and therefor destructors are really bad for time-relevant operations like finishing a span.

jcchavezs · 2017-07-15T09:59:27Z

Agree @felixfbecker but even in that case, every thread of PHP is closed to the others.

As far as I understand, the main point with the refcounting (and I think it is not the refcounting itself but the way the refcounting is being done) is the possibility of an span to be kept opened by other thread without doing anything, that is not our case (I think) so as I said, this is not something that would concern us but I do believe we should wait on this PR until there is an agreement, does it makes sense to you?

jcchavezs · 2017-07-20T09:27:26Z

PLEASE CONTINUE THE DISCUSSION AT opentracing/opentracing-php#2

jcchavezs force-pushed the in_process_propagation branch from 0d5d912 to e983dfc Compare June 15, 2017 10:55

jcchavezs commented Jun 15, 2017

View reviewed changes

beberlei reviewed Jun 15, 2017

View reviewed changes

jcchavezs force-pushed the in_process_propagation branch 2 times, most recently from 1d004e4 to f78fd8e Compare June 15, 2017 14:13

jcchavezs force-pushed the in_process_propagation branch 2 times, most recently from b174bf3 to d7d4b97 Compare June 15, 2017 17:06

jcchavezs commented Jun 15, 2017

View reviewed changes

jcchavezs force-pushed the in_process_propagation branch 2 times, most recently from d6e206c to b8a3ec3 Compare June 15, 2017 21:17

felixfbecker reviewed Jul 6, 2017

View reviewed changes

Adds activeSpanSource interface and include it as a dependency on tra…

307a5fa

…cer.

jcchavezs force-pushed the in_process_propagation branch from b8a3ec3 to 307a5fa Compare July 12, 2017 07:39

Renames activeSpan into getActiveSpan on SpanSource.

acd73b0

felixfbecker reviewed Jul 12, 2017

View reviewed changes

jcchavezs mentioned this pull request Aug 1, 2017

In process propagation opentracing/opentracing-php#2

Closed

jcchavezs closed this Dec 3, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds activeSpanSource #22

Adds activeSpanSource #22

jcchavezs commented Jun 15, 2017 •

edited

Loading

jcchavezs Jun 15, 2017 •

edited

Loading

felixfbecker Jun 15, 2017

jcchavezs Jun 15, 2017

beberlei Jun 15, 2017

beberlei Jun 15, 2017

beberlei Jun 15, 2017

jcchavezs Jun 15, 2017

beberlei Jun 15, 2017

beberlei Jun 15, 2017

jcchavezs Jun 15, 2017 •

edited

Loading

felixfbecker Jun 15, 2017

jcchavezs Jul 6, 2017 •

edited

Loading

felixfbecker Jul 6, 2017 •

edited

Loading

jcchavezs commented Jun 15, 2017

jcchavezs Jun 15, 2017

felixfbecker Jun 15, 2017

jcchavezs Jun 15, 2017

felixfbecker Jul 6, 2017

felixfbecker Jul 6, 2017

felixfbecker Jul 6, 2017

felixfbecker Jul 6, 2017

felixfbecker Jul 12, 2017

jcchavezs Jul 12, 2017

felixfbecker Jul 12, 2017

tedsuo commented Jul 15, 2017

felixfbecker commented Jul 15, 2017

jcchavezs commented Jul 15, 2017

felixfbecker commented Jul 15, 2017 •

edited

Loading

jcchavezs commented Jul 15, 2017

jcchavezs commented Jul 20, 2017

Adds activeSpanSource #22

Adds activeSpanSource #22

Conversation

jcchavezs commented Jun 15, 2017 • edited Loading

jcchavezs Jun 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcchavezs Jun 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcchavezs Jul 6, 2017 • edited Loading

Choose a reason for hiding this comment

felixfbecker Jul 6, 2017 • edited Loading

Choose a reason for hiding this comment

jcchavezs commented Jun 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tedsuo commented Jul 15, 2017

felixfbecker commented Jul 15, 2017

jcchavezs commented Jul 15, 2017

felixfbecker commented Jul 15, 2017 • edited Loading

jcchavezs commented Jul 15, 2017

jcchavezs commented Jul 20, 2017

jcchavezs commented Jun 15, 2017 •

edited

Loading

jcchavezs Jun 15, 2017 •

edited

Loading

jcchavezs Jun 15, 2017 •

edited

Loading

jcchavezs Jul 6, 2017 •

edited

Loading

felixfbecker Jul 6, 2017 •

edited

Loading

felixfbecker commented Jul 15, 2017 •

edited

Loading