-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Description
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
- Delete thrift-gen
- Clean up Makefile.Thrift.mk
- Upgrade OTEL Contrib to also use types from jaeger-idl instead of from Jaeger (after Jaeger's Feb release and after OTEL upgrades to it) Remove dependencies on Jaeger from opentelemetry-collector-contrib #6408