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

Deprecate jaeger exporters #5190

Merged
merged 3 commits into from
Jul 1, 2023

Conversation

jack-berg
Copy link
Member

Resolves #5096.

@jack-berg jack-berg requested a review from a team February 9, 2023 22:47
@codecov
Copy link

codecov bot commented Feb 9, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.24 ⚠️

Comparison is base (6ab37f1) 91.35% compared to head (4ea53d1) 91.11%.

❗ Current head 4ea53d1 differs from pull request most recent head ef1de6e. Consider uploading reports for the commit ef1de6e to get more accurate results

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #5190      +/-   ##
============================================
- Coverage     91.35%   91.11%   -0.24%     
+ Complexity     4969     4886      -83     
============================================
  Files           552      549       -3     
  Lines         14581    14420     -161     
  Branches       1358     1380      +22     
============================================
- Hits          13320    13139     -181     
- Misses          872      891      +19     
- Partials        389      390       +1     
Impacted Files Coverage Δ
...porter/jaeger/thrift/JaegerThriftSpanExporter.java 81.13% <ø> (ø)
...jaeger/thrift/JaegerThriftSpanExporterBuilder.java 84.61% <ø> (ø)
...emetry/exporter/jaeger/JaegerGrpcSpanExporter.java 87.09% <ø> (ø)
...exporter/jaeger/JaegerGrpcSpanExporterBuilder.java 80.64% <ø> (-1.18%) ⬇️
...aeger/internal/JaegerGrpcSpanExporterProvider.java 100.00% <100.00%> (ø)

... and 166 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Member

@mateuszrzeszutek mateuszrzeszutek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉 🎉 🎉

@jkwatson
Copy link
Contributor

There's no reason these wouldn't keep working in the future, correct? We'll make sure our exporter APIs are stable, so these should keep working, if all is well.

@jack-berg
Copy link
Member Author

There's no reason these wouldn't keep working in the future, correct?

Are you asking if after deprecating and eventually remove these, folks would continue to be able to use old versions of the jaeger exporters with new versions of the SDK?

If so, its actually not as good of a story as I would like. The opentelemetry-exporter-jaeger depends on internal APIs in opentelemetry-exporter-common. If those APIs were to change, the exporter would break.

The opentelemetry-exporter-jaeger-thift does NOT make use of opentelemetry-exporter-common so is relatively safe. Although it delegates the work to the deprecated io.jaegertracing:jaeger-client client library, which presents its own problems.

README.md Outdated Show resolved Hide resolved
@jkwatson
Copy link
Contributor

There's no reason these wouldn't keep working in the future, correct?

Are you asking if after deprecating and eventually remove these, folks would continue to be able to use old versions of the jaeger exporters with new versions of the SDK?

If so, its actually not as good of a story as I would like. The opentelemetry-exporter-jaeger depends on internal APIs in opentelemetry-exporter-common. If those APIs were to change, the exporter would break.

If users won't be able to use the exporter, then we definitely shouldn't include the older version in the bom still, since that would imply (I think) that it should work.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 2, 2023

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Mar 2, 2023
@github-actions
Copy link
Contributor

Closed as inactive. Feel free to reopen if this PR is still being worked on.

@github-actions github-actions bot closed this Mar 16, 2023
@jack-berg jack-berg reopened this Jun 28, 2023
@jack-berg
Copy link
Member Author

Re-opening this PR now that the jaeger exporter has been removed from the specification (open-telemetry/opentelemetry-specification#3567).

I've adjusted deprecation policy to keep security bugfixes until 1.34.0 (6 months), and no changes (security or otherwise) after. This reflects the policy of the native jaeger clients, and we should need to hold ourselves to a stricter standard.

Copy link
Contributor

@breedx-splk breedx-splk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is cool, and pretty generous on the timeline. As long as we think the maintenance should remain minimal, this approach should be fine.

The ongoing risk is some future CVE in the jaeger-clients dependency tree in the next 6 months. I think the mitigation is to either fork the deprecated repo and temporarily provide an internal fix, or (if I remember) build the protobufs ourselves. Hopefully neither of those will be necessary. 🤞🏻

@jack-berg
Copy link
Member Author

The ongoing risk is some future CVE in the jaeger-clients dependency tree in the next 6 months.

Luckily its only opentelemetry-exporter-jaeger-thrift which relies on the jaeger-clients, and the usage of thrift is significantly lower than protobuf:

Screenshot 2023-06-30 at 10 59 21 AM Screenshot 2023-06-30 at 10 59 31 AM

@jack-berg jack-berg merged commit 749d316 into open-telemetry:main Jul 1, 2023
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deprecate jaeger exporters
4 participants