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.

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

Merged
merged 21 commits into from Jul 2, 2023

Conversation

marcalff
Copy link
Member

@marcalff marcalff commented Mar 8, 2023

Fixes #1938

Changes

  • Remove the jaeger exporter
  • Remove usage of thrift
  • Remove usage of boost

For significant contributions please make sure you have completed the following items:

  • CHANGELOG.md updated for non-trivial changes
  • Unit tests have been added
  • Changes in public API reviewed

@codecov
Copy link

codecov bot commented Mar 8, 2023

Codecov Report

Merging #2031 (db7396f) into main (049ab63) will decrease coverage by 0.02%.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@            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     

see 1 file with indirect coverage changes

@marcalff marcalff marked this pull request as ready for review March 8, 2023 22:01
@marcalff marcalff requested a review from a team as a code owner March 8, 2023 22:01
Copy link
Member

@esigo esigo left a 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 :)

@esigo esigo added the ok-to-merge The PR is ok to merge (has two approves or raised by a maintainer/approver and has one approve) label Mar 9, 2023
@@ -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.

@lalitb
Copy link
Member

lalitb commented Mar 10, 2023

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 -

https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/sdk-environment-variables.md#jaeger-exporter

Jaeger exporter support will be removed from OpenTelemetry in July 2023.

And changelog - https://github.com/open-telemetry/opentelemetry-specification/blob/b674b49a9be32bf38a58865e9a04803c0f9349fb/CHANGELOG.md?plain=1#LL213C3-L213C70

- Deprecate jaeger exporter, scheduled for spec removal in July 2023.

Will it make sense to merge this PR when the Jaeger exporter reference is removed from specs ?

@marcalff
Copy link
Member Author

- Deprecate jaeger exporter, scheduled for spec removal in July 2023.

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.

@marcalff marcalff changed the title [REMOVAL] Remove the jeager exporter [ON HOLD UNTIL JULY 2023] [REMOVAL] Remove the jeager exporter Mar 10, 2023
@marcalff marcalff marked this pull request as draft March 10, 2023 09:22
@lalitb lalitb removed the ok-to-merge The PR is ok to merge (has two approves or raised by a maintainer/approver and has one approve) label Mar 14, 2023
@marcalff marcalff marked this pull request as ready for review June 14, 2023 19:13
@marcalff marcalff changed the title [ON HOLD UNTIL JULY 2023] [REMOVAL] Remove the jeager exporter [REMOVAL] Remove the jaeger exporter Jun 14, 2023
@marcalff
Copy link
Member Author

Now ready for review.

@marcalff
Copy link
Member Author

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.

@marcalff marcalff added the pr:do-not-merge This PR is not ready to be merged. label Jun 27, 2023
@marcalff
Copy link
Member Author

do not merge yet, minor rework on Jaeger Propagator needed.

Will clear the do not merge flag once resolved.

@marcalff
Copy link
Member Author

Jaeger Propagator is no longer removed, per spec.

Updated DEPRECATED.md to indicate the propagator stays deprecated in opentelemetry-cpp (not removed)

@marcalff marcalff removed the pr:do-not-merge This PR is not ready to be merged. label Jun 27, 2023
Copy link
Contributor

@ThomsonTan ThomsonTan left a 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

Copy link
Member

@lalitb lalitb left a 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 :)

@marcalff marcalff merged commit 42709f4 into open-telemetry:main Jul 2, 2023
43 checks passed
@marcalff marcalff deleted the remove_jaeger_exporter_1938 branch September 5, 2023 15:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[REMOVAL] Remove the Jaeger Exporter
4 participants