Replace gogo/protobuf with google/protobuf #5032

tdeebswihart · 2023-10-24T22:29:28Z

What changed?

gogo/protobuf has been replaced with Google's official go compiler.

Why?

gogo/protobuf has been deprecated for some time and the community is moving on, building new tools (like vtproto) atop google's v2 compiler.

How did you test it?

make test

Potential risks

The change from embedded gogo-generated-structs to google-generated-pointers-to-structs created a risk of nil pointer exceptions. I've fixed all the ones our tests found but it's possible there are more lurking in the new code.
This change may cause our performance to decrease. Certainly encoding/deconding of proto objects will become slower, but the overuse of pointers by the google compiler may negatively affect our overall performance. We'll need to keep an eye on the GC stats
This breaks the HTTP API. We will not support shortand payload encoding in this first pass; that will come once this initial work is in testing.

Breaking changes for developers

*time.Time in proto structs will now be timestamppb.Timestamp
*time.Duration will now be durationpb.Duration
V2-generated structs embed locks, so you cannot dereference them. go vet will scream at you about this. If you need a copy, use proto.Clone.
- If the performance of this sucks then I will either update our code generator to add shallow-clone methods or hand-roll the ones we need
Proto enums will, when formatted to JSON, now be in SCREAMING_SNAKE_CASE rather than PascalCase. We decided (in discussion with the SDK team) that now was as good a time as any to rip the bandage off.
Proto objects, or objects embedding protos, cannot be compared using reflect.DeepEqual or anything that uses it. This includes testify and mock equality testers!
- You will need to use the common/testing/protorequire, common/testing/protoassert, or common/testing/protomock packages instead. I've implemented proto-compatible matchers and assertions there for all cases I've encountered
- If you need reflect.DeepEqual for any reason you can use go.temporal.io/api/temporalproto.DeepEqual instead

Note that history loading will not be impacted by the JSON changes: I rewrote history loading to dynamically fix incoming history JSON data (like all our other sdks); you can find this code in my fork of our go API alongside its tests.

🚨Sharp Edges Introduced🚨

Beware *timestamppb.Timestamp.AsTime(). If you need to extract a time value from a proto time (timestamppb) always make sure to check whether it's nil first. When the proto object is nil AsTime() will return a non-zero time at the proto epoch: UTC midnight on January 1, 1970.

I've made this mistake multiple times during this transition and each time it's been a pain to debug

Is hotfix candidate?

Haha. No.

Release Plan

Our goal is to release our API, Go API, Go SDK, and Server all at once, then update our UI (and ui-server), CLI, and HTTP API in a second pass. Until the second pass happens no changes to our JSON formatting will be apparent to users

tdeebswihart · 2023-10-27T23:12:07Z

Please ignore golangci-lint: I changed enough code that it's complaining about existing issues that I happen to brush the dust off of

bergundy · 2023-11-07T18:46:39Z

Makefile

+	@go install google.golang.org/protobuf/cmd/protoc-gen-go@latest
 	@go install google.golang.org/grpc/cmd/protoc-gen-go-grpc@latest


Curious why we're installing latest as opposed to using the modfile.

No reason. I'd be happy to change that if the team cares

I like latest. I would use it everywhere. Yes, build can be broken because of breaking changes in dependencies (and it happened few times already) but I think it is "ok" trade-off for keeping these build dependencies up to date.

dnr

not done yet, but I got through proto/ (going in alphabetical order) and figured I should send what I had so far

Makefile

dnr · 2023-11-09T01:13:10Z

common/archiver/s3store/query_parser_test.go

@@ -188,6 +187,10 @@ func (s *queryParserSuite) TestParsePrecision() {
 	}
 }

+func ptr[t any](v t) *t {


I want this in the standard library... maybe common/util/util.go though?
Also, use capital T for a type variable for consistency

Is it used anywhere else?

I swapped t for T but didn't put this into our utility library. I'd prefer that we address things regularly (using &stackVar) instead of this in our normal code; I added this to make tests simpler to write

common/clock/hybrid_logical_clock/hybrid_logical_clock.go

common/dynamicconfig/file_based_client.go

common/log/tag/zap_tag.go

common/persistence/visibility/store/elasticsearch/visibility_store_read_test.go

dnr · 2023-11-14T06:11:40Z

common/primitives/timestamp/time.go

@@ -27,23 +27,24 @@ package timestamp
 import (
 	"time"

-	"github.com/gogo/protobuf/types"
+	"google.golang.org/protobuf/types/known/durationpb"
+	"google.golang.org/protobuf/types/known/timestamppb"
 )

 // Timestamp provides easy conversions and utility comparison functions
 // making go to proto time comparison straightforward
 type Timestamp struct {


afaict this is unused.. why not delete it? (or at least don't touch it in this PR and delete it later)

I would love to see this go away!

I'm planning to remove it in a separate PR after the whole proto adventure is over

develop/buildkite/Dockerfile

dnr · 2023-11-14T08:18:49Z

go.mod

@@ -65,6 +68,7 @@ require (
 )

 require (
+	github.com/gogo/protobuf v1.3.2 // indirect


there's still an indirect dep? is that from sdk and it'll go away soon?

The SDK supports Gogo for unmarshaling user-supplied protos. I have no idea if we need to continue supporting it; @cretz do we still need to support gogo in our payload converters or can it be removed entirely?

I think we can remove it. It sort of breaking change but I doubt that anyone use gogo to generate in/out params.

The SDK team has mentioned we need to keep this as some of our customers use Gogo/protobuf

dnr · 2023-11-14T08:21:00Z

proto/dependencies/google/protobuf/empty.proto

+//       rpc Bar(google.protobuf.Empty) returns (google.protobuf.Empty);
+//     }
+//
+message Empty {}


we can't get this through a dependency with the other well-known types?

yes, I would add it to api repo.

actually, why do you need it all? I don't see it's being used.

This is the only repo that uses it so I didn't bother vendoring it in our API repo. I can if you'd prefer

It is used in exactly one location

I would vote for api repo. @dnr why did you use it at all? Why not bool or enum?

Makefile

alexshtin · 2023-11-09T03:20:36Z

Makefile

+	@go install google.golang.org/protobuf/cmd/protoc-gen-go@latest
 	@go install google.golang.org/grpc/cmd/protoc-gen-go-grpc@latest


I like latest. I would use it everywhere. Yes, build can be broken because of breaking changes in dependencies (and it happened few times already) but I think it is "ok" trade-off for keeping these build dependencies up to date.

common/primitives/timestamp/time.go

alexshtin · 2023-11-16T00:21:28Z

service/matching/forwarder.go

-		if expirationDuration <= 0 {
+	var expirationDuration *durationpb.Duration
+	expirationTime := task.event.Data.ExpiryTime.AsTime()
+	if task.event.Data.ExpiryTime != nil && !expirationTime.IsZero() {


I understand that line above would work even if ExpiryTime is nil. But this check here seems to be not needed. If you want to keep it, move it higher.

This check is needed, and skipping the nil check causes a test failure because of how the zero protobuf time (not a zero go time) is handled.

we probably only need to check whether ExpiryTime is nil, honestly. I'll see what happens if I do

service/matching/matching_engine.go

alexshtin · 2023-11-16T01:00:28Z

tests/functional_test_base.go

 	strctV := reflect.ValueOf(strct)
 	strctT := strctV.Type()

 	ret := map[string]any{}

 	for i := 0; i < strctV.NumField(); i++ {
 		field := strctV.Field(i)
-		var fieldData any
+		// Skip private members


"public" => "exported",
"private" => "unexported".

We compare the equality of the History structs formatted using this against concrete strings (search for EqualHistory). Before I added this the naive reflection we used here dumped the unexported members that the google compiler generated, polluting our tests. You can comment that line out and see how bad it was

alexshtin · 2023-11-16T04:15:00Z

tests/update_workflow_test.go

@@ -662,6 +662,7 @@ func (s *functionalSuite) TestUpdateWorkflow_NormalScheduledWorkflowTask_AcceptC
 				case 1:
 					return nil, nil
 				case 2:
+					s.Require().True(len(task.Messages) > 0, "Task has no messages", task)


What is that? Why not s.True()?

Without the require we'd immediately access the first element of a zero-length slice on the next line, crashing and dumping a useless stack trace

It should always have them, but ok. I will check it later.

alexshtin · 2023-11-16T04:20:37Z

go.mod

@@ -65,6 +68,7 @@ require (
 )

 require (
+	github.com/gogo/protobuf v1.3.2 // indirect


I think we can remove it. It sort of breaking change but I doubt that anyone use gogo to generate in/out params.

dnr

comments through service/frontend

dnr · 2023-11-16T23:18:13Z

service/frontend/operator_handler.go

@@ -70,6 +70,8 @@ var _ OperatorHandler = (*OperatorHandlerImpl)(nil)
 type (
 	// OperatorHandlerImpl - gRPC handler interface for operator service
 	OperatorHandlerImpl struct {
+		operatorservice.UnimplementedOperatorServiceServer


does this make it so you can leave out handlers and they'll just return unimplemented? if so I don't think we should do this. requiring someone to write stub (or real) implementations seems like a good thing?

Yes and I've replaced that with the Unsafe variant generated by the GRPC package which will cause a compilation error if we leave out an impl

dnr · 2023-11-16T23:34:37Z

service/frontend/http_api_server.go

-		return proxy.NewGRPCGatewayJSONPBMarshaler(m, u)
-	}
+	return newProtoJsonMarshaler(indent)
+	// TODO: reintroduce shorthand JSON marshaling here


and is the intention to update go.temporal.io/api/proxy and move this back? or do it in the new marshaller in this package? I don't really understand what's going on in this area

dnr · 2023-11-16T23:35:52Z

service/frontend/workflow_handler.go

@@ -104,6 +106,7 @@ var (
 type (
 	// WorkflowHandler - gRPC handler interface for workflowservice
 	WorkflowHandler struct {
+		workflowservice.UnimplementedWorkflowServiceServer


same question here

We have to embed either this or (as I just now discovered) the UnsafeFOOServiceServer generated by the GRPC code. It's a requirement as we must implement an internal-only interface...

I'm going to swap these to use the Unsafe variant as that will cause compilation errors when new methods are added

dnr · 2023-11-16T23:38:03Z

service/frontend/workflow_handler.go

@@ -2091,7 +2094,7 @@ func (wh *WorkflowHandler) ListArchivedWorkflowExecutions(ctx context.Context, r

 	// special handling of ExecutionTime for cron or retry
 	for _, execution := range archiverResponse.Executions {
-		if timestamp.TimeValue(execution.GetExecutionTime()).IsZero() {
+		if execution.ExecutionTime == nil || execution.GetExecutionTime().AsTime().IsZero() {


don't need Get if you just checked nil

Suggested change

if execution.ExecutionTime == nil || execution.GetExecutionTime().AsTime().IsZero() {

if execution.ExecutionTime == nil || execution.ExecutionTime.AsTime().IsZero() {

d'oh, thanks.

dnr · 2023-11-17T00:00:26Z

service/frontend/workflow_handler.go

@@ -4145,9 +4147,10 @@ func (wh *WorkflowHandler) unaliasStartWorkflowExecutionRequestSearchAttributes(
 	}

 	// Shallow copy request and replace SearchAttributes fields only.
-	newRequest := *request
+	// FIXME actually shallow copy


in this pr or later?

I left these in here in case we needed shallow copies for performance. Once I've gone through the performance testing I've kicked off I'll see if these are necessary (they probably aren't) and address them appropriately at that point.

I'm going to change these to // NOTE instead of fixmes

I actually just deleted my note entirely. I don't think it's necessary, and were this actually a problem we'd find it easily in a profile

dnr · 2023-11-17T17:44:25Z

service/frontend/workflow_handler.go

 	newRequest.SearchAttributes = unaliasedSas
-	return &newRequest, nil
+	return newRequest, nil


also, definitely not in this PR, but I think this pattern of replacing the SA field in SWERequest and other messages is error-prone (see the recent bug that Roey fixed). we should use the type system to differentiate between aliased and unaliased names. (which I would call external and internal since "alias" could be read both ways.) all user-facing api messages should have only external names, and internal names should be present in internal structs only. ideally confined to visibility, but that may be hard.

anyway, that's a big refactoring that can happen later.

dnr · 2023-11-17T17:50:11Z

service/frontend/workflow_handler_deprecated.go

@@ -182,7 +182,8 @@ func (wh *WorkflowHandler) getWorkflowExecutionHistory(
 					ctx,
 					wh.metricsScope(ctx),
 					namespaceID,
-					*execution,
+					// FIXME shallow copy
+					common.CloneProto(execution),


WorkflowExecution has only simple fields, so this already is a shallow copy

I would do it like this: just pass the pointer here. then on the four lines that set execution.RunId, have those lines do the copy like execution = &commonpb.WorkflowExecution{WorkflowId: execution.WorkflowId, RunId: newRunId}. that way we treat WorkflowExecution as immutable.

I can't actually figure out why we shallow copied these in the first place; I don't see any mutations in these calls

Please don't review this commit. You will regret it

**What changed?** gogo/protobuf has been replaced with Google's official go compiler. **Why?** gogo/protobuf has been deprecated for some time and the community is moving on, building new tools (like vtproto) atop google's v2 compiler. **How did you test it?** This branch is actively used by my [SDK](temporalio/sdk-go#1256) and [server](temporalio/temporal#5032) PRs so you can see it in use there **Potential risks** None I can think of. **Breaking changes for developers** - `*time.Time` in proto structs will now be [timestamppb.Timestamp](https://pkg.go.dev/google.golang.org/protobuf@v1.31.0/types/known/timestamppb#section-documentation) - `*time.Duration` will now be [durationpb.Duration](https://pkg.go.dev/google.golang.org/protobuf/types/known/durationpb) - V2-generated structs embed locks, so you cannot dereference them. `go vet` will scream at you about this. If you need a copy, use `proto.Clone`. - Proto objects, or objects embedding protos, cannot be compared using `reflect.DeepEqual` or _anything_ that uses it. This includes `testify` and `mock` equality testers! - You will need to use the `protorequire` ro `protoassert`packages instead. I've implemented proto-compatible assertions there for all cases I've encountered - If you need `reflect.DeepEqual` for any reason you can use `go.temporal.io/api/temporalproto.DeepEqual` instead

tdeebswihart changed the title ~~Nomo gogo~~ Replace gogo/protobuf with google/protobuf Oct 24, 2023

tdeebswihart force-pushed the nomo-gogo branch 3 times, most recently from e87a0ae to b393589 Compare October 25, 2023 23:05

tdeebswihart mentioned this pull request Oct 26, 2023

Replace gogo/protobuf with google/protobuf temporalio/features#359

Merged

tdeebswihart force-pushed the nomo-gogo branch 3 times, most recently from 44cde0e to a0d20c6 Compare October 27, 2023 22:47

tdeebswihart marked this pull request as ready for review October 27, 2023 22:56

tdeebswihart requested a review from a team as a code owner October 27, 2023 22:56

tdeebswihart force-pushed the nomo-gogo branch from b6bf2c8 to 08d86dc Compare October 27, 2023 23:11

tdeebswihart force-pushed the nomo-gogo branch 3 times, most recently from 62b6681 to 561dcbd Compare November 2, 2023 21:16

tdeebswihart force-pushed the nomo-gogo branch from 561dcbd to 33ac19e Compare November 6, 2023 19:26

bergundy reviewed Nov 7, 2023

View reviewed changes

dnr reviewed Nov 14, 2023

View reviewed changes

alexshtin reviewed Nov 16, 2023

View reviewed changes

dnr reviewed Nov 17, 2023

View reviewed changes

tdeebswihart force-pushed the nomo-gogo branch 6 times, most recently from 7a4bd20 to a99f8b1 Compare November 20, 2023 19:32

alexshtin approved these changes Nov 20, 2023

View reviewed changes

tdeebswihart added 3 commits November 21, 2023 11:54

Replace gogo/protobuf with google/protobuf

0e07a14

Regenerate mocks, protobuf structs, etc.

1d3e2ce

Please don't review this commit. You will regret it

Fix a bash-ism in protoc.sh

bae9157

tdeebswihart added 8 commits November 21, 2023 11:55

Try to fix protoc.sh again

567ce8e

Fix protoc. Again

1cd7ff5

Use GOOS when picking OS

38cfeaf

Point directly at my fork of the sdk

4614536

Fix a bug I introduced in timeout handling

8b6dff5

Fix dlq printing

87a568e

Fix shellcheck complaints

9342736

Fix a few more test failures

a00b509

tdeebswihart force-pushed the nomo-gogo branch from 04fd496 to a00b509 Compare November 21, 2023 20:01

tdeebswihart added 2 commits November 21, 2023 13:07

require.Contains doesn't work with proto objects

940dd3c

Remove replaces to my forks

7103d86

tdeebswihart enabled auto-merge (squash) November 21, 2023 22:48

tdeebswihart disabled auto-merge November 21, 2023 22:55

tdeebswihart merged commit 1be76e3 into temporalio:main Nov 21, 2023

tdeebswihart deleted the nomo-gogo branch November 21, 2023 23:19

debasishbsws mentioned this pull request Apr 16, 2024

Update temporalio/ui-server module to 2.25.0 and fix: GHSA-8f25-w7qj-r7hc temporalio/cli#530

Closed

This was referenced May 7, 2024

Consider migration from gogo to vtprotobuf #38

Closed

Support gRPC Reflection API #448

Closed

		@go install google.golang.org/protobuf/cmd/protoc-gen-go@latest
		@go install google.golang.org/grpc/cmd/protoc-gen-go-grpc@latest

	if execution.ExecutionTime == nil \|\| execution.GetExecutionTime().AsTime().IsZero() {
	if execution.ExecutionTime == nil \|\| execution.ExecutionTime.AsTime().IsZero() {

Replace gogo/protobuf with google/protobuf #5032

Replace gogo/protobuf with google/protobuf #5032

Conversation

tdeebswihart commented Oct 24, 2023 • edited Loading

tdeebswihart commented Oct 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tdeebswihart commented Oct 24, 2023 •

edited

Loading