[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 |
|
|
||
| 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.
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.
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=OFFwill 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 - 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) |
ThomsonTan
left a comment
There was a problem hiding this comment.
Thanks for driving this change, @marcalff
lalitb
left a comment
There was a problem hiding this comment.
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>
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>
sys-cluster/ceph[jaeger] depends on opentelemetry-cpp[jaeger] while opentelemetry-cpp removes jaeger from v1.10.0 in open-telemetry/opentelemetry-cpp#2031 Remove 19.2.0 ebuild which should be replaced by its revision Signed-off-by: Yiyang Wu <xgreenlandforwyy@gmail.com>
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> Signed-off-by: Sam James <sam@gentoo.org>
sys-cluster/ceph[jaeger] depends on opentelemetry-cpp[jaeger] while opentelemetry-cpp removes jaeger from v1.10.0 in open-telemetry/opentelemetry-cpp#2031 Remove 19.2.0 ebuild which should be replaced by its revision Signed-off-by: Yiyang Wu <xgreenlandforwyy@gmail.com> Closes: #38146 Signed-off-by: Sam James <sam@gentoo.org>
Fixes #1938
Changes
For significant contributions please make sure you have completed the following items:
CHANGELOG.mdupdated for non-trivial changes