jaegertracing / jaeger

CNCF Jaeger, a Distributed Tracing Platform
https://www.jaegertracing.io/
Apache License 2.0
20.34k stars 2.43k forks source link

Find a way to ignore gogoproto annotations in the IDL #1213

Closed yurishkuro closed 4 years ago

yurishkuro commented 5 years ago

Requirement - what kind of business use case are you trying to solve?

Use model/model.proto to generate code in other languages.

Problem - what in Jaeger blocks you from solving the requirement?

The model/model.proto currently contains a lot of gogoproto options needed to make the internal domain model types more efficient. While these annotations are optional for other languages, the file does not compile if the compiler does not have access to the gogoproto IDLs.

Proposal - what do you suggest to solve the problem or improve the existing situation?

There are two ways we can go about solving this problem:

  1. Write a script that strips the gogoproto annotations before compiling the IDL in other languages.
  2. Create a reusable Docker image for generating code in all languages, which includes all the dependent IDLs, including gogoproto.
pavolloffay commented 5 years ago

So once gogoproto IDL is included generation to other languages work?

yurishkuro commented 5 years ago

I think so.

zoidyzoidzoid commented 5 years ago

While testing a Docker image to solve https://github.com/jaegertracing/jaeger-idl/issues/55, it definitely seems so.

annanay25 commented 4 years ago

@yurishkuro - Znly has developed a comprehensive docker image for this use case - https://github.com/znly/docker-protobuf#supported-languages. This has been forked into https://github.com/TheThingsIndustries/docker-protobuf and the developer claims to maintain it and update dependencies bi-weekly.

If we're not okay with using this, we could fork it into jaegertracing/docker-protobuf and I could help maintain it.

yurishkuro commented 4 years ago

The second repo looks pretty good, but we may need to fork and maintain our own in order to keep the dependencies versions in sync with the main repo.

annanay25 commented 4 years ago

We could maintain our version of the docker build ... command (which is in build.sh) and continue to use the upstream repository for the Dockerfile. However, I can already think of improvements, like only loading modules in the Dockerfile that we require.

yurishkuro commented 4 years ago

Yes, I think that repo loads too many modules.

Also, the build.sh file seems redundant, in znly repo the package versions were defined directly in the Dockerfile (still parameterized in one place).

I haven't looked into why the other repo also has the go.mod file.

annanay25 commented 4 years ago

Also, the build.sh file seems redundant, in znly repo the package versions were defined directly in the Dockerfile (still parameterized in one place).

I agree. I think the build.sh file was setup to integrate with GitHub Actions, but we could do way with that.

I haven't looked into why the other repo also has the go.mod file.

go.mod is the dependency list for check-versions.go - which seems to query the GitHub API and check for the latest release versions of all repositories (protobuf, grpc-ecosystem, etc..)

annanay25 commented 4 years ago

@yurishkuro - Pinging to check if we can go ahead and fork.

yurishkuro commented 4 years ago

Go ahead

annanay25 commented 4 years ago

Have created https://github.com/jaegertracing/docker-protobuf for this.

Will update the repository with corresponding repository versions in jaeger/master.

pavolloffay commented 4 years ago

@annanay25 can be this closed?

yurishkuro commented 4 years ago

I don’t think so. We need to move the proto files to idl repo and change the build to use the new Docker image.

pavolloffay commented 4 years ago

I thought there was a different ticket for it and this just deal with gogo annotations to which we agreed to put all dependencies into classpath and use https://github.com/jaegertracing/docker-protobuf

annanay25 commented 4 years ago

If someone can take up the migration task https://github.com/jaegertracing/jaeger-idl/issues/55 I can update the build to use the docker image.

yurishkuro commented 4 years ago

I suggest we do the build change first.