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

feat: update protobuf image version #502

Closed
wants to merge 1 commit into from

Conversation

swartz-k
Copy link
Contributor

@swartz-k swartz-k commented Feb 3, 2021

What this PR does:
Update protobuf image and opentelemetry collector version
Which issue(s) this PR fixes:
Fixes #479

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@mdisibio
Copy link
Contributor

mdisibio commented Feb 3, 2021

Hi, thanks for looking into this. We have looked into upgrading the open-telemetry-collector dependency as well and it looks like this is running into the same hurdles. make vendor-dependencies fails with:

 github.com/grafana/tempo/modules/distributor/receiver imports
	go.opentelemetry.io/collector/receiver/opencensusreceiver imports
	github.com/soheilhy/cmux tested by
	github.com/soheilhy/cmux.test imports
	google.golang.org/grpc/examples/helloworld/helloworld: ambiguous import: found package google.golang.org/grpc/examples/helloworld/helloworld in multiple modules:
	google.golang.org/grpc v1.33.2 (/home/runner/go/pkg/mod/google.golang.org/grpc@v1.29.1/examples/helloworld/helloworld)
	google.golang.org/grpc/examples v0.0.0-20210129004707-0bc741730b81 (/home/runner/go/pkg/mod/google.golang.org/grpc/examples@v0.0.0-20210129004707-0bc741730b81/helloworld/helloworld)
Makefile:149: recipe for target 'vendor-dependencies' failed

The root cause is that a cmux test relies on grpc/examples, which conflicts with the grpc version required by cortex/etc. One possible way to address this is to vendor cmux and delete the conflicting test.

Were you able to build and run this change locally? If so, I would be interested to learn how that worked, since we ran into the same hurdles.

@swartz-k swartz-k force-pushed the feat/update_protobuf branch 5 times, most recently from 982d3b7 to 0fae2d8 Compare February 22, 2021 04:31
@swartz-k
Copy link
Contributor Author

swartz-k commented Feb 22, 2021

Were you able to build and run this change locally? If so, I would be interested to learn how that worked, since we ran into the same hurdles.

Sorry for the late. I tested it locally by make vendor-check and make tempo but it's different with CI. I am working on it.
image

I think maybe I need to add a make docker-vendor-check in Makefile to test CI in locally.

@swartz-k swartz-k force-pushed the feat/update_protobuf branch 5 times, most recently from 69081b1 to 67958a6 Compare February 22, 2021 05:54
@swartz-k
Copy link
Contributor Author

I think maybe I need to add a make docker-vendor-check in Makefile to test CI in locally.

In docker I got the same error. This problem can be solved by rm -rf /go/pkg/mod/google.golang.org/grpc@v1.29.1/examples/helloworld/helloworld
image

@joe-elliott
Copy link
Member

We have talked and @mdisibio is pursuing a more holistic method of fix this. It will be a combination of fixing our vendor mangling, upgrade us to 1.16 and add direct support for OTEL proto into the distributors instead of relying on the shims.

Thank you for the research @swartz-k, but I think it's best to let him carry this one forward.

@swartz-k
Copy link
Contributor Author

You are welcome. Thanks.

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.

Upgrade to the latest protobuf docker image
3 participants