-
Notifications
You must be signed in to change notification settings - Fork 417
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
[REMOVAL] Remove the jaeger exporter #2031
[REMOVAL] Remove the jaeger exporter #2031
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #2031 +/- ##
==========================================
- Coverage 87.53% 87.51% -0.02%
==========================================
Files 169 169
Lines 4889 4889
==========================================
- Hits 4279 4278 -1
- Misses 610 611 +1 |
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 PR :)
@@ -175,8 +175,6 @@ option(WITH_ELASTICSEARCH | |||
|
|||
option(WITH_ZPAGES "Whether to include the Zpages Server in the SDK" OFF) | |||
|
|||
option(WITH_JAEGER "DEPRECATED - Whether to include the Jaeger exporter" OFF) |
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.
Could we keep the option but only allow the default value? I mean if we delete the option, the existing cmake invocation like cmake -DWITH_JAEGER=OFF
will have warning triggered. The same applies to cmake -DWITH_JAEGER=ON
. Better to show an explicit error message the user tries to enable Jaeger?
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.
Could we keep the option but only allow the default value? I mean if we delete the option, the existing cmake invocation like
cmake -DWITH_JAEGER=OFF
will have warning triggered. The same applies tocmake -DWITH_JAEGER=ON
. Better to show an explicit error message the user tries to enable Jaeger?
Thanks for the early comments.
The initial deprecation was announced with release 1.8.2 published on 2023-01-31.
It was advertised at great length, including with pinned issues, so users should be well aware of this by now.
With this removal, the following commands:
cmake -DWITH_JAEGER=ON
cmake -DWITH_JAEGER=OFF
will both produce a warning, with CMake indicating that an unknown option is used:
CMake Warning:
Manually-specified variables were not used by the project:
WITH_JAEGER
Using -DWITH_JAEGER=OFF
produces a warning only, and will not cause the build to fail.
The fact that the WITH_JAEGER
option is no longer available is now documented in the CHANGELOG, in the important changes section, for the next release.
I don't think the CMakeList.txt should still contain logic about obsolete options, as it makes removing options impossible, this is mitigated by the important changes in the CHANGELOG instead.
Thanks for the PR. Changes look good. Was trying to see if the specs says anything about removing support of Jaeger . There are couple of reference -
And changelog - https://github.com/open-telemetry/opentelemetry-specification/blob/b674b49a9be32bf38a58865e9a04803c0f9349fb/CHANGELOG.md?plain=1#LL213C3-L213C70
Will it make sense to merge this PR when the Jaeger exporter reference is removed from specs ? |
Looks like I missed the date from the spec when writing the C++ deprecation PR, which is why the planned removal date for C++ was set to April, 2023 instead. Ok to wait for July 2023 then. |
Now ready for review. |
TODO, waiting on spec changes. The Jaeger Propagator will not be removed from the spec: so it needs to be preserved in opentelemetry-cpp as well. |
do not merge yet, minor rework on Jaeger Propagator needed. Will clear the do not merge flag once resolved. |
Jaeger Propagator is no longer removed, per spec. Updated DEPRECATED.md to indicate the propagator stays deprecated in opentelemetry-cpp (not removed) |
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.
Thanks for driving this change, @marcalff
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. Nicely done, as usual :)
IUSE=jager and dev-libs/thrift dependency is removed by open-telemetry/opentelemetry-cpp#2031 Add remote-id to metadata Bug: https://bugs.gentoo.org/900707 Signed-off-by: Yiyang Wu <xgreenlandforwyy@gmail.com>
opentelemetry-cpp removes jaeger from v1.10.0 in open-telemetry/opentelemetry-cpp#2031 Signed-off-by: Yiyang Wu <xgreenlandforwyy@gmail.com>
Fixes #1938
Changes
For significant contributions please make sure you have completed the following items:
CHANGELOG.md
updated for non-trivial changes