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

Upgrade proto to v1.4.0 #193

Merged
merged 8 commits into from
Dec 2, 2024
Merged

Conversation

florianl
Copy link
Contributor

Release of the v1.4.0 version of the OTLP.

@florianl florianl requested a review from a team as a code owner November 21, 2024 20:09
Release of the [v1.4.0][otlp] version of the OTLP.

[otlp]: https://github.com/open-telemetry/opentelemetry-proto/releases/tag/v1.4.0

Signed-off-by: Florian Lehner <[email protected]>
@pellared
Copy link
Member

I have a feeling that a newer version of Go is needed because of go-grpc dependency:

See #194 (comment)

@florianl
Copy link
Contributor Author

I have a feeling that a newer version of Go is needed because of go-grpc dependency:

@pellared this proposed change does not touch dependencies. There is no change in go.mod or go.sum. Can you provide more details, why the Go version should be bumped here?

@pellared
Copy link
Member

I think it is a bug that should be fixed before the release.

@florianl
Copy link
Contributor Author

I'm not sure and can't follow how this is related to this PR. From checking the PR again, I didn't identify an element, that would require a Go version bump.
Is there something in the current code base, that would require a Go version bump, was missed and is unrelated to this PR?

@pellared
Copy link
Member

pellared commented Nov 26, 2024

I'm not sure and can't follow how this is related to this PR. From checking the PR again, I didn't identify an element, that would require a Go version bump. Is there something in the current code base, that would require a Go version bump, was missed and is unrelated to this PR?

I created a bug:

README.md Outdated Show resolved Hide resolved
go.mod was updated with ad54459

Signed-off-by: Florian Lehner <[email protected]>
@florianl
Copy link
Contributor Author

The following error sounds like a circular dependency issue resulting from multiple go.mod files:

go: finding module for package go.opentelemetry.io/proto/slim/otlp/profiles/v1development
go: go.opentelemetry.io/proto/gen/go/go.opentelemetry.io/proto/slim/otlp/collector/profiles/v1development imports
	go.opentelemetry.io/proto/slim/otlp/profiles/v1development: module go.opentelemetry.io/proto/slim/otlp@latest found (v1.3.1), but does not contain package go.opentelemetry.io/proto/slim/otlp/profiles/v1development
make: *** [Makefile:169: go-mod-tidy/.] Error 1

@dmathieu
Copy link
Member

From the CI output, it looks like go-mod-tidy still doesn't remove the gen folder, which is weird as it works fine both on my mac and on a debian VM.

@pellared
Copy link
Member

pellared commented Nov 28, 2024

You should not generate slim/otlp/collector/profiles/v1development. slim modules should not have collector directory which brings the grpc dependency.

I wonder how was it even generated...

@florianl
Copy link
Contributor Author

You should not generate slim/otlp/collector/profiles/v1development. slim modules should not have collector directory which brings the grpc dependency.

I wonder how was it even generated...

Can you provide more background on this? the Makefile target gen-otlp-protobuf-slim does not differentiate between collector and other parts.

.PHONY: gen-otlp-protobuf-slim
gen-otlp-protobuf-slim: $(SOURCE_PROTOSLIM_FILES)
rm -rf ./$(PROTOBUF_TEMP_DIR)
mkdir -p ./$(PROTOBUF_TEMP_DIR)
$(foreach file,$(SOURCE_PROTOSLIM_FILES),$(call exec-command,$(PROTOC_SLIM) $(PROTO_INCLUDES) --go_out=./$(PROTOBUF_TEMP_DIR) $(file)))

While in the target gen-otlp-protobuf for every signal also a collector part is generated:
https://github.com/open-telemetry/opentelemetry-proto-go/blob/f83a33f5490808194d466cc3ff18c6e66a48a407/Makefile#L118C9-L126

florianl added a commit to florianl/opentelemetry-proto-go that referenced this pull request Nov 28, 2024
Release of the [v1.4.0][otlp] version of the OTLP.

Verify open-telemetry#193

[otlp]: https://github.com/open-telemetry/opentelemetry-proto/releases/tag/v1.4.0

Signed-off-by: Florian Lehner <[email protected]>
@florianl
Copy link
Contributor Author

florianl commented Nov 28, 2024

I can reproduce the issue (see https://github.com/florianl/opentelemetry-proto-go/tree/release-v1.4.0-try-2) with a clean state on main and following the release process:

# update Makefile and repalce experimental with development
make sync VERSION=v1.4.0
# update versions.yaml from 1.3.1 to 1.4.0
make verify-versions
git diff

It is in all cases the combination of make clean protobuf go-mod-tidy ( as executed in CI), that breaks something.

@pellared
Copy link
Member

pellared commented Nov 28, 2024

Can you provide more background on this?

See: #155

Maybe I did some mistake(s). I am not a make expert.

Copy link
Member

@pellared pellared left a comment

Choose a reason for hiding this comment

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

@florianl, @dmathieu, good work 👍

@dmathieu dmathieu changed the title Release v1.4.0 Upgrade proto to v1.4.0 Nov 28, 2024
Copy link
Member

@MikeGoldsmith MikeGoldsmith left a comment

Choose a reason for hiding this comment

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

Thanks @florianl

@MikeGoldsmith MikeGoldsmith merged commit 158176a into open-telemetry:main Dec 2, 2024
5 checks passed
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.

4 participants