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

Remove dependency on deprecated github.com/golang/protobuf #4911

Closed
mmorel-35 opened this issue Oct 31, 2023 · 7 comments · Fixed by #5060
Closed

Remove dependency on deprecated github.com/golang/protobuf #4911

mmorel-35 opened this issue Oct 31, 2023 · 7 comments · Fixed by #5060
Labels
enhancement good first issue Good for beginners help wanted Features that maintainers are willing to accept but do not have cycles to implement

Comments

@mmorel-35
Copy link
Contributor

Requirement

Use only one library to handle protobuf

Problem

There are three dependencies to handle protobuf in jaeger:

  • github.com/gogo/protobuf : DEPRECATED
  • github.com/golang/protobuf superseded by google.golang.org/protobuf
  • google.golang.org/protobuf

Proposal

Migrate to google.golang.org/protobuf

Open questions

No response

@yurishkuro
Copy link
Member

github.com/golang/protobuf superseded by google.golang.org/protobuf

yes, this one we should clean up, we probably just need to regenerate types using newer version.

$ grep -r github.com/golang/protobuf .
./go.mod:26:	github.com/golang/protobuf v1.5.3
./proto-gen/api_v3/query_service.pb.gw.go:16:	"github.com/golang/protobuf/descriptor"
./proto-gen/api_v3/query_service.pb.gw.go:17:	"github.com/golang/protobuf/proto"
./plugin/storage/grpc/proto/storageprototest/storage_test.pb.go:24:	proto "github.com/golang/protobuf/proto"
./model/prototest/model_test.pb.go:24:	proto "github.com/golang/protobuf/proto"

We don't have any concrete plan to move away from gogo, as it provides a significant efficiency boost over default official go-protobuf. OTEL's data model also using it - if they find a solution we could move in sync.

@yurishkuro yurishkuro added help wanted Features that maintainers are willing to accept but do not have cycles to implement good first issue Good for beginners labels Oct 31, 2023
@akagami-harsh
Copy link
Member

akagami-harsh commented Nov 1, 2023

hi @yurishkuro, i want to work on this issue. So basically we have to migrate from using the github.com/golang/protobuf dependence to using google.golang.org/protobuf for handling Protocol Buffers.

@yurishkuro
Copy link
Member

No we're not changing gogo, only the other one

yurishkuro added a commit that referenced this issue Nov 8, 2023
#4917)

## Which problem is this PR solving?
- Part of #4911 

## Description of the changes
- using `google.golang.org/protobuf` in place of
`github.com/golang/protobuf`
- `github.com/golang/protobuf` is not completely removed it is still
being used as an indirect dependency

## How was this change tested?
- `make test`

## Checklist
- [x] I have read
https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
- [x] I have signed all commits
- [ ] I have added unit tests for the new functionality
- [x] I have run lint and test steps successfully
  - for `jaeger`: `make lint test`
  - for `jaeger-ui`: `yarn lint` and `yarn test`

---------

Signed-off-by: Harshvir Potpose <[email protected]>
Co-authored-by: Yuri Shkuro <[email protected]>
@EshaanAgg
Copy link
Contributor

Hi! If this issue is unassigned and no one is working on it, I would love to take this up. I have some experience working with protobuf in Go, but am completely new to JaegerUI. Would appreciate any help or links to documentation about the same!

@yurishkuro
Copy link
Member

yurishkuro commented Nov 9, 2023

Status update - the above 3 PRs managed to remove github.com/golang/protobuf from the main code paths, but it still remains on the path that uses grpc-gateway. Related discussion in #4917. To resolve that, we need:

@yurishkuro yurishkuro changed the title [Feature]: use only one library for protobuf ? Remove dependency on deprecated github.com/golang/protobuf Nov 9, 2023
yurishkuro added a commit that referenced this issue Dec 30, 2023
## Which problem is this PR solving?
- Part of #5052
- Continues #5054
- Closes #4911

## Description of the changes
- Replace grpc-gateway based implementation with manual HTTP
implementation from #5054
- Clean up spurious grpc-gateway usage (e.g. all-in-one test that did
not need it)
- Delete grpc-gateway step from `make proto` and remove the
corresponding generated file
- `go mod tidy` removes grpc-gateway and github.com/golang/protobuf

## How was this change tested?
- CI

---------

Signed-off-by: Yuri Shkuro <[email protected]>
@xdobiasl
Copy link

It seems that this is not fixed, dependency to github.com/golang/protobuf is still there in jaeger 1.55.0 linux packages:

xdobiasl@ldobias-cz:jaeger-1.55.0-linux-amd64$ for i in *;do echo $i:; (go version -m $i |grep protobuf);done
example-hotrod:
        dep     github.com/gogo/protobuf        v1.3.2  h1:Ov1cvc58UF3b5XjBnZv7+opcTcQFZebYjWzi34vdm4Q=
        dep     github.com/golang/protobuf      v1.5.3  h1:KhyjKVUg7Usr/dYsdSqoFveMYd5ko72D+zANwlG1mmg=
        dep     google.golang.org/protobuf      v1.32.0 h1:pPC6BG5ex8PDFnkbrGU3EixyhKcQ2aDuBS36lqK/C7I=
jaeger-agent:
        dep     github.com/gogo/protobuf        v1.3.2  h1:Ov1cvc58UF3b5XjBnZv7+opcTcQFZebYjWzi34vdm4Q=
        dep     github.com/golang/protobuf      v1.5.3  h1:KhyjKVUg7Usr/dYsdSqoFveMYd5ko72D+zANwlG1mmg=
        dep     google.golang.org/protobuf      v1.32.0 h1:pPC6BG5ex8PDFnkbrGU3EixyhKcQ2aDuBS36lqK/C7I=
jaeger-all-in-one:
        dep     github.com/gogo/protobuf        v1.3.2  h1:Ov1cvc58UF3b5XjBnZv7+opcTcQFZebYjWzi34vdm4Q=
        dep     github.com/golang/protobuf      v1.5.3  h1:KhyjKVUg7Usr/dYsdSqoFveMYd5ko72D+zANwlG1mmg=
        dep     google.golang.org/protobuf      v1.32.0 h1:pPC6BG5ex8PDFnkbrGU3EixyhKcQ2aDuBS36lqK/C7I=
jaeger-collector:
        dep     github.com/gogo/protobuf        v1.3.2  h1:Ov1cvc58UF3b5XjBnZv7+opcTcQFZebYjWzi34vdm4Q=
        dep     github.com/golang/protobuf      v1.5.3  h1:KhyjKVUg7Usr/dYsdSqoFveMYd5ko72D+zANwlG1mmg=
        dep     google.golang.org/protobuf      v1.32.0 h1:pPC6BG5ex8PDFnkbrGU3EixyhKcQ2aDuBS36lqK/C7I=
jaeger-ingester:
        dep     github.com/gogo/protobuf        v1.3.2  h1:Ov1cvc58UF3b5XjBnZv7+opcTcQFZebYjWzi34vdm4Q=
        dep     github.com/golang/protobuf      v1.5.3  h1:KhyjKVUg7Usr/dYsdSqoFveMYd5ko72D+zANwlG1mmg=
        dep     google.golang.org/protobuf      v1.32.0 h1:pPC6BG5ex8PDFnkbrGU3EixyhKcQ2aDuBS36lqK/C7I=
jaeger-query:
        dep     github.com/gogo/protobuf        v1.3.2  h1:Ov1cvc58UF3b5XjBnZv7+opcTcQFZebYjWzi34vdm4Q=
        dep     github.com/golang/protobuf      v1.5.3  h1:KhyjKVUg7Usr/dYsdSqoFveMYd5ko72D+zANwlG1mmg=
        dep     google.golang.org/protobuf      v1.32.0 h1:pPC6BG5ex8PDFnkbrGU3EixyhKcQ2aDuBS36lqK/C7I=

@yurishkuro
Copy link
Member

The objective of this ticket was to remove direct dependencies, which was achieved:

$ grep protobuf go.mod
	github.com/gogo/protobuf v1.3.2
	google.golang.org/protobuf v1.33.0
	github.com/golang/protobuf v1.5.3 // indirect

We may not be able to remove indirect dependencies, especially if we're already on the latest versions of libraries that pull that indirect dependency.

We need to understand what is pulling github.com/golang/protobuf - if it's gogo, then there's nothing we can do without migrating off gogo (which we cannot now, because OTEL uses it).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement good first issue Good for beginners help wanted Features that maintainers are willing to accept but do not have cycles to implement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants