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.

Sign up for GitHub

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

Jump to bottom

Consider moving to google.golang.org/protobuf/proto #317

Closed
mem opened this issue Jul 15, 2021 · 5 comments
Closed

Consider moving to google.golang.org/protobuf/proto #317

mem opened this issue Jul 15, 2021 · 5 comments

Comments

@mem
Copy link
Contributor

mem commented Jul 15, 2021

github.com/golang/protobuf/proto has been "deprecated" in favor of google.golang.org/protobuf/proto. For this codebase in particular this involves changing calls to proto.MarshalTextString to prototext.Marshal.

This is related to #281.
@beorn7
Copy link
Member

beorn7 commented Dec 1, 2022

My assumption so far is that the move would mean a breaking change for the users of prometheus/client_golang (which exposes a few protobuf specific parts in its interface). If we can pull it off without a breaking change, great. But if it does mean a breaking change, it has to be navigated carefully and in coordination with @kakkoyun and @bwplotka .

@bwplotka bwplotka mentioned this issue Dec 2, 2022
Open
@bwplotka
Copy link
Member

bwplotka commented Dec 2, 2022

Ack, I think we have to do it at some point. Added ticket on our side: prometheus/client_golang#1175

zhsj added a commit to zhsj/prometheus-common that referenced this issue Dec 22, 2022
@zhsj
Replace deprecated github.com/golang/protobuf package
9a16683 
This replaces usage of proto.{Float64,Int32,Int64,String,Uint32,Uint64},
which doesn't break the interface.

And replaces ptypes.TimestampProto by google.golang.org/protobuf/types/known/timestamppb.
This works since github.com/golang/protobuf has been bumped to v1.5.2,
which makes ptypes.TimestampProto an alias to timestamppb.Timestamp.
This is only used test (openmetrics_create_test.go), which won't break
interfaces.

Updates: prometheus#317

Signed-off-by: Shengjing Zhu <zhsj@debian.org>
@zhsj zhsj mentioned this issue Dec 22, 2022
Closed
zhsj added a commit to zhsj/prometheus-common that referenced this issue Dec 22, 2022
@zhsj
Replace deprecated github.com/golang/protobuf package
99f1ad9 
This replaces usage of proto.{Float64,Int32,Int64,String,Uint32,Uint64},
which doesn't break the interface.

And replaces ptypes.TimestampProto by google.golang.org/protobuf/types/known/timestamppb.
This works since github.com/golang/protobuf has been bumped to v1.5.2,
which makes ptypes.TimestampProto an alias to timestamppb.Timestamp.
This is only used test (openmetrics_create_test.go), which won't break
interfaces.

Updates: prometheus#317

Signed-off-by: Shengjing Zhu <zhsj@debian.org>
@zhsj
Copy link

zhsj commented Dec 22, 2022

Continue my comments #427 (comment) here, since I find it's not easy to get rid of proto.MarshalTextString.

proto.MarshalTextString results a string with <,> as delim, but prototext.Marshal uses {,} as delim. The upstream doc says the text format is not stable. Should we rely on this?

Besides, there's no decoder for FmtProtoText, is there any user for this format?

@beorn7
Copy link
Member

beorn7 commented Dec 30, 2022

I don't think anyone depends on the exact format of FmtProtoText (at least I don't think anyone should depend on it).

zhsj added a commit to zhsj/prometheus-common that referenced this issue Mar 1, 2023
@zhsj
Replace deprecated github.com/golang/protobuf package
9932f23 
This replaces usage of proto.{Float64,Int32,Int64,String,Uint32,Uint64},
which doesn't break the interface.

And replaces ptypes.TimestampProto by google.golang.org/protobuf/types/known/timestamppb.
This works since github.com/golang/protobuf has been bumped to v1.5.2,
which makes ptypes.TimestampProto an alias to timestamppb.Timestamp.
This is only used test (openmetrics_create_test.go), which won't break
interfaces.

Updates: prometheus#317

Signed-off-by: Shengjing Zhu <zhsj@debian.org>
zhsj added a commit to zhsj/prometheus-common that referenced this issue Mar 1, 2023
@zhsj
Replace deprecated github.com/golang/protobuf package
9511718 
This replaces usage of proto.{Float64,Int32,Int64,String,Uint32,Uint64},
which doesn't break the interface.

And replaces ptypes.TimestampProto by google.golang.org/protobuf/types/known/timestamppb.
This works since github.com/golang/protobuf has been bumped to v1.5.2,
which makes ptypes.TimestampProto an alias to timestamppb.Timestamp.
This is only used test (openmetrics_create_test.go), which won't break
interfaces.

Updates: prometheus#317

Signed-off-by: Shengjing Zhu <zhsj@debian.org>
zhsj added a commit to zhsj/prometheus-common that referenced this issue Mar 1, 2023
@zhsj
Replace deprecated github.com/golang/protobuf package
439a611 
This replaces usage of proto.{Float64,Int32,Int64,String,Uint32,Uint64},
which doesn't break the interface.

And replaces ptypes.TimestampProto by google.golang.org/protobuf/types/known/timestamppb.
This works since github.com/golang/protobuf has been bumped to v1.5.2,
which makes ptypes.TimestampProto an alias to timestamppb.Timestamp.
This is only used test (openmetrics_create_test.go), which won't break
interfaces.

Updates: prometheus#317

Signed-off-by: Shengjing Zhu <zhsj@debian.org>
@SuperQ
Copy link
Member

SuperQ commented May 4, 2023

I fixed the issue of proto.MarshalTextString() by updating prometheus/client_model to google.golang.org/protobuf/proto first.

This is now fixed with #479

@SuperQ SuperQ closed this as completed May 4, 2023
@amotl amotl mentioned this issue Jul 21, 2023
Closed
@rufreakde rufreakde mentioned this issue Sep 7, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@mem @zhsj @SuperQ @beorn7 @bwplotka