-
Notifications
You must be signed in to change notification settings - Fork 251
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
Generate Go code #74
Generate Go code #74
Conversation
10c6e40
to
4dbae23
Compare
0c71fcc
to
ed5aa6a
Compare
1. Added makefile to generate code for Go and Java. 2. Removed no longer needed java_build.sh, now that makefile contains all that functionality. 3. Run `make gen-go` to generate Go code. This allows generated Go code to be imported and used elsewhere, particularly in OpenTelemetry Collector.
ed5aa6a
to
6290966
Compare
@open-telemetry/specs-approvers please review. |
Please update |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM giving I have limited experience with Go protos. I think it's a good idea to generate Go files right from this repository
I think we decided to go with a different model. Every repo that needs the proto uses got submodules and generates their own code there. What do you think about that? |
The OpenCensus model we used seems to be working fine. I didn't see much downsides to that. In any case if any repo wants to generate their own code they are free to do it. I think generating the code here does not prevent that. Are there any downsides of generating the code here? The benefit I see is that it is much easier to begin using OTLP, we lower the barrier to adoption. |
@bogdandrutu any thoughts on this? This is blocking Collector work that I don't want to delay that because the longer we delay the more components will need to be re-written after we adopt OTLP. |
@bogdandrutu I think Go generation is fine in general. Just to simplify things for now since schema is essential for collector. I would also be against having all other languages here. |
The feedback I got about this was from @yurishkuro who suggested that it is better because this way every repo controls their protoc compiler version etc. I don't want to block and I think it is easy to move if we decide to do that (just a path change in the go files). So I am ok to submit this here for the moment. |
@tigrannajaryan I would probably suggest to use vanity urls for the proto and maybe use something like: |
I'm not a fan of vanity urls, they have been known to have hiccups, even on highly popular domains. @tigrannajaryan why can't this be simply generated in the collector repository? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine to merge to unblock other work, but I think an IDL repo should only contain IDL. Otherwise it becomes multi-language monorepo with all kinds of resulting issues.
I am happy to go with an alternate approach. If we don't want the generated code to be in this IDL repo then where should it live? Is it in the language libraries? Or a completely separate repo just for generated code for a particular language? If we want to do a vanity URL I will need some help with that since I don't know how to publish at go.opentelemetry.io (I assume this needs to serve the meta tag to point to wherever the package is stored).
I think it can but will still require a vanity URL, won't it? Or we simply use |
The idea is that the generated code is just for the repo that generates it. It does not need a public url because it should not be shared. It can be under internal/proto-gen/. |
contains all that functionality.
make gen-go
to generate Go code.This allows generated Go code to be imported and used elsewhere,
particularly in OpenTelemetry Collector.