Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

In process propagation #2

Closed
wants to merge 2 commits into from
Closed

In process propagation #2

wants to merge 2 commits into from

Conversation

jcchavezs
Copy link
Collaborator

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

```php
$parent = GlobalTracer::getGlobalTracer()->startManualSpan('parent'); ...

// Since the parent span has been created by using startActiveSpan we don't need
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You use startManualSpan for the parent here. I think this is a copy-and-paste mistake, since you want to demonstrate the use of startActiveSpaǹ.

@tPl0ch
Copy link
Contributor

tPl0ch commented Jul 29, 2017

Hi, as a possible implementer I am asking myself if it makes sense waiting for this PR to be merged. When do you plan to add this?

@jcchavezs
Copy link
Collaborator Author

@tPl0ch nice to have you around. So basically there is a lot of buzz around this concept of in process propagation mostly in Java (opentracing/opentracing-java#166) and even the python implementation hasn't been merged yet. Although most of the concerns discussed in the Java PR does not affect us (as we don't have threads) it feels like there is not a full agreement on the main concept so we kind of decided to hold on this PR until there is a solid agreement on the final spec and then we implement what is needed.

It is up to you to wait on this one but I encourage you to start doing it right now and give us some feedback on your experience using this library, I'd tell you that if we merge this PR we won't include breaking changes so even if we merge it in the mean time it won't end up on too much rework.

Finally I'd like to mention that this is mostly a RC yet, but we plan to release it very soon (with small tweaks but nothing substantial) so feel confident to start using it. It would be also nice if you can come and say hi to our gitter room: https://gitter.im/opentracing/opentracing-php

@tPl0ch
Copy link
Contributor

tPl0ch commented Jul 30, 2017

@jcchavezs The reason why I am asking is that when I was browsing the repository I realized that some of the features developed in this PR have leaked into the master branch. See https://github.com/opentracing/opentracing-php/blob/master/src/OpenTracing/SpanOptions.php#L6 - ChildOf is not defined.

Generally I am also missing some statements about the stability of this library. Is this lib going to follow a semantic version approach?

EDIT:

Additionally, this PR would break the Tracer API, see https://github.com/opentracing/opentracing-php/pull/2/files#diff-bd85a3792bd64b152939e0b1b78086e2R32 .

EDIT 2:

Although most of the concerns discussed in the Java PR does not affect us (as we don't have threads)

See http://php.net/manual/en/intro.pthreads.php. It is completely possible to do threaded programming in PHP (the question if you should is another one, though).

@jcchavezs
Copy link
Collaborator Author

@tPl0ch I am sorry I did not give you more context on this PR. Basically this branch is outdated and I think I will close this for now until the in process propagation is fully defined so we avoid confusions. Regarding the missing features in master, let me clarify it: this branch was created a month ago when the API was slightly different than now (see jcchavezs/opentracing-php#22) so some concepts were changed since then (one of the more visible ones was the fact that now we don't have specific classes per reference type but we have one Reference class). In addition, when this branch was created we expected this concept to be already settled and we accounted for the Python implementation to be merged very soon (which was not the case). In summary, whenever this PR is ready to be taken into account, it will be open for people to discuss but it should not be subject of discussion for now (this is on me).

Regarding the threads in PHP I knew about this pthreads library in V2 but I have the impression (correct me if I am wrong) that it is not wide used and don't know much about production systems using it but if there is a real interest in it we could include it later.

Finally, we are discussing now the versioning for this library. To be clear, what we have now is mostly what we might have as a first release and soon we will tag this code as the initial version (there are high probabilities to call it 1.0-beta1 or something like that) and yes, we will of course use semver

If you are about to (or up to) work on an implementation of opentracing in PHP come and talk with us in the gitter channel and we will more than happy to listen to you and solve any doubt you might have.

@jcchavezs
Copy link
Collaborator Author

Closed for now.

@jcchavezs jcchavezs closed this Aug 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants