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.

Sign up for GitHub

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

Jump to bottom

[chore] Update Jaeger model import paths to use jaeger-idl v1 #37782

Merged
merged 5 commits into from
Feb 10, 2025
Merged

[chore] Update Jaeger model import paths to use jaeger-idl v1 #37782

merged 5 commits into from
Feb 10, 2025

Conversation

Nabil-Salah
Copy link
Contributor

Description

Link to tracking issue

jaegertracing/jaeger#6494

Testing

go test

Documentation

@Nabil-Salah
feat: use jaeger-idl models
effc8df 
Signed-off-by: nabil salah <[email protected]>
@linux-foundation-easycla Linux Foundation: EasyCLA
Copy link

linux-foundation-easycla bot commented Feb 7, 2025 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

@Nabil-Salah Nabil-Salah mentioned this pull request Feb 7, 2025
Closed
@Nabil-Salah
Copy link
Contributor Author

Nabil-Salah commented Feb 7, 2025 •
edited
Loading

@yurishkuro
should i also update proto-gen , thrift-gen in same pr?

@yurishkuro
Copy link
Member

Yes

@github-actions github-actions bot added receiver/zipkin Zipkin receiver testbed labels Feb 7, 2025
@Nabil-Salah
Copy link
Contributor Author

@yurishkuro
can you give workflow approval

@crobert-1
Copy link
Member

Hello @Nabil-Salah, thanks for your contribution! To be able to move this PR forward, please sign the CLA. 👍

@Nabil-Salah Nabil-Salah marked this pull request as ready for review February 7, 2025 23:20
@Nabil-Salah Nabil-Salah requested a review from a team as a code owner February 7, 2025 23:20
@Nabil-Salah
Copy link
Contributor Author

hello @crobert-1 i signed it now :D

@Nabil-Salah Nabil-Salah changed the title Update Jaeger model import paths to use jaeger-idl v1 [chore] Update Jaeger model import paths to use jaeger-idl v1 Feb 7, 2025
@Nabil-Salah
fix: ci make gotidy
f7d6786 
Signed-off-by: nabil salah <[email protected]>
@Nabil-Salah
Copy link
Contributor Author

can you run the workflow once again :D @yurishkuro @crobert-1

@yurishkuro
Copy link
Member

@Nabil-Salah while waiting for the merge, can you analyze what the remaining dependencies on /jaeger/ there are? I've seen while doing a review some remaining direct dependencies and curiously some indirect ones.

@Nabil-Salah
Copy link
Contributor Author

@yurishkuro I came up with this report https://docs.google.com/document/d/11ITBz59augUHL40jzgPKzEULUEPKCUwChRDERt7zQAk/edit?usp=sharing

@yurishkuro
Copy link
Member

I came up with this report

thanks. I left a bunch of comments, seems like it still needs a decent amount of clean-up. Breaking dependency on the agent code would be the largest impact (Jaeger no longer uses agent code, we only kept it because of this dependency).

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

🎉 🎉 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yurishkuro yurishkuro yurishkuro approved these changes

@songy23 songy23 songy23 approved these changes

@Aneurysm9 Aneurysm9 Awaiting requested review from Aneurysm9

@atoulme atoulme Awaiting requested review from atoulme atoulme is a code owner

@dao-jun dao-jun Awaiting requested review from dao-jun

@dmitryax dmitryax Awaiting requested review from dmitryax dmitryax is a code owner

@frzifus frzifus Awaiting requested review from frzifus

@MovieStoreGuy MovieStoreGuy Awaiting requested review from MovieStoreGuy MovieStoreGuy is a code owner

@pavolloffay pavolloffay Awaiting requested review from pavolloffay

@VihasMakwana VihasMakwana Awaiting requested review from VihasMakwana

@yotamloe yotamloe Awaiting requested review from yotamloe

@andrzej-stencel andrzej-stencel Awaiting requested review from andrzej-stencel andrzej-stencel is a code owner

@crobert-1 crobert-1 Awaiting requested review from crobert-1 crobert-1 is a code owner

Assignees

@dmitryax dmitryax

Labels
exporter/awskinesis exporter/kafka exporter/logzio exporter/pulsar exporter/sapm extension/encoding extension/jaegerremotesampling pkg/translator/jaeger pkg/translator/zipkin ready to merge Code review completed; ready to merge by maintainers receiver/jaeger receiver/kafka receiver/pulsar receiver/sapm receiver/zipkin Zipkin receiver testbed
Projects
None yet
Milestone
next release
Development

Successfully merging this pull request may close these issues.
5 participants
@Nabil-Salah @yurishkuro @crobert-1 @songy23 @dmitryax