Remove generated code from repository #79
Comments
|
@tigrannajaryan What were the reasons to include the generated code (as opposed to just the build files required to generate it) anyway? |
|
@Oberon00 So that the code can be consumed by other repos that depend on it. This was the approach used by OpenCensus proto and I went with it. I am not opposed to removing generated code from this repo provided that there is a clear way to consume the proto from dependent projects. |
|
Coming from more of a Java background, I would have expected the build in the repository to publish some artifacts to some external repository instead of committing the generated files to version control. Then other repositories would depend on that by referencing the generated artifact. |
|
Yet another possibility would be to invoke the build script from the client projects. If it's more than a single line of client build script to do so, tune the proto build so that it becomes just a single line 😄 |
|
We could use Docker with a prebuilt protoc for this task.
…On Wed, Dec 11, 2019 at 7:27 AM Christian Neumüller < ***@***.***> wrote:
Yet another possibility would be to invoke the build script from the
client projects. If it's more than a single line of client build script to
do so, tune the proto build so that it becomes just a single line 😄
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#79?email_source=notifications&email_token=AA3WFCP3MBAW23AAOUB2LXTQYEBFBA5CNFSM4JTU6ITKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEGTQZYA#issuecomment-564595936>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA3WFCPINR57C6RFWP4UKJLQYEBFBANCNFSM4JTU6ITA>
.
|
|
Docker is also good option. We use this https://hub.docker.com/r/znly/protoc for our internal builds. |
Go files are stale. They need to be regenerated any time .proto files are changed. This needs to happen until open-telemetry#79 is implemented.
Go files are stale. They need to be regenerated any time .proto files are changed. I executed `make gen-go`. This needs to happen until open-telemetry#79 is implemented.
Go files are stale. They need to be regenerated any time .proto files are changed. I executed `make gen-go`. This needs to happen until #79 is implemented.
|
@open-telemetry/specs-approvers does anybody know how to generate Go ProtoBuf code correctly from a different repo? Generated Go files import each other and import statements include full URL as specified in What is the right way to do this? I was not able to find a |
This comment has been minimized.
This comment has been minimized.
In Go, source code is the artifact, and usually generated code is committed in the "library" repos like this one. If generated code was only used within this repo, then it could have been generated as part of the build. Third parties shouldn't need to generate code they don't own. Also, committing generated code makes it easier to review side effects in diff after running |
|
However, this repository is not only used by Go and there are real, practical drawbacks to having the generated code here, namely that every change requires re-running
A straightforward way to solve this is to make an |
|
Generated code implies framework versions, for different languages, which is clearly not the scope of this IDL repo. It belongs to the repos that are actually using that code, such as collector or SDKs (and they can easily have different framework versions between themselves). |
|
Currently I see two reasonable options:
|
Who are the end users and why do they care about proto-generated code? Most users will use SDK and collector as is, the generated code is a hidden implementation detail of those.
That code is internal to each repo (and can be placed under /internal dir). |
If it's hidden, that's probably fine. But currently proto-generated types are exposed by collector. See for example Currently, to implement a custom exporter for collector, you deal with So if I want to implement an exporter for a custom backend, I will have to generate these types? If the plan is to only use these types internally and not use them for extensibility, that may not be an issue. @tigrannajaryan can probably answer this. |
|
even if they are not hidden, I don't think there's a lot of chance of confusion. If one is writing a custom exporter for collector, they use proto types from collector repo. |
|
I personally don't see any benefits in generating types for the same protos in more than one place from the Go ecosystem perspective. If this repo is not appropriate for storing Go packages, let's decide on the right one. Both This issue is driven by non-Go concerns, and the current state seems to be working fine for both Go SDK and collector. Duplicate scripts do bring extra cost of keeping them in sync and generated files up-to-date though. As a side note, committing generated code is officially recommended for Go: https://blog.golang.org/generate
|
|
I 100% agree on committing generated code. Just in the right place. |
|
FYI I have successfully set up a separate repository for generating Go files: https://github.com/nilebox/opentelemetry-proto-go-experimental Minor caveats:
Overall these are very minor tweaks, so we should be good to go with this option if we agree to move Go files out of this repo. If someone with permissions can create the NOTE: This will require changing base Go package, so it will affect imports in |
|
We can move the generated to code to a separate repo if that's the consensus. From Go files' importer's perspective it makes no difference, which repo to import from and as a person who works on the proto frequently I don't care one way or another since I have the tooling installed anyway. I understand it may prevent others who are not so much involved with the proto work to make contributions if they don't have the tooling installed locally. We should still keep the CI in this repo that performs code generation to ensure we don't break *.proto files accidentally. One downside of moving Go files elsewhere is that any change to proto requires a change here, then in that other repo, then elsewhere that needs to use Go files (e.g. Collector), but it is not a big deal since hopefully it will be rare once we are past the active phase (we already are I think). |
This is currently work-in-progress, but most likely we will not expose OTLP types in Collector directly. See open-telemetry/opentelemetry-collector#584 |
FWIW Line 17 in a65b867 I've tried to change that in #112, but if we plan to extract Go to the new repo, I would move Go-specific CI part there as well. We can also run Agree with the rest of the comment about benefits and downsides. |
|
I think it is useful to merge #112 since it is an improvement over current state. This issue may take a bit time to resolve, no need to wait for resolution. |
|
@tigrannajaryan can we do what we discuss and try to generate the protos inside the collector repo? |
|
The Collector doesn't use generated Go types from this repo anymore: open-telemetry/opentelemetry-collector#1037 So we may consider doing the same in https://github.com/open-telemetry/opentelemetry-go and remove generated types from this repo. |
|
The Go SIG is a fan of the approach outlined in #79 (comment) @open-telemetry/technical-committee can you help setting up a repository for this as described in that comment? |
|
why separate repo instead of opentelemetry-go? |
The consistency of a centralized location acting as a source of truth would ensure compatibility across all uses. This includes avoiding any differences in compilation and versioning. It would also avoid the unnecessary tight coupling of two different projects. As pointed out above, in Go projects the common approach to dependencies is to import the dependency instead of bundling into a single code-base. For what it's worth, we are fine with it staying here as well. It sounds like that is causing a burden to developers wanting to avoid running make prior to a commit though. Has the option to only run the Go generation on a release been explored? |
Proto IDL is the necessary compatibility layer. Different libs can compile IDL into classes all they want, it does not break compatibility. Different libs can also use different code generators (e.g. gogoproto in Go has like 5 different variants). Generated code creates unique versioning issues that are much better handled within projects that depend on that code. I feel like we're bikeshedding a simple decision: since opentelemetry-go needs to generate code from proto, it should just do it in its repo, under /internal/ package so that it's not unintentionally exposed as an API. If some other project/repo also needs to generate code, they can do it again. |
|
+1 to what @yurishkuro said. In .net we simply copy files: https://github.com/open-telemetry/opentelemetry-dotnet/tree/master/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation |
|
Here's and example of the exact type of error I was talking about avoiding: open-telemetry/opentelemetry-go#845 I don't appreciate my point being trivialized by saying we are just bikeshedding. I take this very seriously as it has to do with the stability of the OpenTelemetry project, something I took responsibility for when I assumed the role of a maintainer for the Go project. It seems like this solution is being steamrolled in. I'm at a loss as to how to discuss a solution to a problem when this solution was prescribed from the start and differing opinions are not being listened to. |
|
I don't mean any offense. open-telemetry/opentelemetry-go#845 is a versioning issue where Go SDK and Go Collector are built with different versions of the proto file. It would break no matter what, even if there was a shared repo with generated code, because SDK and collector are on different release schedules and the proto changes were incompatible. The Technical Committee discussed this topic today and our recommendation remains the same: remove gen code from this repo and re-gen it in the repos that need it (NB: this is the approach the Jaeger SDKs used for several years). @bogdandrutu volunteered to create a Docker image (similar to https://github.com/jaegertracing/docker-protobuf, a wrapper around protoc, with the necessary plugins) that can be used for generating code in different languages. |
|
FWIW there is no Ideally, there should be an OTLP compatibility matrix between SDKs and Collector releases. |
as @nilebox pointed out:
This was built from unreleased code. Additionally, the error in the linked ticket is about the concept of This is something @bogdandrutu assured people wouldn't be an issue when it was decided to split up #137 as it would all be gated by a release of this centralized and universal source of truth.
Understood. I'm disappointing this form of decision making was used and other opinions were not a part of the conversation (especially representation from one of the known repositories this is going to effect), but understand that alternatives are no longer being explored. |
|
@MrAlias BTW, related to your comment please see here #161 (comment) I agree with you that we need to be stricter and more careful with the releases and protocol changes. I am not sure it necessarily means the protocol needs to be generated in one place. I think @yurishkuro is right that IDL (the ProtoBuf definitions) is equaly strong guarantee of compatibility, provided that we are using the same compilers from the IDL (or compilers that generate fully compatible code from the same IDL - like canonical Protoc and Gogoproto compilers do. Perhaps I am missing something, but I think this allows you to achieve the goals that you stated. |
To play devil's advocate, even that is not a requirement - if someone so desires then can write completely custom serialization of protobuf and not use code-gen at all (I believe Zipkin's native SDK Brave does exactly that and avoids any framework dependencies). The compatibility is in the protocol, which is defined by the IDL files. Code-gen is how one may use it, but there's nothing canonical about generated code. E.g. gogoproto supports like 5 different code-gen flavors, all resulting in different classes but still compatible on the wire. |
|
When merged, open-telemetry/opentelemetry-go#942 will allow this issue to be resolved via a removal of the generated code. However, it may be wise to wait until a new release of |
The generated go code should not be checked in into the repository. See also #77 (comment)
Usually you should generate the files only during the build and maybe publish an artifact with them but never check them into source control (the same way you don't check in binary executables). I see #74 stated reasons to generate the code in this repository, and I think having the build files here is fine. But having the generated code here seems wrong.
The text was updated successfully, but these errors were encountered: