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

Migrate jaeger model/converter/thrift/zipkin #37796

Conversation

Nabil-Salah
Copy link
Contributor

@Nabil-Salah Nabil-Salah commented Feb 9, 2025

Description

Remove jaeger model/converter/thrift/zipkin dependency

Link to tracking issue

Resolves #37795

Part of jaegertracing/jaeger#6409

Testing

Documentation

@Nabil-Salah
Copy link
Contributor Author

Nabil-Salah commented Feb 9, 2025

hello @yurishkuro
i tried first with this pr of moving model/converter/thrift/zipkin
is this the right location it should be moved to?

// A valid model.Trace is always returned, even when there are errors.
// The errors are more of an "fyi", describing issues in the data.
// TODO consider using different return type instead of `error`.
func ToDomain(zSpans []*zipkincore.Span) (*model.Trace, error) {
Copy link
Member

Choose a reason for hiding this comment

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

once this is merged may want to book another ticket to implement direct translation zipkin->OTLP, there's no reason why this needs to go through the jaeger model, just an extra overhead.

@Nabil-Salah Nabil-Salah marked this pull request as ready for review February 9, 2025 20:36
@Nabil-Salah Nabil-Salah requested a review from a team as a code owner February 9, 2025 20:36
Signed-off-by: nabil salah <[email protected]>
@Nabil-Salah
Copy link
Contributor Author

@yurishkuro these lints will require code editing should I edit the code or is there another thing to do?

@yurishkuro
Copy link
Member

yes, you can edit the code. The linter in this repo may be slightly different. Also, you need to make fmt the code

Signed-off-by: nabil salah <[email protected]>
@Nabil-Salah
Copy link
Contributor Author

PTAL @yurishkuro

@Nabil-Salah
Copy link
Contributor Author

PTAL @yurishkuro
I think it's ready to be merged

@yurishkuro yurishkuro added the ready to merge Code review completed; ready to merge by maintainers label Feb 10, 2025
@andrzej-stencel andrzej-stencel merged commit d597217 into open-telemetry:main Feb 10, 2025
174 checks passed
@github-actions github-actions bot added this to the next release milestone Feb 10, 2025
@yurishkuro
Copy link
Member

@Nabil-Salah looks like you missed one

receiver/zipkinreceiver/trace_receiver_test.go
21:	zipkin2 "github.com/jaegertracing/jaeger/model/converter/thrift/zipkin"

@yurishkuro
Copy link
Member

and another

receiver/jaegerreceiver/jaeger_agent_test.go
17:	jaegerconvert "github.com/jaegertracing/jaeger/model/converter/thrift/jaeger"

@Nabil-Salah
Copy link
Contributor Author

@Nabil-Salah looks like you missed one

oh sorry will open another pr and continue working on this

@yurishkuro
Copy link
Member

please link PRs to jaegertracing/jaeger#6409

@Nabil-Salah
Copy link
Contributor Author

Nabil-Salah commented Feb 10, 2025

@yurishkuro
I think the problem why I skipped those was because these are of different modules so I will need to wait for a release to use the newly added code

image

I can attach to head commit I think If you want so but I don't think this is a good way

@Nabil-Salah
Copy link
Contributor Author

if will wait for the release i think we can go for this

once this is merged may want to book another ticket to implement direct translation zipkin->OTLP, there's no reason why this needs to go through the jaeger model, just an extra overhead.

@yurishkuro
Copy link
Member

you are right, converter/thrift/jaeger is a different module, but converter/thrift/zipkin is the same, is it not?

in either case the goal is to break the circular dependencies completely, so we may need to copy converter/thrift/jaeger as well (I think it will be needed for cmd/agent migration).

@yurishkuro
Copy link
Member

missed

$ rg jaegertracing/jaeger/model
receiver/zipkinreceiver/trace_receiver_test.go
21:	zipkin2 "github.com/jaegertracing/jaeger/model/converter/thrift/zipkin"

@Nabil-Salah
Copy link
Contributor Author

yes, I'll make pr for this now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove jaeger model/converter/thrift/zipkin dependency
4 participants