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 converter thrift jaeger #37823

Merged

Conversation

Nabil-Salah
Copy link
Contributor

Description

Remove jaeger model/converter/thrift/jaeger dependency

Link to tracking issue

Resolves #37820

Part of jaegertracing/jaeger#6409

Testing

Documentation

@Nabil-Salah
Copy link
Contributor Author

PTAL @yurishkuro

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.

I don't see the new functions used anywhere.

@Nabil-Salah
Copy link
Contributor Author

Nabil-Salah commented Feb 11, 2025

I don't see the new functions used anywhere.

@yurishkuro
It'll be used in receiver/jaegerreceiver/jaeger_agent_test.go but after the release ( currently can't as they are of different modules )
and also when migrating the cmd/agent

as we planned here: https://docs.google.com/document/d/11ITBz59augUHL40jzgPKzEULUEPKCUwChRDERt7zQAk/edit?usp=drivesdk

@yurishkuro
Copy link
Member

Sorry I don't follow - which release are you blocked on?

@Nabil-Salah
Copy link
Contributor Author

Nabil-Salah commented Feb 11, 2025

@yurishkuro
currently github.com/open-telemetry/opentelemetry-collector-contrib/ v0.119.0

which doesn't has the migrated jaeger, zipkin converter code so i can't use it now in any place outside of there modules

as opentelemetry-collector-contrib repo made of multiple modules all of different github base link

Screenshot_20250211_050949_GitHub.jpg

Screenshot_20250211_051106_GitHub.jpg

@yurishkuro
Copy link
Member

Whole contrib was just upgraded to latest jaeger 1.66, so you don't have dependencies from that side. And internal dependencies in contrib are all correlated, modules don't depend on different versions of other modules, they use local overrides.

@Nabil-Salah
Copy link
Contributor Author

Nabil-Salah commented Feb 13, 2025

PTAL @yurishkuro
I'm back, I started using the imported library the problem was that I couldn't use modules made under the internal folder outside its module

image

so now in the prev pr #37796 I'll need to make a new pr to move the migrated module outside of internal to use it

@Nabil-Salah Nabil-Salah marked this pull request as ready for review February 13, 2025 20:37
@Nabil-Salah Nabil-Salah requested a review from a team as a code owner February 13, 2025 20:37
@@ -14,7 +14,6 @@ import (
"github.com/jaegertracing/jaeger-idl/thrift-gen/agent"
jaegerthrift "github.com/jaegertracing/jaeger-idl/thrift-gen/jaeger"
"github.com/jaegertracing/jaeger/cmd/agent/app/servers/thriftudp"
jaegerconvert "github.com/jaegertracing/jaeger/model/converter/thrift/jaeger"
Copy link
Member

Choose a reason for hiding this comment

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

is this literally the only file in contrib that depends on this Jaeger module? I don't think it makes sense to migrate in this case, we can probably just refactor the test to not have the dependency.

Copy link
Member

Choose a reason for hiding this comment

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

e.g. this is the only place I see it being used

jexp.EmitBatch(context.Background(), modelToThrift(batch))

i.e. it needs to construct thrift payload to send to the server. We can just load one of the JSON thrift fixtures and parse it in Thrift model.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i can do this
but it will also be used when migrating cmd/agent
#37796

Copy link
Contributor Author

@Nabil-Salah Nabil-Salah Feb 13, 2025

Choose a reason for hiding this comment

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

so what way you suggest going with?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see what you mean. Let's proceed here then.

@yurishkuro yurishkuro added the ready to merge Code review completed; ready to merge by maintainers label Feb 14, 2025
@djaglowski djaglowski merged commit 54063ce into open-telemetry:main Feb 14, 2025
162 checks passed
@github-actions github-actions bot added this to the next release milestone Feb 14, 2025
@Nabil-Salah Nabil-Salah deleted the migrating_converter_thrift_jaeger branch February 14, 2025 17:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg/translator/jaeger ready to merge Code review completed; ready to merge by maintainers receiver/jaeger
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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