search

opentracing / opentracing-php

OpenTracing API for PHP
Apache License 2.0
505 stars 56 forks source link

1.0 release or W3C specification? #93

Closed cawolf closed 3 years ago

cawolf commented 4 years ago

Hi,

we recently used your library to abstract from a Jaeger client used in our software. Currently, both this library and the client itself are in beta/dev states, but both proved to be very useful in our day-to-day work.

Seeing that the last commit to this repository was in April 18, I'm wondering if you are still pursuing a 1.0 stable release, or if we should turn our attention to the ongoing W3C draft ( https://w3c.github.io/trace-context/ ). What do you suggest?

Thanks for your insights, best regards, @cawolf

jcchavezs commented 4 years ago

I am up to release a 1.0 since there are libraries already relying on this library. Will see if I can get my head around it.

cawolf commented 4 years ago

Do you need any help? I'm happy to do so :)

jcchavezs commented 4 years ago

I think we can still move this to 1.0. There are a couple of issues we need to address first:

  1. PHP 7.1+
  2. Revisit the ScopeManager API
  3. Fix https://github.com/opentracing/opentracing-php/issues/91
  4. Address https://github.com/opentracing/opentracing-php/issues/35
  5. (nice to have but not necessarily required) https://github.com/opentracing/opentracing-go/pull/146

I think @piotrooo was also interested (sorry I took so long) and @ellisv might be too.

Do you feel like taking 1?

cawolf commented 4 years ago

Sure thing!

I did not find an issue describing the problem, and looking at the source, I found out that Travis is already configured for 7.1 through 7.3 and the package constraint is at ^7.1.

Can you open a seperate issue for that?

piotrooo commented 4 years ago

I think we should also clean (abandon?) opened PR’s, they are stale.

cawolf commented 4 years ago
  1. is done, can you explain 2. a little bit more?
jcchavezs commented 4 years ago

Revisit the ScopeManager API

We could stick with what we have now in terms of scope, my main concern was implementation when it comes to event loop in PHP but I would say we can postpone this until we can play around with things like https://github.com/driftphp/context

piotrooo commented 4 years ago

For sure you are familiar with https://www.php.net/supported-versions.php, what is a reason for supporting EOL 7.1 version?

cawolf commented 4 years ago

@piotrooo I just linked the PR, the discussion was already held here.

But to not just discard your concern: @jcchavezs, did your opinion on 7.1 vs 7.2 change in the meantime and should we upgrade to 7.2?

piotrooo commented 4 years ago

@cawolf I forgot about that. You are right.

cawolf commented 4 years ago

So, as far as I see there is only the optional tracer test suite for tracer implementors left to do. Does anything else stand in the way of releasing 1.0.0?

piotrooo commented 4 years ago

I think we should also:

  1. Cleanup (remove/merge) feature branches (fixes_mock_tests_ns, in_process_propagation, in_process_propagation_2 and removes_from_headers).
  2. Create branch 1.0.x for fixes which will be merged from master and cut from them 1.0.0 release.

PS. @jcchavezs from this time, you should make development on your own fork, and makes PRs for review and finally merge.

@cawolf any other thoughts?

cawolf commented 4 years ago

Can't come up with anything relevant right now @piotrooo . Any plans on how to tackle the cleanup?

piotrooo commented 4 years ago

How about my suggestions? Waiting also on @jcchavezs response.

cawolf commented 4 years ago

@jcchavezs do you have time to do the cleanup and prepare the release? Or can you elevate @piotrooo to be able to do it?

jcchavezs commented 4 years ago

Hi there! Sorry I haven't come back to you sooner. Before we can do the release I wanted to exercise the API in https://github.com/jcchavezs/zipkin-php-opentracing/ but did not have enough time to do it. I truly believe we need to make sure everything is alright and for that I would propose that we do PRs with the current master against https://github.com/jcchavezs/zipkin-php-opentracing/ and jonahgeorge/jaeger-client-php or jukylin/jaeger-php (reason I choose them is because they are the ones being used in https://github.com/hyperf/hyperf and https://github.com/cawolf/OpentracingBundle-core), that way we will make sure we are in the right track. Also feel free to mention another library you think is more interesting to be tried with new version.

Does that work for you? Could any of you take that work?

On Fri, 19 Jun 2020, 08:45 Christian Alexander Wolf, < notifications@github.com> wrote:

@jcchavezs https://github.com/jcchavezs do you have time to do the cleanup and prepare the release? Or can you elevate @piotrooo https://github.com/piotrooo to be able to do it?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/opentracing/opentracing-php/issues/93#issuecomment-646466894, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAXOYAVESNCT2BW5S3R42WTRXMCPTANCNFSM4J3ZDRYQ .

cawolf commented 4 years ago

I can do that for auxmoney/OpentracingBundle-Jaeger (including jukylin/jaeger-php), auxmoney/OpentracingBundle-Zipkin (including jcchavezs/zipkin-opentracing) and auxmoney/OpentracingBundle-core. I would just not like to release these versions with a dev-master dependency again, so would the quality checks in the PRs be enought to validate the functionality?

jcchavezs commented 4 years ago

I think so. Unless someone else has a project in mind that also is commonly used by community but I guess there is not.

On Tue, 30 Jun 2020, 08:59 Christian Alexander Wolf, < notifications@github.com> wrote:

I can do that for auxmoney/OpentracingBundle-Jaeger (including jukylin/jaeger-php), auxmoney/OpentracingBundle-Zipkin (including jcchavezs/zipkin-opentracing) and auxmoney/OpentracingBundle-core. I would just not like to release these versions with a dev-master dependency again, so would the quality checks in the PRs be enought to validate the functionality?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/opentracing/opentracing-php/issues/93#issuecomment-651587790, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAXOYAREDNPBPWQWRHCZQZTRZGELLANCNFSM4J3ZDRYQ .

jcchavezs commented 4 years ago

Also pinging @ellisv who has a Jaeger implementation, not sure how active it is as I think he uses inside his company https://github.com/ellisv/jaeger-client-php

On Tue, 30 Jun 2020, 09:31 José Carlos Chávez, jcchavezs@gmail.com wrote:

I think so. Unless someone else has a project in mind that also is commonly used by community but I guess there is not.

On Tue, 30 Jun 2020, 08:59 Christian Alexander Wolf, < notifications@github.com> wrote:

I can do that for auxmoney/OpentracingBundle-Jaeger (including jukylin/jaeger-php), auxmoney/OpentracingBundle-Zipkin (including jcchavezs/zipkin-opentracing) and auxmoney/OpentracingBundle-core. I would just not like to release these versions with a dev-master dependency again, so would the quality checks in the PRs be enought to validate the functionality?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/opentracing/opentracing-php/issues/93#issuecomment-651587790, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAXOYAREDNPBPWQWRHCZQZTRZGELLANCNFSM4J3ZDRYQ .

ellisv commented 4 years ago

Hey @jcchavezs I haven't been active for awhile at opentracing. I do not have anything to add and I am not against taking it to 1.0.

mszabo-wikia commented 4 years ago

https://github.com/jonahgeorge/jaeger-client-php is another active Jaeger client.

omnilight commented 3 years ago

Hi, is this library alive? It's stuck at beta6 for 1,5 years for now. It's PHP 8 released, so we need at least support for it (let it be beta7 for example).

Or maybe we can just release 1.0.0?

omnilight commented 3 years ago

@jcchavezs Hi, are we able to make a new release?

jcchavezs commented 3 years ago

From my POV i would be better if someone could work on the update for https://github.com/jcchavezs/zipkin-php-opentracing, https://github.com/jonahgeorge/jaeger-client-php or auxmoney/OpentracingBundle-Jaeger, auxmoney/OpentracingBundle-Zipkin and auxmoney/OpentracingBundle-core to make sure this API goes in the right direction. Not sure if @cawolf made some progress on that. Are you up to do that try @omnilight ?

omnilight commented 3 years ago

@jcchavezs From the one side we can start commiting to the other library, but I think it would be better to finish this one.

For now it's almost a year since last release, and I believe we can just release version 1.0, or at least a new beta

omnilight commented 3 years ago

Or even better, we can release for example version 0.1.0 or something. At least that would not force us to explicitly mention beta in requirements of root project

cawolf commented 3 years ago

Unfortunately, our base Jaeger implementation is stuck at a pre PHP 7.1 level, and the PR to solve it is stuck since June. We are currently thinking about forking the library under our company github handle and pushing it forward.

Unrelated to that, I came to the conclusion that we should be able to release a 1.0.0 anyway. From glancing over the implementing libraries, all of them should be able to adapt (faster or slower) to the major version and its requirements. The benefit for applications of using a stable release of opentracing-php imho outweighs the smaller changes needed for library owners.

jcchavezs commented 3 years ago

I opened a last PR and after that I will release 1.0

On Thu, 10 Dec 2020, 08:01 Christian Alexander Wolf, < notifications@github.com> wrote:

Unfortunately, our base Jaeger implementation is stuck at a pre PHP 7.1 level, and the [PR to solve it|https://github.com/ jukylin/jaeger-php/pull/82 https://github.com/jukylin/jaeger-php/pull/82] is stuck since June. We are currently thinking about forking the library under our company github handle and pushing it forward.

Unrelated to that, I came to the conclusion that we should be able to release a 1.0.0 anyway. From glancing over the implementing libraries, all of them should be able to adapt (faster or slower) to the major version and its requirements. The benefit for applications of using a stable release of opentracing-php imho outweighs the smaller changes needed for library owners.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/opentracing/opentracing-php/issues/93#issuecomment-742285894, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAXOYATCD633FOST5XFNCHLSUBW3VANCNFSM4J3ZDRYQ .

jcchavezs commented 3 years ago

Done https://github.com/opentracing/opentracing-php/releases/tag/1.0.0