-
Notifications
You must be signed in to change notification settings - Fork 438
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
Opentracing shim #1909
Conversation
Codecov Report
Additional details and impacted files@@ 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
|
Is it better to move this into a standalone directory? |
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 :) |
opentracing-shim/include/opentelemetry/opentracingshim/propagation.h
Outdated
Show resolved
Hide resolved
The copyright check in CI is new, please fix:
|
There was a problem hiding this 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 <[email protected]>
Date: Sat Feb 4 20:32:33 2023 -0800
upgrade prometheus-cpp to v1.1.0 (#1954)
Please fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand correctly, the Opentracing shim introduces a dependency on the opentracing-cpp repository, which is a deprecated library, per:
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.
There was a problem hiding this 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.
There was a problem hiding this 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 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. |
Fixes #78, answers to #1180
Changes
For significant contributions please make sure you have completed the following items:
CHANGELOG.md
updated for non-trivial changes