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 proto-gen/thrift-gen to jaeger-idl #6494

Open
6 tasks
yurishkuro opened this issue Jan 6, 2025 · 3 comments
Open
6 tasks

Migrate proto-gen/thrift-gen to jaeger-idl #6494

yurishkuro opened this issue Jan 6, 2025 · 3 comments
Labels
good first issue Good for beginners help wanted Features that maintainers are willing to accept but do not have cycles to implement

Comments

@yurishkuro
Copy link
Member

yurishkuro commented Jan 6, 2025

In #6408 it was proposed that we break up the circular dependency between Jaeger and OTEL code bases by cloning some of the code from Jaeger to OTEL, in particular the auto-generated classes for Proto and Thrift.

However, it turns out Proto does not work if the same Protobuf type is registered more than once, because Proto uses a global registry. For example, when I used model.proto to generate the output into a different folder proto-gen/model/ (compared to our standard model/) in #6493, then the unit test that tries to use both models fails

panic: proto: duplicate enum registered: jaeger.api_v2.ValueType

goroutine 1 [running]:
github.com/gogo/protobuf/proto.RegisterEnum(...)
        /Users/ysh/golang/pkg/mod/github.com/gogo/protobuf@v1.3.2/proto/properties.go:520
github.com/jaegertracing/jaeger/proto-gen/model.init.0()
        /Users/ysh/dev/jaegertracing/jaeger/proto-gen/model/model.pb.go:713 +0x41c
FAIL    github.com/jaegertracing/jaeger/proto-gen/model 0.452s

As a result, a number of sub-tasks proposed in #6408 will not work if both OTEL and Jaeger have proto-generated types for the same .proto interfaces. This is likely the same issue as reported in open-telemetry/opentelemetry-go-contrib#6555.

One possible workaround is to tweak the IDL files to use a different namespace for the Proto types, e.g. instead of jaeger.api_v2.ValueType the OTEL could use otel.jaeger.api_v2.ValueType. This only works if the generated structs are used to marshal binary payloads like []byte where the namespace of the type does not matter. But if an actual Proto service is implemented then I don't think it can use a different namespace, since then the clients won't be able to use that service.

Since it does not seem like we'd be able to easily avoid OTEL code using some of the Jaeger proto structs, the proposal is to move them out of Jaeger repository. We already include jaeger-idl as a submodule and read the proto files from there to generate code. Instead, we can use jaeger-idl as a regular Go dependency and generate the code in that repository (we already do that today as part of IDL repo CI, but we don't persist that code).

Tasks:

  • reproduce the build steps used in Jaeger to generate proto code in the jaeger-idl repo
  • commit Go code in jaeger-idl repo
  • Upgrade Jaeger to use generated Go code from jaeger-idl as a dependency
    • Delete proto-gen
    • Clean up Makefile.Proto.mk
  • Upgrade OTEL Contrib to also use types from jaeger-idl instead of from Jaeger
@won-js
Copy link
Contributor

won-js commented Jan 8, 2025

I'll give this issue a try, though it looks a bit challenging for me. However, if someone else wants to work on it, feel free to take it

@danish9039
Copy link
Contributor

working on it !

I'll give this issue a try, though it looks a bit challenging for me. However, if someone else wants to work on it, feel free to take it

currently working on this

@adityachopra29
Copy link
Contributor

Seems like it is taking some time. Also trying this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for beginners help wanted Features that maintainers are willing to accept but do not have cycles to implement
Projects
None yet
Development

No branches or pull requests

4 participants