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 all 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
15 changes: 0 additions & 15 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

#
Expand Down Expand Up @@ -88,7 +87,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.sync.test
- name: generate test cert
env:
Expand Down Expand Up @@ -122,7 +120,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.async.test
- name: generate test cert
env:
Expand Down Expand Up @@ -156,7 +153,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.sync.test
- name: generate test cert
env:
Expand Down Expand Up @@ -190,7 +186,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.async.test
- name: generate test cert
env:
Expand Down Expand Up @@ -235,7 +230,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 @@ -455,7 +449,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 @@ -477,7 +470,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 @@ -499,7 +491,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 @@ -521,7 +512,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 @@ -543,7 +533,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 @@ -565,7 +554,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 @@ -587,7 +575,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 @@ -609,7 +596,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 @@ -738,7 +724,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
10 changes: 10 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,19 @@ Increment the:

## [Unreleased]

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

* [API] Remove include_trace_context
[#2194](https://github.com/open-telemetry/opentelemetry-cpp/pull/2194)

Important changes:

* [REMOVAL] Remove the jaeger exporter
[#2031](https://github.com/open-telemetry/opentelemetry-cpp/pull/2031)
* The CMake `WITH_JAEGER` option has been removed
* Please remove usage of `WITH_JAEGER` from user scripts and makefiles.

## [1.9.1] 2023-05-26

* [DEPRECATION] Drop C++11 support
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 @@ -182,8 +182,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 @@ -295,25 +293,6 @@ function(set_target_version target_name)
endif()
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 @@ -405,7 +384,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
60 changes: 22 additions & 38 deletions DEPRECATED.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,9 +93,7 @@ but the C++11 build will no longer be tested for each new release.

## [Third party dependencies]

### Jaeger

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

## [Build tools]

Expand All @@ -107,15 +105,7 @@ N/A

## [opentelemetry-cpp API]

N/A

## [opentelemetry-cpp SDK]

N/A

## [opentelemetry-cpp Exporter]

### Jaeger exporter
### Jaeger propagator

#### Announcement (Jaeger)

Expand All @@ -124,50 +114,44 @@ N/A
* PR: [DEPRECATION] Deprecate the Jaeger exporter
[#1923](https://github.com/open-telemetry/opentelemetry-cpp/pull/1923)

#### Motivation (Jaeger)
This PR also listed the Jaeger propagator as deprecated.

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

The initial Jaeger announcement in release 1.35 reads as:
The Jaeger Exporter is now (July 2023) removed from the OpenTelemetry specification.

"
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.
"
The Jaeger Propagator remains, because changing propagation is a longer
process compared to changing an export format.

At time of writing, Jan 2023, the client libraries have been archived 6
months ago already.
New deployments however are encouraged to use a W3C compliant propagator,
and avoid the Jaeger propagator, which is now deprecated.

#### 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.
Use a W3C compliant propagator instead.

That is, use class HttpTraceContext and "traceparent" tags.

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).
Do not use class JaegerPropagator and "uber-trace-id" tags.

#### Planned removal (Jaeger)

* Date: July, 2023
No date set yet for the Jaeger Propagator.

## [opentelemetry-cpp SDK]

N/A

## [opentelemetry-cpp Exporter]

N/A

## [Documentation]

Expand Down
34 changes: 0 additions & 34 deletions bazel/repository.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,18 +153,6 @@ def opentelemetry_cpp_deps():
],
)

# libthrift (optional)
maybe(
http_archive,
name = "com_github_thrift",
build_file_content = _ALL_CONTENT,
sha256 = "5ae1c4d16452a22eaf9d802ba7489907147c2b316ff38c9758918552fae5132c",
strip_prefix = "thrift-0.14.1",
urls = [
"https://github.com/apache/thrift/archive/refs/tags/v0.14.1.tar.gz",
],
)

# rules foreign cc
maybe(
http_archive,
Expand Down Expand Up @@ -195,25 +183,3 @@ def opentelemetry_cpp_deps():
"https://github.com/opentracing/opentracing-cpp/archive/refs/tags/v1.6.0.tar.gz",
],
)

# boost headers from vcpkg
maybe(
native.new_local_repository,
name = "boost_all_hdrs",
build_file_content = """
package(default_visibility = ["//visibility:public"])
cc_library(
name = "boost_all_hdrs",
hdrs = glob(
["include/**/*.h*"],
),
strip_include_prefix = "include",
copts = [
"-isystem include",
"-fexceptions",
],
visibility = ["//visibility:public"],
)
""",
path = "tools/vcpkg/installed/x64-windows/",
)
1 change: 0 additions & 1 deletion ci/do_ci.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,6 @@ switch ($action) {
cmake $SRC_DIR `
-DOTELCPP_MAINTAINER_MODE=ON `
-DWITH_NO_DEPRECATED_CODE=ON `
-DWITH_JAEGER=OFF `
-DVCPKG_TARGET_TRIPLET=x64-windows `
"-DCMAKE_TOOLCHAIN_FILE=$VCPKG_DIR/scripts/buildsystems/vcpkg.cmake"
$exit = $LASTEXITCODE
Expand Down