search

open-telemetry / opamp-java

OpAMP protocol implementation in Java
Apache License 2.0
6 stars 6 forks source link

Consume opamp-spec as a submodule and use protos from it #1

Closed tigrannajaryan closed 2 years ago

tigrannajaryan commented 2 years ago

Depends on https://github.com/open-telemetry/opamp-go/issues/133

This is similar to what we do with OTLP protos in https://github.com/open-telemetry/opentelemetry-proto-go

We need a makefile to re-generate the files every time protos are updated. Generated files should be stored in this repo.

Ideally also a github action that verifies that protos match the generated file.

tigrannajaryan commented 2 years ago

@jlegoff can you help with this?

jlegoff commented 2 years ago

@tigrannajaryan yes, I can take it when the protos are moved to opamp-spec

tigrannajaryan commented 2 years ago

Sounds good, assigned to you.

In the meantime if you want to start with https://github.com/open-telemetry/opamp-java/issues/3 it would be great.

tigrannajaryan commented 2 years ago

Protos are in the spec now: https://github.com/open-telemetry/opamp-spec/pull/126

If you need anything to be changed in proto files for Java (e.g. java_ options) please create a PR against the spec repo.

jlegoff commented 2 years ago

I have started to work on this issue here - I'm struggling with a few build issues but I'm getting help from @jack-berg

tigrannajaryan commented 2 years ago

I have started to work on this issue here - I'm struggling with a few build issues but I'm getting help from @jack-berg

Sounds good. Please move incrementally, keep the PRs small so that they are easy to review.

jlegoff commented 2 years ago

Please move incrementally, keep the PRs small so that they are easy to review

I've made a first PR and kept is as small as I could, even though there's a lot of boiler plate code with the gradle scripts 😅

jack-berg commented 2 years ago

Generated files should be stored in this repo.

@tigrannajaryan why store the files in the repo?

We need a makefile to re-generate the files every time protos are updated.

Why use a makefile instead of standard java tooling with gradle?

I think the opentelemetry-proto-java and opentelemetry-java gets this right:

tigrannajaryan commented 2 years ago

@tigrannajaryan why store the files in the repo?

I find it helpful to look at diffs in generated file in PRs. For example if we change generation options diffs are very important to see the effect of the change. If the files are not stored in the repo there is no way to do it. Also if generated files are not stored in the repo ensuring reproducing build is more difficult - you have to ensure you generation process is also reproducible, which is doable but adds one more thing to worry about.

Why use a makefile instead of standard java tooling with gradle?

Ignore that. Use whatever tool makes sense for java.

jack-berg commented 2 years ago

I find it helpful to look at diffs in generated file in PRs. For example if we change generation options diffs are very important to see the effect of the change. If the files are not stored in the repo there is no way to do it.

I don't disagree with that. However, it does break convention with opentelemetry-java. In my experience, the tooling we use to generate proto classes is very reliable. Having modules that depend on the generated classes like the eventual opamp client / server and corresponding test files improves confidence.

I'm neutral overall, given that there will be no change in experience for someone consuming the artifact whether or not the generated classes are checked into source code.

tigrannajaryan commented 2 years ago

If you don't see the need to have the diffs of generated files in PRs then go with whatever you prefer. It should be long-term maintainers' decision and I am only a temporary maintainer here. It is also possible to change this decision later and start storing generated files in the repo, so no big deal, start with what you prefer now.