Replace model imports with jaeger-idl

[Nabil-Salah]

Which problem is this PR solving?

• part of Migrate proto-gen/thrift-gen to jaeger-idl #6494

Description of the changes

• replace model imports to use jaeger-idl model imports

How was this change tested?

• make test

Checklist

• I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
• I have signed all commits
• I have added unit tests for the new functionality
• I have run lint and test steps successfully
  • for jaeger: make lint test
  • for jaeger-ui: npm run lint and npm run test

[yurishkuro]

ah, crap

error: at least one *_test.go file must be in all directories with go files so that they are counted for code coverage
       if no tests are possible for a package (e.g. it only defines types), create empty_test.go

don't think this can be merged on its own, need to make full migration to jaeger-idl as part of the same PR.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.09%. Comparing base (5fa0297) to head (db40dcf).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6595      +/-   ##
==========================================
- Coverage   96.11%   96.09%   -0.03%     
==========================================
  Files         366      366              
  Lines       20949    20949              
==========================================
- Hits        20136    20131       -5     
- Misses        618      622       +4     
- Partials      195      196       +1     

Flag	Coverage Δ
badger_v1	9.57% <16.66%> (ø)
badger_v2	1.75% <16.66%> (ø)
cassandra-4.x-v1-manual	15.29% <16.66%> (ø)
cassandra-4.x-v2-auto	1.74% <16.66%> (ø)
cassandra-4.x-v2-manual	1.74% <16.66%> (ø)
cassandra-5.x-v1-manual	15.29% <16.66%> (ø)
cassandra-5.x-v2-auto	1.74% <16.66%> (ø)
cassandra-5.x-v2-manual	1.74% <16.66%> (ø)
elasticsearch-6.x-v1	19.42% <16.66%> (ø)
elasticsearch-7.x-v1	19.50% <16.66%> (ø)
elasticsearch-8.x-v1	19.67% <16.66%> (ø)
elasticsearch-8.x-v2	1.75% <16.66%> (ø)
grpc_v1	11.02% <16.66%> (ø)
grpc_v2	7.77% <16.66%> (ø)
kafka-3.x-v1	9.86% <16.66%> (ø)
kafka-3.x-v2	1.75% <16.66%> (ø)
memory_v2	1.75% <16.66%> (ø)
opensearch-1.x-v1	19.55% <16.66%> (ø)
opensearch-2.x-v1	19.55% <16.66%> (ø)
opensearch-2.x-v2	1.75% <16.66%> (ø)
tailsampling-processor	0.47% <0.00%> (ø)
unittests	94.92% <100.00%> (-0.03%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Nabil-Salah]

@yurishkuro
after i tried the full-migration it didn't work because OTEL jaegerreceiver uses jaeger/proto-gen/api_v2 which now uses jaeger-idl

[yurishkuro]

can we copy jaegerreceiver into cmd/jaeger/internal/receiver/jaegerreceiver?

Basically we need to break the circular dependency loop, I don't see another way other than copying. We will probably also need to copy kafka receiver / producer.

Zipkin receiver should be upgraded in OTEL upstream to de-couple. Worse case copy it too.

[yurishkuro]

Suggestion - if you have a proof of concept that copying breaks the dependency hell, then I would suggest putting a separate PR with the copy of those modules as is, and then a separate PR (this one) with swapping the models across the code base.

[Nabil-Salah]

okay i'll try copying with keeping module swap and look if it compiles and runs well and give you screenshots will use make lint test this can give proof of concept

and after that if it works I'll make the second pr and then come back here as you stated

[Nabil-Salah]

@yurishkuro
it looks promising it compiles well and tests work

make lint

make test

I only copied jaegerreceiver

I think I can start now with a copy of those modules

[yurishkuro]

ok, but why is CI all broken? Do the binaries actually run, or are we getting that duplicate registration error?

[yurishkuro]

maybe you didn't push, but I would expect to see a change in cmd/jaeger/internal/components where we swap OTEL components for the local copies.

[Nabil-Salah]

yes this what happened
i changed cmd/jaeger/internal/components yes

for ci it test will fail because of duplicate registration error but it fixes when I manually update model.pb.go but I didn't push the edit I think it has to be fixed from the .proto file

[Nabil-Salah]

@yurishkuro
Do you think I should continue with making another pr for the OTEL modules?
to copy jaegerreceiver and translator

[yurishkuro]

Do you think I should continue with making another pr for the OTEL modules?

I think I prefer to continue with this PR first by adding more copied modules until we get to a green build.

I don't think you should be changing model.pb.go. However, there is a possible trick we could use if we get stuck - to delete the generated code and replace it with aliases to the corresponding jaeger-idl types. That's a completely alternative approach that we can try, it may not even require copying OTEL modules, since it will indirectly repoint everything.

[yurishkuro]

That's a completely alternative approach that we can try, it may not even require copying OTEL modules, since it will indirectly repoint everything.

Maybe it's worth giving it a try in another PR before you spend more time on copying and renaming. Aliases could be a much easier path.

[yurishkuro]

Do you mean try making aliase from jaeger models to jaeger-idl models in all jaeger model files without any import replacement?

Yes. Basically hollow-out model.pb.go and proto-gen/api_v2/*.go to only contain aliases to types / interfaces (also delete extensions to model, since we moved them to idl), and see if everything else just works.

[Nabil-Salah]

okay will give it a try

[yurishkuro]

we don't need to copy any modules now, just perform the imports replacement

[Nabil-Salah]

okay will do so

[Nabil-Salah]

@yurishkuro make sure to look at this

• thrift-gen got updated by make thrift
• scripts/lint/import-order-cleanup.py

also I think now we can Upgrade OTEL as next step :D

[yurishkuro]

Why do we need this?

[Nabil-Salah]

@yurishkuro
to use model/otelids.go because in this file we need both imports

[yurishkuro]

🎉 woohoo

[Nabil-Salah]

that's awesome 😄