Replace deprecated github.com/golang/protobuf package #427

zhsj · 2022-12-22T03:48:19Z

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: #317

Signed-off-by: Shengjing Zhu zhsj@debian.org

zhsj · 2022-12-22T03:52:26Z

Some format changes in the PR is caused by gopls format, I can leave them untouched if you don't like that.

And this PR doesn't replace proto.MarshalTextString. After golang/protobuf#1442 is fixed, we can only use google.golang.org/protobuf and still consume v1 message from client_model package.

zhsj · 2022-12-22T06:48:27Z

For proto.MarshalTextString, I think we can do like this

diff --git a/expfmt/encode.go b/expfmt/encode.go
index 64dc0eb..73e84ee 100644
--- a/expfmt/encode.go
+++ b/expfmt/encode.go
@@ -18,9 +18,10 @@ import (
        "io"
        "net/http"
 
-       "github.com/golang/protobuf/proto" //nolint:staticcheck // Ignore SA1019. Need to keep deprecated package for compatibility.
        "github.com/matttproud/golang_protobuf_extensions/pbutil"
        "github.com/prometheus/common/internal/bitbucket.org/ww/goautoneg"
+       "google.golang.org/protobuf/encoding/prototext"
+       "google.golang.org/protobuf/runtime/protoimpl"
 
        dto "github.com/prometheus/client_model/go"
 )
@@ -133,7 +134,11 @@ func NewEncoder(w io.Writer, format Format) Encoder {
        case FmtProtoText:
                return encoderCloser{
                        encode: func(v *dto.MetricFamily) error {
-                               _, err := fmt.Fprintln(w, proto.MarshalTextString(v))
+                               m, err := prototext.Marshal(protoimpl.X.ProtoMessageV2Of(v))
+                               if err != nil {
+                                       return err
+                               }
+                               _, err = w.Write(m)
                                return err
                        },
                        close: func() error { return nil },

This uses google.golang.org/protobuf/runtime/protoimpl. Regarding golang/protobuf#1442, the grpc people feel it's a temporary solution to use that package. But well I think it's already in the public exported place..

roidelapluie · 2022-12-22T08:48:48Z

CircleCI Pipeline — Could not find a usable config.yml, you may have revoked the CircleCI OAuth app.

zhsj · 2022-12-22T09:22:39Z

CircleCI Pipeline — Could not find a usable config.yml, you may have revoked the CircleCI OAuth app.

It's now fixed. I logged in CircleCI again. Probably I've revoked it before.

SuperQ · 2023-02-25T19:26:37Z

This needs a rebase to pick up go.mod conflicts.

zhsj · 2023-03-01T12:34:17Z

rebased.

SuperQ · 2023-03-01T15:06:37Z

Sorry, needs a rebase again. 😂

beorn7

(Individual comments are just nitpicking.)

If I remember correctly, the problem is that consumers of this package have to change their import paths in the same way at the same time. Even though the interface doesn't change, a different import path still means we have different types. This could even lead to really tricky situations if someone has multiple dependencies, and some are still using the old version of prometheus/common while others use the new. That's usually caught by changing the import path with new major versions. But prometheus/common is still 0.x. However, prometheus/client_golang is already at 1.x, so there is an expectation to not do breaking changes.

I think the call if we can make this change or not should be made by @kakkoyun and @bwplotka . Given how frequently prometheus/client_golang is used, that's not an easy decision.

beorn7 · 2023-03-01T16:40:14Z

expfmt/decode_test.go

 	"github.com/prometheus/common/model"
+
+	dto "github.com/prometheus/client_model/go"
+	"google.golang.org/protobuf/proto"


Nit: In the Prometheus code base, we tend to keep imports from other repos than the current one in its own block above the imports from the same repo (as it was before). And we usually also have the named imports in a different block (which wasn't adhered to before, but you could fix it now that we are on it).

format is fixed.

beorn7 · 2023-03-01T16:42:45Z

expfmt/openmetrics_create_test.go

 	dto "github.com/prometheus/client_model/go"
+	"google.golang.org/protobuf/proto"
+	"google.golang.org/protobuf/types/known/timestamppb"


Import nit as before: Have the named imports in their own block after the unnamed imports (as it was before).

beorn7 · 2023-03-01T16:43:29Z

expfmt/encode_test.go

 	dto "github.com/prometheus/client_model/go"
+	"google.golang.org/protobuf/proto"


You could fix the block structure here (it wasn't quite right before, see other nitpicking comments).

beorn7 · 2023-03-01T16:45:00Z

expfmt/text_parse.go

 	"github.com/prometheus/common/model"
+
+	dto "github.com/prometheus/client_model/go"
+	"google.golang.org/protobuf/proto"


Same nitpicking as before. This has to be

"google.golang.org/protobuf/proto" dto "github.com/prometheus/client_model/go" "github.com/prometheus/common/model"

And yes, it was wrong in a different way before...

beorn7 · 2023-03-01T16:45:20Z

expfmt/text_parse_test.go

 	dto "github.com/prometheus/client_model/go"
+	"google.golang.org/protobuf/proto"


Import nitpick as above.

beorn7 · 2023-03-01T17:05:56Z

Hmmm, thinking more about it, the only usage of the proto package outside of tests is in text_parse.go line 288, and this particular function just returns a pointer to a (standard) string. Which means that the change in this PR should actually be transparent, and importers of this package should be able to continue using the proto package of their choice. (I was already wondering why this even works without changing prometheus/client_model…)

So I would cautiously say now that this PR is OK (modulo import nitpicking ;). Still, would be good to have @kakkoyun / @bwplotka confirm.

zhsj · 2023-03-01T17:09:04Z

If I remember correctly, the problem is that consumers of this package have to change their import paths in the same way at the same time. Even though the interface doesn't change, a different import path still means we have different types.

The break already happened when bumping google/protobuf from 1.3 to 1.4, which forces using google.golang.org/protobuf.

As you can see the go.mod changes, it only moves google.golang.org/protobuf from indirect to direct.

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 · 2023-03-01T17:22:51Z

The break already happened when bumping google/protobuf from 1.3 to 1.4, which forces using google.golang.org/protobuf.

The above is not related to this repo. But for other repos that have pb.go files.

This repo as @beorn7 said, "returns a pointer to a (standard) string".

And the string format should be its interface. That's why proto.MarshalTextString is still using the old golang/protobuf package, see #317 (comment).

beorn7

Thank you very much. I'm fairly convinced this will work as intended. Would still be good to get @kakkoyun / @bwplotka approve this.

SuperQ · 2023-05-04T10:22:51Z

Fixed in #479

zhsj closed this Dec 22, 2022

zhsj reopened this Dec 22, 2022

zhsj force-pushed the proto branch from 9a16683 to 99f1ad9 Compare December 22, 2022 09:19

zhsj mentioned this pull request Dec 22, 2022

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

Closed

amotl mentioned this pull request Feb 28, 2023

Update protocol buffers library crate/cratedb-prometheus-adapter#62

Closed

zhsj force-pushed the proto branch from 99f1ad9 to 9932f23 Compare March 1, 2023 12:33

SuperQ requested a review from roidelapluie March 1, 2023 15:06

zhsj force-pushed the proto branch from 9932f23 to 9511718 Compare March 1, 2023 16:00

SuperQ requested a review from beorn7 March 1, 2023 16:19

beorn7 reviewed Mar 1, 2023

View reviewed changes

zhsj force-pushed the proto branch from 9511718 to 439a611 Compare March 1, 2023 17:16

beorn7 approved these changes Mar 1, 2023

View reviewed changes

SuperQ closed this May 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace deprecated github.com/golang/protobuf package #427

Replace deprecated github.com/golang/protobuf package #427

zhsj commented Dec 22, 2022

zhsj commented Dec 22, 2022

zhsj commented Dec 22, 2022

roidelapluie commented Dec 22, 2022

zhsj commented Dec 22, 2022

SuperQ commented Feb 25, 2023

zhsj commented Mar 1, 2023

SuperQ commented Mar 1, 2023

beorn7 left a comment •

edited

Loading

beorn7 Mar 1, 2023

zhsj Mar 1, 2023

beorn7 Mar 1, 2023

beorn7 Mar 1, 2023

beorn7 Mar 1, 2023

beorn7 Mar 1, 2023

beorn7 commented Mar 1, 2023

zhsj commented Mar 1, 2023

zhsj commented Mar 1, 2023

beorn7 left a comment

SuperQ commented May 4, 2023

		dto "github.com/prometheus/client_model/go"
		"google.golang.org/protobuf/proto"

Replace deprecated github.com/golang/protobuf package #427

Replace deprecated github.com/golang/protobuf package #427

Conversation

zhsj commented Dec 22, 2022

zhsj commented Dec 22, 2022

zhsj commented Dec 22, 2022

roidelapluie commented Dec 22, 2022

zhsj commented Dec 22, 2022

SuperQ commented Feb 25, 2023

zhsj commented Mar 1, 2023

SuperQ commented Mar 1, 2023

beorn7 left a comment • edited Loading

Choose a reason for hiding this comment

beorn7 Mar 1, 2023

Choose a reason for hiding this comment

zhsj Mar 1, 2023

Choose a reason for hiding this comment

beorn7 Mar 1, 2023

Choose a reason for hiding this comment

beorn7 Mar 1, 2023

Choose a reason for hiding this comment

beorn7 Mar 1, 2023

Choose a reason for hiding this comment

beorn7 Mar 1, 2023

Choose a reason for hiding this comment

beorn7 commented Mar 1, 2023

zhsj commented Mar 1, 2023

zhsj commented Mar 1, 2023

beorn7 left a comment

Choose a reason for hiding this comment

SuperQ commented May 4, 2023

beorn7 left a comment •

edited

Loading