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

Create OpenTracing Shim #747

Closed
tidal opened this issue Jul 2, 2022 · 6 comments · Fixed by open-telemetry/opentelemetry-php-contrib#157
Closed

Create OpenTracing Shim #747

tidal opened this issue Jul 2, 2022 · 6 comments · Fixed by open-telemetry/opentelemetry-php-contrib#157
Assignees
Labels
enhancement New feature or request help wanted This issue is looking for someone to work on it medium medium sized task opentelemetry-php-contrib This issue is related to the opentelemetry-php-contrib repository Work In Progress This is a Work in Progress, not ready to be merged

Comments

@tidal
Copy link
Member

tidal commented Jul 2, 2022

Specification Reference:
https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/compatibility/opentracing.md
OpenTracing PHP repository:
https://github.com/opentracing/opentracing-php

@tidal tidal added enhancement New feature or request help wanted This issue is looking for someone to work on it release:required-for-ga medium medium sized task opentelemetry-php-contrib This issue is related to the opentelemetry-php-contrib repository labels Jul 2, 2022
@Grunet
Copy link
Contributor

Grunet commented Jul 6, 2022

I'm guessing this one doesn't need to be thought out from scratch too much, and just following along with another language's implementation (or 2) with the spec at hand will get 90% of the way there.

Like for JS it looks like they managed this in roughly 400 lines of TS code - https://github.com/open-telemetry/opentelemetry-js/tree/f59c5b268bd60778d7a0d185a6044688f9e3dd51/packages/opentelemetry-shim-opentracing

If no one's able to pick this up after the next week or so, I might be able to and take a crack at it

@bobstrecansky
Copy link
Collaborator

@Grunet - are you still planning on working on this?

@Grunet
Copy link
Contributor

Grunet commented Aug 11, 2022

Am a little personally underwater atm (ideally will not be in ~2-3 weeks) but once not was planning to take a stab at #723 first.

But if this is a higher priority I can definitely try this first (and announce when I'm starting on it).

Otherwise totally cool if someone else wants to take a shot. I did not look into it too deeply before.

@Grunet
Copy link
Contributor

Grunet commented Sep 12, 2022

Looking at it more closely now, I have 2 other observations

  1. JS and Python keep all of the Shim classes in a single file, but Java separates them out into their own files with the usual 1 per class deal. Since PHP/this repo has the same style, Java is probably the example to follow most closely.
  1. The other repos all seem to publish a separate package for this shim, which the spec itself seems to kinda mandate when it says "This functionality MUST be defined in its own OpenTracing Shim Layer, not in the OpenTracing nor the OpenTelemetry API or SDK". So presumably the same needs to be done here, meaning the "gitsplit" setup needs to be extended I believe (initially added in this PR).

I'm starting at a new gig next week so won't have much time right after that probably, but I do have a day off before then, so I will see how far I can get with 1) and then drop any notes on partial/progress here

@Grunet
Copy link
Contributor

Grunet commented Sep 12, 2022

So there are a total of 27 methods across 5 interfaces that you need to fill out to make any OpenTracing-PHP implementation (including an OTEL-backed one as is needed here)

Scaffolding the initial files and taking a stab at the first 7 methods for OpenTracing's "Tracer" interface ended up taking me roughly around 7 hours (a large chunk of which was trying to understand-then-translate Java and JS's behaviors for the methods I didn't conceptually understand as well up front. I'm looking at you propagation lol). I also totally skipped some things for that interface implementation (like translating span creation options between OpenTracing and OTEL) just to try to get something in place for all of the methods.

You can see where I got to here - https://github.com/Grunet/opentelemetry-php/pull/6/files

I sadly most likely won't have mind energy in the next few weeks to carry this to completion, so anyone feel free to pick up from there as needed (but I will also aim to loop back if/when I do to see how I can help!)

@brettmc
Copy link
Collaborator

brettmc commented Feb 28, 2023

It looks like no other SIG has implemented this yet: https://github.com/open-telemetry/opentelemetry-specification/blob/main/spec-compliance-matrix.md#opentracing-compatibility so, my vote is to drop this as a requirement for a GA release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted This issue is looking for someone to work on it medium medium sized task opentelemetry-php-contrib This issue is related to the opentelemetry-php-contrib repository Work In Progress This is a Work in Progress, not ready to be merged
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants