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

Remove the Jaeger Exporter #3964

Merged
merged 6 commits into from
Apr 8, 2024
Merged

Remove the Jaeger Exporter #3964

merged 6 commits into from
Apr 8, 2024

Conversation

pellared
Copy link
Member

@pellared pellared commented Mar 26, 2024

Fixes #3980

Follows #3551

Leftover after #3567:

The PR is now revised as follows:

  • the Jaeger Exporter is removed

@pellared
Copy link
Member Author

@marcalff Can you double-check please?

@pellared pellared marked this pull request as ready for review March 26, 2024 10:12
@pellared pellared requested review from a team March 26, 2024 10:12
@marcalff
Copy link
Member

@marcalff Can you double-check please?

I tried to remove file specification/trace/sdk_exporters/jaeger.md before, but Yuri had concerns about preserving the content.

See #3567 (comment)

Please check with @yurishkuro

@pellared
Copy link
Member Author

pellared commented Mar 26, 2024

See #3567 (comment)

I don't think we should delete this, rather it should be moved somewhere collector-related.

  1. AFAIK Collector has also removed this exporter. I guess the comment is outdated. CC @open-telemetry/collector-approvers
  2. Having it as listed as a Tracing SDK exporter feels wrong.

@marcalff
Copy link
Member

See #3567 (comment)

I don't think we should delete this, rather it should be moved somewhere collector-related.

1. AFAIK Collector has also removed this exporter. I guess the comment is outdated. CC @open-telemetry/collector-approvers

2. Having it as listed as a Tracing SDK exporter feels wrong.

To clarify, I am fine with this cleanup, which I support, as the Jaeger exporter is no longer used.

However, I am not in a position to formally approve it, having no voice for specs.

@mx-psi
Copy link
Member

mx-psi commented Mar 26, 2024

In the Collector, we removed the Jaeger exporter on v0.86.0, specifically on open-telemetry/opentelemetry-collector-contrib/pull/26546

Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

So the situation today is that because in the past OTEL needed to transform to Jaeger format we wrote this transformation spec and there is a transformer in collector-contrib matching that spec. Even though the exporters in both SDK and collector may be safely removed now, the transformer is still actively used by Jaeger itself. So the simplest solution is to move the transformer spec to the transformer dir in collector-contrib. Longer term we can consider migrating that code completely to Jaeger, but the transformer is technically a public API in the contrib, we don't know if other people are also using it.

Requesting changes.

@pellared
Copy link
Member Author

pellared commented Apr 2, 2024

@open-telemetry/collector-contrib-maintainer, what do you think of @yurishkuro proposal: #3964 (review)? Are you fine moving the spec to collector-contrib? If so then I think this PR should blocked until the spec is added to collector.

@mx-psi
Copy link
Member

mx-psi commented Apr 2, 2024

Per our CODEOWNERS pkg/translator/jaeger is owned by @open-telemetry/collector-approvers and @frzifus. As part of said group I am in favor of moving the spec to collector-contrib for now, though personally I would like to see collector-contrib eventually be exclusively for components and move this library to some other repository

@yurishkuro
Copy link
Member

I'd be fine with hosting the lib as a standalone repo in the jaeger org. At some point, if we convert the main jaeger repo into multi-module monorepo we could move it there.

@pellared
Copy link
Member Author

pellared commented Apr 3, 2024

@mx-psi I created #3980 for tracking (and marked this PR as blocked).

@pellared pellared changed the title Remove the Jaeger Exporter [Blocked] Remove the Jaeger Exporter Apr 3, 2024
@pellared pellared marked this pull request as draft April 3, 2024 06:54
@pellared
Copy link
Member Author

pellared commented Apr 5, 2024

Ready to review as open-telemetry/opentelemetry-collector-contrib#32187 is already merged.

@pellared pellared marked this pull request as ready for review April 5, 2024 18:41
@pellared pellared changed the title [Blocked] Remove the Jaeger Exporter Remove the Jaeger Exporter Apr 5, 2024
@pellared pellared requested a review from trask April 5, 2024 18:56
@reyang reyang merged commit 584b979 into open-telemetry:main Apr 8, 2024
7 checks passed
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.

pkg/translator/jaeger: Copy the Jeager exporter docs from the specification
8 participants