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

Opentracing shim #1909

Merged
merged 66 commits into from
Mar 3, 2023
Merged

Opentracing shim #1909

merged 66 commits into from
Mar 3, 2023

Conversation

chusitoo
Copy link
Contributor

@chusitoo chusitoo commented Jan 9, 2023

Fixes #78, answers to #1180

Changes

  • Implement Opentracing shim as described in https://opentelemetry.io/docs/reference/specification/compatibility/opentracing/
  • Unit tests are a combination of legacy Opentracing tests as well as new tests mimicking the Java and JS counterparts
  • Add CMake support (turned off by default) for externally linked Opentracing library or use local submodule
  • Add basic build support with Bazel. Windows is unsupported due to Opentracing lacking build support for the platform.

For significant contributions please make sure you have completed the following items:

  • CHANGELOG.md updated for non-trivial changes
  • Unit tests have been added
  • Changes in public API reviewed

@chusitoo chusitoo requested a review from a team January 9, 2023 04:44
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jan 9, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@codecov
Copy link

codecov bot commented Jan 9, 2023

Codecov Report

Merging #1909 (ac8dc8f) into main (9a5bb8d) will decrease coverage by 0.02%.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1909      +/-   ##
==========================================
- Coverage   87.32%   87.29%   -0.02%     
==========================================
  Files         166      166              
  Lines        4673     4673              
==========================================
- Hits         4080     4079       -1     
- Misses        593      594       +1     
Impacted Files Coverage Δ
sdk/src/trace/batch_span_processor.cc 90.70% <0.00%> (-0.77%) ⬇️

@owent
Copy link
Member

owent commented Jan 9, 2023

Is it better to move this into a standalone directory?

@lalitb
Copy link
Member

lalitb commented Jan 9, 2023

Thanks for the PR. Tagging @rnburn as someone contributed to both otel-cpp and opentracing-cpp if he would like to review this. though I don't see him active on Github for long now, just trying the luck :)

@marcalff
Copy link
Member

marcalff commented Feb 9, 2023

The copyright check in CI is new, please fix:

Missing copyright in ./opentracing-shim/BUILD
Missing license in ./opentracing-shim/BUILD
Missing copyright in ./opentracing-shim/CMakeLists.txt
Missing license in ./opentracing-shim/CMakeLists.txt

Copy link
Member

@marcalff marcalff left a comment

Choose a reason for hiding this comment

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

In third_party/prometheus-cpp, the git submodule for prometheus changed.

It looks like a merge issue with:

commit 9200d0c8fb4684e5fffd2c6915c8117a747c8e2b
Author: Lalit Kumar Bhasin <lalit_fin@yahoo.com>
Date:   Sat Feb 4 20:32:33 2023 -0800

    upgrade prometheus-cpp to v1.1.0 (#1954)

Please fix.

Copy link
Member

@marcalff marcalff left a comment

Choose a reason for hiding this comment

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

@chusitoo

If I understand correctly, the Opentracing shim introduces a dependency on the opentracing-cpp repository, which is a deprecated library, per:

opentracing/specification#163

It is a bit unusual to add dependencies like that, but it makes sense in this case, so ok.

Not knowing enough of Opentracing, it is a bit hard to review the code for correctness for every details.

That said, the overall code looks ok.

The contributed code is protected by a WITH_OPENTRACING option, and is separate from the opentelemetry-cpp code which is otherwise unchanged, so there is very little technical risks with this PR, in my opinion.

SpanContextShim::extractFrom() has a hack with kUniqueTag, but after looking at it I don't have a better proposal to offer, so ok with it.

Please fix the prometheus-cpp spurious change, and I will approve this PR.

Great work.

Copy link
Member

@marcalff marcalff left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the contribution.

Copy link
Member

@lalitb lalitb left a comment

Choose a reason for hiding this comment

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

Sorry for responding late on it. Changes in general look good, and clean approach - the shim confined to separate directory, and behind feature flag.
Most of the existing contributors doesn't have expertise on Opentracing, and bandwidth to manage this component, so @chusitoo it would be appreciated if you can own any related issues/changes going ahead :)

@lalitb lalitb added the ok-to-merge The PR is ok to merge (has two approves or raised by a maintainer/approver and has one approve) label Mar 3, 2023
@chusitoo
Copy link
Contributor Author

chusitoo commented Mar 3, 2023

Sorry for responding late on it. Changes in general look good, and clean approach - the shim confined to separate directory, and behind feature flag. Most of the existing contributors doesn't have expertise on Opentracing, and bandwidth to manage this component, so @chusitoo it would be appreciated if you can own any related issues/changes going ahead :)

@lalitb Thanks for the review. Not a problem, I am not an expert in OpenTracing myself but I can support any related issues or requests related to the shim.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ok-to-merge The PR is ok to merge (has two approves or raised by a maintainer/approver and has one approve)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement OpenTracing shim
7 participants