In process propagation

[jcchavezs]

This is related to the in_process_propagation.

Ping @beberlei @felixfbecker @yurishkuro @tPl0ch

[tedsuo]

Any reason startActiveSpan can't be called startSpan, rather than deprecate the method?

[jcchavezs]

@tedsuo we are following other implementations like python

[tedsuo]

Fair enough. I'd prefer we hadn't called it StartActiveSpan there either, but perhaps that ship has sailed...

[jcchavezs]

Ping @jonahgeorge

[jcchavezs]

We have two options here:

• We remove the startSpan method and notify all the implementors about this or
• We keep the startSpan as deprecated.

Whatever we choose I believe we will be ready to release 1.0.0. Please react with :one: or :two: wether you prefer the first one or the second one. Otherwise drop a comment expressing your concerns.

Ping @tPl0ch @tedsuo @felixfbecker @beberlei @yurishkuro @lvht @jonahgeorge @reimertz

[jcchavezs]

:one:

[taoso]

@jcchavezs In my opinion, only startSpan is really needed.

[taoso]

I think we should only offer the most essential API. Less is more, and less is better.

The opentracing specific even does not define a trace-id!!! It only defined span, tag, context, and reference. It's enough. How to trace all spans with different references is all about implementation. So should our php interface.

[taoso]

@jcchavezs Any thing not specified in the OT spec should never be part of our opentracing-php API.

Thank you.

[jonahgeorge]

I would also prefer renaming startActiveSpan to startSpan rather than deprecating startSpan or removing it.

[felixfbecker]

I would like to keep a method for manual span creation around as a fallback to use in an environment that needs to rely on manual span propagation. For example, I currently use https://github.com/sabre-io/event as an event loop, which doesn't have hooks to make sure the active span is correctly set for each event loop tick / context. So I have to resort back to passing around the Span objects as parameters, which I have no problem with, so I don't see any harm in keeping this functionality.

[taoso]

I need to make a summary of my consideration. @beberlei @felixfbecker @yurishkuro @tPl0ch @jcchavezs @tedsuo @jonahgeorge

My Opinion

I think we should only offer the most essential API. Less is more, and less is better. Any thing not specified in the OT spec should never be part of our opentracing-php API.

I have said these words again and again.

In my opinion, the job to maintain the reference between span should leave to the user, even not the OT vendor.

However, if we really like to shift this feature, there still some issue in this PR.

The current issues

startSpan API name

The current startSpan is the very startManualSpan. Why introduce another startManualSpan and rename the current startSpan to startActiveSpan?

I propose to introduce the startActiveSpan API only.

The finish behavior of the finish API of the active span

With the startActiveSpan API, we get a span that has a parent of the current span and then become the new current span. It sounds great. However, what will happen if we call the new generated span's finish method?

In my opinion, the tracer should finish the span normally, and restore the current span with the stoped span's parent span. But I can not see any requirements in the API's doc.

The getActiveSpanSource API

Why people use the startManualSpan API? I think they use the API because they have their own infrastructure to maintain the reference of there span. It seems no need to use the getActiveSpanSource API to get the shifted ActiveSpanSouce for them. If they can use our ActiveSpanSource, why not use the getActiveSpanSource directly?

So I propose leave the ActiveSpanSource for internal use only, and drop the getActiveSpanSource API. As a result, the ActiveSpanSource interface can be dropped as well.

The getActiveSpan API

Please offer more detail about the getActiveSpan API.

If user use the startActiveSpan API, the Tracer will trace span automatically. If they trace span themselves, the will use the startSpan(or your startManualSpan), and the getActiveSpan will return nothing.

I can not see any need to use this API.

[taoso]

@tedsuo we are following other implementations like python

@jcchavezs Please following the OT spec.

[jcchavezs]

Unfortunately the implementation details changed and the Python PR (which is the one I based this PR on) has been closed in favour of https://github.com/opentracing/opentracing-python/pull/63.

I am closing this PR for now. I created an issue for this matter and I will open a new PR by this week.

[beberlei]

@jcchavezs the only thing that changed in the new python implementation is this weird with something as scope change, which PHP doesn't have in the language (blocks), so we should keep this PR.

[jcchavezs]

@lvht, thanks for the feedback. Please let's follow up further questions in https://github.com/opentracing/opentracing-php/pull/45

startSpan API name The current startSpan is the very startManualSpan. Why introduce another startManualSpan and rename the current startSpan to startActiveSpan? I propose to introduce the startActiveSpan API only.

In some cases you don't want to create a scope for in-process context propagation. For example in concurrency situations, after pcntl_fork you don't want to open a scope for internal spans created in both parent and child process.

The finish behavior of the finish API of the active span With the startActiveSpan API, we get a span that has a parent of the current span and then become the new current span. It sounds great. However, what will happen if we call the new generated span's finish method? In my opinion, the tracer should finish the span normally, and restore the current span with the stoped span's parent span. But I can not see any requirements in the API's doc.

This is a valid concern an a good question. We discussed this a long time ago and we decided to decouple the behaviour, that is why we added this note: https://github.com/opentracing/opentracing-php/pull/45/files#diff-e01221a8e1fafbccea47b6bc2f5f9f28R33

The getActiveSpanSource API Why people use the startManualSpan API? I think they use the API because they have their own infrastructure to maintain the reference of there span. It seems no need to use the getActiveSpanSource API to get the shifted ActiveSpanSouce for them. If they can use our ActiveSpanSource, why not use the getActiveSpanSource directly? So I propose leave the ActiveSpanSource for internal use only, and drop the getActiveSpanSource API. As a result, the ActiveSpanSource interface can be dropped as well.

I did a change in this direction. To be honest having the scope control inside the tracer make things more understandable. I had the opportunity to try the separated objects approach and it was quite inconvinient.

The getActiveSpan API Please offer more detail about the getActiveSpan API. If user use the startActiveSpan API, the Tracer will trace span automatically. If they trace span themselves, the will use the startSpan(or your startManualSpan), and the getActiveSpan will return nothing. I can not see any need to use this API.

You might want to add specific tags or logs to the current span and that is not possible without accessing the currentSpan.

[jcchavezs]

Closing in favor of https://github.com/opentracing/opentracing-php/pull/47