Remove generated code from repository about opentelemetry-proto HOT 36 CLOSED

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.

from opentelemetry-proto.

The consistency of a centralized location acting as a source of truth would ensure compatibility across all uses.

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.

from opentelemetry-proto.

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.

from opentelemetry-proto.

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.

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 gen-go.

from opentelemetry-proto.

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 make gen-go, which means:

• I can't use the GitHub online editor.
• I need to set up all the dependencies for the generator even if I'm not interested in it.

A straightforward way to solve this is to make an opentelemetry-proto-go repository which contains the generated code. You could add opentelemetry-proto as as git submodule or use other means to acquire it there. You could move the go-specific build scripts there or leave them here.

from opentelemetry-proto.

I 100% agree on committing generated code. Just in the right place.

from opentelemetry-proto.

FWIW there is no v0.4.0 opentelemetry-proto release matching the latest Collector release.
I created an issue #161 to address that.

Ideally, there should be an OTLP compatibility matrix between SDKs and Collector releases.

from opentelemetry-proto.

@tigrannajaryan What were the reasons to include the generated code (as opposed to just the build files required to generate it) anyway?

from opentelemetry-proto.

@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.

from opentelemetry-proto.

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.

from opentelemetry-proto.

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 😄

from opentelemetry-proto.

from opentelemetry-proto.

Docker is also good option. We use this https://hub.docker.com/r/znly/protoc for our internal builds.

from opentelemetry-proto.

@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 go_package option in .proto files. This is currently set to this repository's URL. The net result is that the generated files cannot be compiled because they end up importing non-existing packages.

What is the right way to do this? I was not able to find a protoc option to override import paths. Is one expected to manually alter the import paths after generation (or go_package option before generation)?

from opentelemetry-proto.

@OPEN 원격 분석 / 사양 승인자는 다른 리포지토리에서 Go ProtoBuf 코드를 올바르게 생성하는 방법을 알고 있습니까?

생성된 Go 파일은 서로 가져오고 가져오기 문은 .proto 파일의 옵션에 지정된 전체 URL을 포함합니다. 이 정보는 현재 이 리포지토리의 URL로 설정되어 있습니다. 결과적으로 생성된 파일은 존재하지 않는 패키지를 가져오기 때문에 컴파일할 수 없습니다.go_package

이 작업을 수행하는 올바른 방법은 무엇입니까? 가져오기 경로를 재정의할 수 있는 옵션을 찾을 수 없습니다. 생성 후 가져오기 경로(또는 생성 전 옵션)를 수동으로 변경해야 합니까?protoc``go_package

from opentelemetry-proto.

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).

from opentelemetry-proto.

Currently opentelemetry-collector doesn't have a dependency on opentelemetry-go, but it would still be nice to generate code only once and commit it so that end users don't have to generate it themselves. If both repos will store generated code independently, it may add confusion for the end users on which one is the source of truth. We will also have to keep generation scripts in two places in sync to make sure they are compatible with each other (i.e. using correct flags in protoc commands etc).

I see two reasonable options:

1. We may move generated code to opentelemetry-go and make opentelemetry-collector dependent on it, unless there is a strong reason against that.
2. Language specific repos opentemetry-proto-go, opentelemetry-proto-java etc will effectively just split this repo into multiple repos, so that might be preferred since it won't change anything for the end users.

from opentelemetry-proto.

Currently opentelemetry-collector doesn't have a dependency on opentelemetry-go, but it would still be nice to generate code only once and commit it so that end users don't have to generate it themselves.

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.

If both repos will store generated code independently, it may add confusion for the end users on which one is the source of truth.

That code is internal to each repo (and can be placed under /internal dir).

from opentelemetry-proto.

Most users will use SDK and collector as is, the generated code is a hidden implementation detail of those.

If it's hidden, that's probably fine. But currently proto-generated types are exposed by collector.

See for example OTLPTraceData https://github.com/open-telemetry/opentelemetry-collector/blob/3115b835d0289b5a7fef8b87db6bb838d57ed48a/consumer/consumerdata/consumerdata.go#L44

Currently, to implement a custom exporter for collector, you deal with TraceData and MetricsData types, which use OpenCensus types. They are going to be replaced with OTLP types generated in this repo, if I understand correctly.

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.

from opentelemetry-proto.

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.

from opentelemetry-proto.

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 opentelemetry-go and opentelemetry-proto-go seem fine to me. Separate opentelemetry-proto-go is probably better for using git submodules and validating that generated types are in sync with protos in CI (see #112).

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

Just keep in mind that it is for package authors, not clients, if only for the reason that the program it invokes might not be available on the target machine. Also, if the containing package is intended for import by go get, once the file is generated (and tested!) it must be checked into the source code repository to be available to clients.

from opentelemetry-proto.

FYI I have successfully set up a separate repository for generating Go files: https://github.com/nilebox/opentelemetry-proto-go-experimental

Minor caveats:

• For code generation I copied proto directory from a submodule to the top level, and deleted it at the end - this is required for relative paths between .proto files to work
• I had to modify go_package in .proto files (via forked repo) to reflect the new GitHub repo path, since it takes priority over all other command line parameters and is now the only recommended way for Go, see golang/protobuf#791 (comment) and golang/protobuf#63 (comment). This to me proves that we should only generate Go files in one place and use it for go_package in this repo.
• Scripts for Travis CI and local generation are slightly different due to difference in path where source code is checked out.
• I added checksum-based script for validating that checked-in Go files are in sync with the proto repo. Previously in #112 I tried using git status but it doesn't work with detached HEAD in Travis CI.

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 opentelemetry-proto-go repo, I can work on the extraction.

NOTE: This will require changing base Go package, so it will affect imports in opentelemetry-go and opentelemetry-collector repos. This should be straightforward once the new repo has all generated Go files in place.

from opentelemetry-proto.

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).

from opentelemetry-proto.

Most users will use SDK and collector as is, the generated code is a hidden implementation detail of those.

If it's hidden, that's probably fine. But currently proto-generated types are exposed by collector.

See for example OTLPTraceData https://github.com/open-telemetry/opentelemetry-collector/blob/3115b835d0289b5a7fef8b87db6bb838d57ed48a/consumer/consumerdata/consumerdata.go#L44

Currently, to implement a custom exporter for collector, you deal with TraceData and MetricsData types, which use OpenCensus types. They are going to be replaced with OTLP types generated in this repo, if I understand correctly.

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.

This is currently work-in-progress, but most likely we will not expose OTLP types in Collector directly. See open-telemetry/opentelemetry-collector#584

from opentelemetry-proto.

We should still keep the CI in this repo that performs code generation to ensure we don't break *.proto files accidentally.

FWIW gen-go is not executed in CI currently:

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 go build in the separate repo, which would guarantee that changes are valid and safe (e.g. imports are correct).

Agree with the rest of the comment about benefits and downsides.

from opentelemetry-proto.

@tigrannajaryan can we do what we discuss and try to generate the protos inside the collector repo?

from opentelemetry-proto.

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.
This is going to be a breaking change though (package renaming).

from opentelemetry-proto.

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?

from opentelemetry-proto.

why separate repo instead of opentelemetry-go?

from opentelemetry-proto.

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?

from opentelemetry-proto.

+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

from opentelemetry-proto.

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.

from opentelemetry-proto.

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.

as @nilebox pointed out:

there is no v0.4.0 opentelemetry-proto release matching the latest Collector release.

This was built from unreleased code. Additionally, the error in the linked ticket is about the concept of Temporality, a message type still under active debate and likely to change.

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.

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.

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.

from opentelemetry-proto.

@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.

from opentelemetry-proto.

provided that we are using the same compilers from the IDL

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.

from opentelemetry-proto.

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 go.opentelemetry.io/otel is cut which explicitly removes the go.mod dependency on the generated code.

from opentelemetry-proto.