Skip to content
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

Import open-telemetry/opentelemetry-proto submodule and generate protobuf bindings locally #942

Merged
merged 4 commits into from
Jul 16, 2020

Conversation

evantorrie
Copy link
Contributor

@evantorrie evantorrie commented Jul 16, 2020

Fixes #793 and will allow opentelemetry-proto repository maintainers to remove the generated Go code from their repo.

  1. OTLP .proto sources added as git submodule at internal/opentelemetry-proto.
  2. Generated protobuf files committed to internal/opentelemetry-proto-gen.
  3. References to Go module github.com/open-telemetry/opentelemetry-proto now changed to reference internal package go.opentelemetry.io/otel/internal/opentelemetry-proto-gen.

In normal workflow, the git submodule should not be populated. Only do so with git submodule update --init when a new version of the protobuf sources are released in the opentelemetry-proto repository and you need to force the workflow to regenerate updated Go bindings in internal/opentelemetry-proto-gen.

@evantorrie evantorrie changed the title Import open-telemetry/opentelemetry-proto submodule under internal Import open-telemetry/opentelemetry-proto submodule and generate protobuf bindings locally Jul 16, 2020
Copy link
Member

@lizthegrey lizthegrey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's otel-collector doing with this same problem?

example/otel-collector/go.sum Show resolved Hide resolved
@MrAlias
Copy link
Contributor

MrAlias commented Jul 16, 2020

@lizthegrey for backstory: open-telemetry/opentelemetry-proto#79 (comment)

and from that the collector solution: open-telemetry/opentelemetry-collector#1037
it is just a manually run command

@MrAlias MrAlias merged commit 166c703 into master Jul 16, 2020
@MrAlias MrAlias deleted the protobuf-import branch July 16, 2020 20:59
@lizthegrey
Copy link
Member

@lizthegrey for backstory: open-telemetry/opentelemetry-proto#79 (comment)

and from that the collector solution: open-telemetry/opentelemetry-collector#1037
it is just a manually run command

Ah, excellent, so a similar solution of checking the generated .pb.go files in subject to normal code review, but not run automatically from circleci and instead run by hand. We should consider upstreaming the Github Action so that they get the benefit of it too!

@yurishkuro
Copy link
Member

🎉 🎉 🎉

@yurishkuro
Copy link
Member

Recommendation: in Jaeger we have a separate CI step "check-proto" that re-runs the .pb.go file generation and verifies that checked-in version is the same as would-be-generated one. It catches issues when something in the toolchain changes, like the version of generator plugin.

@jmacd
Copy link
Contributor

jmacd commented Jul 17, 2020

Thanks @evantorrie

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider generating Go types for OTLP in this repo
5 participants