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

Move off of gogo/protobuf to e.g. vtproto #11908

Open
jiacai2050 opened this issue Jan 30, 2023 · 11 comments
Open

Move off of gogo/protobuf to e.g. vtproto #11908

jiacai2050 opened this issue Jan 30, 2023 · 11 comments

Comments

@jiacai2050
Copy link

Proposal

In current implementation, protobuf files used by remote storage API import gogo.proto

import "gogoproto/gogo.proto";

import "gogoproto/gogo.proto";

But https://github.com/gogo/protobuf is already deprecated, and we only use nullable options, which I think is not a must.

Also other languages besides Go can use those protobuf files easier if we remove it.

@jiacai2050
Copy link
Author

I have written a Rust library to wrap Prometheus remote API by copying protos in prompb, and remove gogo.proto by hand, it would be simpler if this issue is fixed.

@bboreham
Copy link
Member

Hi, sorry this has gone without comment.

It’s a reasonable objective to make the definitions more portable.

However I would not want to see a noticeable performance cost. Google’s proto3 has added a number of fields to generated structs, and in Prometheus remote write every label and sample is a struct.

@bboreham
Copy link
Member

At #10371 it is suggested to use the Buf registry; would that help your case?

@bwplotka
Copy link
Member

bwplotka commented Feb 5, 2024

This is still a valid request, given gogo is deprecated.

See the detailed discussion here: https://groups.google.com/g/prometheus-developers/c/uFWRyqZaQis/m/1OOGT7s5AwAJ

We consider this to be part of GSoC mentorship project, if anyone would be interest to mentor or be mentored.

@bwplotka bwplotka changed the title Remove gogo.proto from protobuf files used by remote storage API Move out of gogo/protobuf to e.g. vtproto Feb 5, 2024
@HikaruSadashi
Copy link

Hey everyone, I'm a third year software engineering student from Toronto, Canada. I initially heard from a friend that was part of the Kubernetes shadow program and I came over GSOC when looking for a reason to contribute my first time! I used Promethues before so I am familiar with it just on a very surface level. This issue feels like a good fit and I think it'll be very fun doing optimization and hopefully I can contribute and learn a thing or two.

Currently, I'm just reading over https://contribute.cncf.io/contributors/getting-started/ and looking through the Promethues codebase to get started (If there are any other resources I'd appreciate the guidance!). I am also currently working on my GSOC proposal and had a question.

Was there any idea of what we want the benchmarks to be other than memory utilization for for remote write and scraping? In other words, is there anything else tested like throughput, latency? From my initial look-over it looks like there is a test or two involving memory here https://github.com/prometheus/prometheus/tree/main/scrape. Sorry if this is lazy question, I am trying to find current benchmarks cause it mentions they are the standard.

@cstyan @macxamin @bwplotka @TheSpiritXIII (Also, is pinging okay, it seems to depend on the issue so I'm not quite sure)

@cstyan
Copy link
Member

cstyan commented Feb 23, 2024

Thanks for your interest @HikaruSadashi. Just be aware that this migration, off of gogoproto and onto something else like csproto or vtproto, is definitely not as easy as it sounds. It will involve quite a bit of optimization work using structures specific to those pacakges or just go in general sync.Pool that a) can be hard to use correctly and b) might be hard to wrap your head around if you don't already have a lot of go/protobuf experience.

As for the benchmarks, the most relevant would be in the remote storage package. There's some in the queue_manager tests:

queue_manager_test.go
1210:func BenchmarkSampleSend(b *testing.B) {
1247:func BenchmarkStoreSeries(b *testing.B) {
1298:func BenchmarkStartup(b *testing.B) {
1766:func BenchmarkBuildWriteRequest(b *testing.B) {
1813:func BenchmarkBuildMinimizedWriteRequest(b *testing.B) {

There's also prometheus/scripts/remotewrite11-bench which well help run multiple versions of prometheus/with different config options + metrics collection so those can easily be compared.

@Clement-Jean
Copy link

I'm currently doing an experiment with the official Protobuf compiler and Go library. If you are interested you can find it here. I don't really get how to compare the benchmarks just yet but I expect this to be a little bit worse because of cache misses (more pointer following than previously).

@beorn7
Copy link
Member

beorn7 commented Mar 6, 2024

Unless something in the official compiler and library has changed fundamentally in the last years, we can be pretty sure that the results will be prohibitively expensive.

Note that a bunch of people are tackling this issue right now. I saw recent activities by @mircodz @Clement-Jean @HikaruSadashi (on the dev mailing list or here in this issue). Maybe you should all talk to each other and see what can be done. Maybe a new channel #prometheus-protobuf-dev on the CNCF Slack would be a good collection point for interested contributors?

@Clement-Jean
Copy link

@beorn7 @HikaruSadashi @mircodz I created the channel, you guys can join.

@cstyan
Copy link
Member

cstyan commented Mar 19, 2024

Moving off of gogoproto is listed as a GSOC project: https://github.com/cncf/mentoring/blob/main/programs/summerofcode/2024.md#move-out-of-gogoprotobuf-to-protobuf-with-vtproto-implementation

@cstyan cstyan changed the title Move out of gogo/protobuf to e.g. vtproto Move off of gogo/protobuf to e.g. vtproto Mar 20, 2024
@HikaruSadashi
Copy link

Sorry for the late reply I just finished finals

@cstyan Yeah I am expecting to have to dig deep with vtproto or csproto. I have protobuf experience but not directly with Go and so I've been playing with both packages and sync.Pool in general so I'm more familiar. Also thanks for the guidance / pointing me to the benchmarks I have been looking over those too.

Also thanks for creating the channel @Clement-Jean

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

No branches or pull requests

7 participants