WIP: Generate go protobuf types/source from .proto files imported as git submodule

[evantorrie]

This PR addresses #793

• Adds a git submodule at internal/opentelemetry-proto tied to a specific tagged release of open-telemetry/opentelemetry-proto.
• Adds Makefile rules to generate protobuf types in internal/opentelemetry-proto-gen from the git submodule sources using docker image namely/protoc-all
• Changes references to previously imported github.com/open-telemetry/opentelemetry-proto packages to now refer to local packages generated during the build at internal/opentelemetry-proto-gen.

Note: I expect that there will be some discussion on the approach taken since this implementation does not check in the generated .pb.go files, but instead generates them fresh on each build from the original source using protoc. This makes use of a docker image which has protoc and go plugin pre-installed, rather than trying to build/install protoc from scratch. As a result, the build now requires docker, even for a make precommit step on the developer's machine.

[lizthegrey]

generates them fresh on each build from the original source using protoc. This makes use of a docker image which has protoc and go plugin pre-installed, rather than trying to build/install protoc from scratch. As a result, the build now requires docker, even for a make precommit step on the developer's machine.

I have some pretty strong feelings against this; requiring a binary blob to build feels like it is adding more dependencies and also potentially makes generated go documentation harder to access.. I'd strongly prefer running a script to update, ensuring that we can diff changes if we change the version of protoc or changing a dep...

[MrAlias]

This approach seems reasonable.

I'm not sure if we should be checking in the generated code or not. Not doing that is definitely going to increase our build times and potentially introduce inconsistent builds. On the other hand, checking in this code, a dependency, that is an entirely separate project seems like a poor choice as well. One that will cause a bloated history (especially since the generated code contains encoded binary data).

[MrAlias]

Suggested change

	- Protobuf sources are now imported as a git submodule from open-telemetry/opentelemetry-proto. (#935)
	Makefile changed to build .go files from .proto sources using docker protoc image.
	- Protobuf sources are now imported as a git submodule from open-telemetry/opentelemetry-proto.
	Makefile changed to build .go files from .proto sources using docker protoc image. (#935)

[MrAlias]

Might want to make the v1 version here a variable as well.

[evantorrie]

Done.

[MrAlias]

How does the upgrade path look? Does it include bumping the submodule and going through all the go.mod files?

Also, might not want to include the version in the comment.

Suggested change

	# replace opentelemetry-proto v0.4.0 package name by repo-local version
	# replace opentelemetry-proto package name by repo-local version

[evantorrie]

With the new PR #938, it will look as follows:

• Bump git submodule ref at internal/opentelemetry-proto to a newer version
• Check in just that change and open the PR.
• The github action from Create protobuf generation GitHub action #938 will then run, creating updated *.pb.go files (if any) in internal/opentelemetry-proto-gen and checking those changes back into the same PR.
• The normal CircleCI build process will run.
• If any changes are required because of the .proto differences, these will need to be resolved by the opener of the PR and will typically be confined to exporters/otlp/.

[MrAlias]

This seems odd. Shouldn't the forced vendoring we are doing here alleviate the dependency on the Go code contained in github.com/open-telemetry/opentelemetry-proto?

[evantorrie]

Yes, this is a result of running make precommit while having the git submodule directory populated.

[MrAlias]

This container needs to be versioned to a tag so when latest is updated it doesn't break things.

E.g.

Suggested change

	docker run $(VOLUMES_MOUNT) namely/protoc-all $(1)
	docker run $(VOLUMES_MOUNT) namely/protoc-all:1.28_2 $(1)

Though, it probably makes sense to keep the version as a variable.

[Aneurysm9]

Perhaps better, pin the image to a digest:

namely/protoc-all@sha256:b3eaf5e5ed4f3d0848ba5f5eda16374eb1d73e1fdbf30b0f13d439d96da5b81e

[MrAlias]

generates them fresh on each build from the original source using protoc. This makes use of a docker image which has protoc and go plugin pre-installed, rather than trying to build/install protoc from scratch. As a result, the build now requires docker, even for a make precommit step on the developer's machine.

I have some pretty strong feelings against this; requiring a binary blob to build feels like it is adding more dependencies and also potentially makes generated go documentation harder to access.. I'd strongly prefer running a script to update, ensuring that we can diff changes if we change the version of protoc or changing a dep...

@lizthegrey what is the binary blob in this situation?

[lizthegrey]

generates them fresh on each build from the original source using protoc. This makes use of a docker image which has protoc and go plugin pre-installed, rather than trying to build/install protoc from scratch. As a result, the build now requires docker, even for a make precommit step on the developer's machine.

I have some pretty strong feelings against this; requiring a binary blob to build feels like it is adding more dependencies and also potentially makes generated go documentation harder to access.. I'd strongly prefer running a script to update, ensuring that we can diff changes if we change the version of protoc or changing a dep...

@lizthegrey what is the binary blob in this situation?

A specific version of the protoc compiler (and docker image that in turn it's packaged in)

[MrAlias]

A specific version of the protoc compiler (and docker image that in turn it's packaged in)

@lizthegrey gotcha.

In your proposal to run a script to update, where is the protoc binary located? Like, is it checked in, or installed on the person who's running the scripts system?

Sorry, this is likely a dumb question, I'm just not seeing the workflow.

[lizthegrey]

A specific version of the protoc compiler (and docker image that in turn it's packaged in)

@lizthegrey gotcha.

In your proposal to run a script to update, where is the protoc binary located? Like, is it checked in, or installed on the person who's running the scripts system?

Sorry, this is likely a dumb question, I'm just not seeing the workflow.

I'm fine with requiring a docker container to perform the update of the generated files, but not for subsequent builds where neither the proto nor protoc has changed. Does that make sense?

[evantorrie]

Understand the comments.. I'm working on an alternative where the updates to *.pb.go files are done as part of a separate workflow and then checked in. Let me experiment for a bit, and leave this as WIP.

[MrAlias]

I'm fine with requiring a docker container to perform the update of the generated files, but not for subsequent builds where neither the proto nor protoc has changed. Does that make sense?

Ah, yeah, that makes things clearer. Thanks.

[evantorrie]

I'll leave this open for a little while longer, but this is going to be closed in favor of #938 and a subsequent PR which will introduce the git submodule.

[evantorrie]

Closed in favor of #938