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.

Sign up for GitHub

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

Jump to bottom

[REMOVAL] Remove the jaeger exporter #2031

Merged
merged 21 commits into from
Jul 2, 2023
Merged

[REMOVAL] Remove the jaeger exporter #2031

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
77d26a8
Fix #1938
marcalff Mar 8, 2023
e923728
CHANGELOG
marcalff Mar 8, 2023
c8ddd35
removed thrift from CI
marcalff Mar 8, 2023
45612eb
Merge branch 'main' into remove_jaeger_exporter_1938
ThomsonTan Mar 9, 2023
c091ec5
Merge branch 'main' into remove_jaeger_exporter_1938
esigo Mar 9, 2023
ff15b15
Merge branch 'main' into remove_jaeger_exporter_1938
marcalff Mar 27, 2023
6a5064c
Merge branch 'main' into remove_jaeger_exporter_1938
marcalff May 22, 2023
21e392b
Fixed submodule merge
marcalff May 22, 2023
519db9a
Merge branch 'main' into remove_jaeger_exporter_1938
marcalff Jun 14, 2023
ba0cd95
Removed boost
marcalff Jun 14, 2023
808ef36
Removed uses of thrift and boost.
marcalff Jun 14, 2023
125365e
Adjusted CHANGELOG
marcalff Jun 14, 2023
a13ad67
Clarify important changes in CHANGELOG.
marcalff Jun 14, 2023
4ec586d
Merge branch 'main' into remove_jaeger_exporter_1938
marcalff Jun 20, 2023
e9ae354
Merge branch 'main' into remove_jaeger_exporter_1938
marcalff Jun 20, 2023
69ebfd3
Restore Jaeger Propagator
marcalff Jun 27, 2023
2ab7c53
Restored jaeger_propagation_test.cc
marcalff Jun 27, 2023
bc8e9d9
Updated DEPRECATED.md, for the Jaeger Propagator.
marcalff Jun 27, 2023
b01e93e
Merge branch 'main' into remove_jaeger_exporter_1938
marcalff Jun 28, 2023
66ec119
Merge branch 'main' into remove_jaeger_exporter_1938
marcalff Jun 29, 2023
db7396f
Merge branch 'main' into remove_jaeger_exporter_1938
marcalff Jun 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 0 additions & 13 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ jobs:
CC: /usr/bin/gcc-10
CXX: /usr/bin/g++-10
run: |
sudo -E ./ci/setup_thrift.sh
./ci/do_ci.sh cmake.test

cmake_gcc_maintainer_test:
Expand All @@ -51,7 +50,6 @@ jobs:
CC: /usr/bin/gcc-12
CXX: /usr/bin/g++-12
run: |
sudo -E ./ci/setup_thrift.sh
./ci/do_ci.sh cmake.maintainer.test

cmake_clang_maintainer_test:
Expand All @@ -76,7 +74,6 @@ jobs:
CC: /usr/bin/clang-14
CXX: /usr/bin/clang++-14
run: |
sudo -E ./ci/setup_thrift.sh
./ci/do_ci.sh cmake.maintainer.test

cmake_msvc_maintainer_test:
Expand Down Expand Up @@ -112,7 +109,6 @@ jobs:
CC: /usr/bin/gcc-10
CXX: /usr/bin/g++-10
run: |
sudo -E ./ci/setup_thrift.sh
./ci/do_ci.sh cmake.with_async_export.test

cmake_absel_stl_test:
Expand Down Expand Up @@ -316,7 +312,6 @@ jobs:
key: bazel_test
- name: setup
run: |
sudo ./ci/setup_thrift.sh dependencies_only
sudo ./ci/setup_ci_environment.sh
sudo ./ci/install_bazelisk.sh
- name: run tests
Expand All @@ -338,7 +333,6 @@ jobs:
key: bazel_test
- name: setup
run: |
sudo ./ci/setup_thrift.sh dependencies_only
sudo ./ci/setup_ci_environment.sh
sudo ./ci/install_bazelisk.sh
- name: run tests
Expand All @@ -360,7 +354,6 @@ jobs:
key: bazel_test
- name: setup
run: |
sudo ./ci/setup_thrift.sh dependencies_only
sudo ./ci/setup_ci_environment.sh
sudo ./ci/install_bazelisk.sh
- name: run tests
Expand All @@ -382,7 +375,6 @@ jobs:
key: bazel_valgrind
- name: setup
run: |
sudo ./ci/setup_thrift.sh dependencies_only
sudo ./ci/setup_ci_environment.sh
sudo ./ci/install_bazelisk.sh
- name: run tests
Expand All @@ -404,7 +396,6 @@ jobs:
key: bazel_noexcept
- name: setup
run: |
sudo ./ci/setup_thrift.sh dependencies_only
sudo ./ci/setup_ci_environment.sh
sudo ./ci/install_bazelisk.sh
- name: run tests
Expand All @@ -426,7 +417,6 @@ jobs:
key: bazel_nortti
- name: setup
run: |
sudo ./ci/setup_thrift.sh dependencies_only
sudo ./ci/setup_ci_environment.sh
sudo ./ci/install_bazelisk.sh
- name: run tests
Expand All @@ -448,7 +438,6 @@ jobs:
key: bazel_asan
- name: setup
run: |
sudo ./ci/setup_thrift.sh dependencies_only
sudo ./ci/setup_ci_environment.sh
sudo ./ci/install_bazelisk.sh
- name: run tests
Expand All @@ -470,7 +459,6 @@ jobs:
key: bazel_tsan
- name: setup
run: |
sudo ./ci/setup_thrift.sh dependencies_only
sudo ./ci/setup_ci_environment.sh
sudo ./ci/install_bazelisk.sh
- name: run tests
Expand Down Expand Up @@ -599,7 +587,6 @@ jobs:
submodules: 'recursive'
- name: setup
run: |
./ci/setup_thrift.ps1
./ci/install_windows_bazelisk.ps1
- name: run tests
run: ./ci/do_ci.ps1 bazel.build
Expand Down
8 changes: 8 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,14 @@ Increment the:

## [Unreleased]

* [REMOVAL] Remove the jeager exporter
[#2031](https://github.com/open-telemetry/opentelemetry-cpp/pull/2031)

Important changes:

* [REMOVAL] Remove the jeager exporter
[#2031](https://github.com/open-telemetry/opentelemetry-cpp/pull/2031)

## [1.8.3] 2023-03-06

* Provide version major/minor/patch macros
Expand Down
22 changes: 0 additions & 22 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor

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?

Copy link
Member Author

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?

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.


option(WITH_NO_GETENV "Whether the platform supports environment variables" OFF)

option(BUILD_TESTING "Whether to enable tests" ON)
Expand Down Expand Up @@ -251,25 +249,6 @@ function(install_windows_deps)
PARENT_SCOPE)
endfunction()

if(WITH_JAEGER)
if(WITH_NO_DEPRECATED_CODE)
message(FATAL_ERROR "Jaeger is DEPRECATED.")
else()
message(WARNING "Jaeger is DEPRECATED.")
endif()

find_package(Thrift QUIET)
if(Thrift_FOUND)
find_package(Boost REQUIRED)
include_directories(${Boost_INCLUDE_DIR})
else()
# Install Thrift and propagate via vcpkg toolchain file
if(WIN32 AND (NOT DEFINED CMAKE_TOOLCHAIN_FILE))
install_windows_deps()
endif()
endif()
endif()

if(MSVC)
# Options for Visual C++ compiler: /Zc:__cplusplus - report an updated value
# for recent C++ language standards. Without this option MSVC returns the
Expand Down Expand Up @@ -368,7 +347,6 @@ endif()

if(WITH_OTLP_HTTP
OR WITH_ELASTICSEARCH
OR WITH_JAEGER
OR WITH_ZIPKIN
OR BUILD_W3CTRACECONTEXT_TEST
OR WITH_EXAMPLES_HTTP)
Expand Down
58 changes: 2 additions & 56 deletions DEPRECATED.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,7 @@ N/A

## [Third party dependencies]

### Jaeger

See [Jaeger exporter](#jaeger-exporter)
N/A

## [Build tools]

Expand All @@ -56,59 +54,7 @@ N/A

## [opentelemetry-cpp Exporter]

### Jaeger exporter

#### Announcement (Jaeger)

* Version: 1.8.2
* Date: 2023-01-31
* PR: [DEPRECATION] Deprecate the Jaeger exporter
[#1923](https://github.com/open-telemetry/opentelemetry-cpp/pull/1923)

#### Motivation (Jaeger)

The Jaeger client libraries are deprecated, as announced
[here](https://www.jaegertracing.io/docs/1.41/client-libraries/).

The initial Jaeger announcement in release 1.35 reads as:

"
We plan to continue accepting pull requests and making new releases of
Jaeger clients through the end of 2021. In January 2022 we will enter a code
freeze period for 6 months, during which time we will no longer accept pull
requests with new features, with the exception of security-related fixes.
After that we will archive the client library repositories and will no
longer accept new changes.
"

At time of writing, Jan 2023, the client libraries have been archived 6
months ago already.

#### Scope (Jaeger)

The following are deprecated and planned for removal:

* the API header `opentelemetry/trace/propagation/jaeger.h`, including:
* the C++ class `JaegerPropagator`
* all the code located under `exporters/jaeger/`, including:
* the jaeger exporter C++ class (`JaegerExporter`)
* the related factory (`JaegerExporterFactory`)
* the related options (`JaegerExporterOptions`)
* the jaeger exporter library(`opentelemetry_exporter_jaeger_trace`)
* the jaeger build options in CMake (`WITH_JAEGER`)
* the dependency on thrift

#### Mitigation (Jaeger)

Jaeger supports natively the OTLP protocol, starting with jaeger 1.35.

An application instrumented with opentelemetry needs to change how the SDK
and exporter are configured to replace the Jaeger exporter with the OTLP
exporter (both OTLP HTTP and OTLP GRPC are supported).

#### Planned removal (Jaeger)

* Date: After April 1st, 2023
N/A

## [Documentation]

Expand Down
124 changes: 0 additions & 124 deletions api/include/opentelemetry/trace/propagation/jaeger.h
View file
Open in desktop

This file was deleted.

17 changes: 0 additions & 17 deletions api/test/trace/propagation/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,20 +36,3 @@ cc_test(
"@com_google_googletest//:gtest_main",
],
)

cc_test(
name = "jaeger_propagation_test",
srcs = [
"jaeger_propagation_test.cc",
"util.h",
],
tags = [
"api",
"test",
"trace",
],
deps = [
"//api",
"@com_google_googletest//:gtest_main",
],
)
13 changes: 0 additions & 13 deletions api/test/trace/propagation/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,16 +10,3 @@ foreach(testname http_text_format_test b3_propagation_test)
TEST_PREFIX trace.
TEST_LIST ${testname})
endforeach()

if(NOT WITH_NO_DEPRECATED_CODE)
foreach(testname jaeger_propagation_test)

add_executable(${testname} "${testname}.cc")
target_link_libraries(${testname} ${GTEST_BOTH_LIBRARIES}
${CMAKE_THREAD_LIBS_INIT} opentelemetry_api)
gtest_add_tests(
TARGET ${testname}
TEST_PREFIX trace.
TEST_LIST ${testname})
endforeach()
endif()