Replace gogo protobuf #1256

tdeebswihart · 2023-10-03T18:29:18Z

What was 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.

Breaking changes

*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 willy-nilly. go vet will scream at you about this
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.

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.

How did you test it?

All tests for this repo pass as do all SDK tests (except for integration tests, which time out on our official repo on master...).

Potential risks

All errors that could arise from this change should be compile-time errors, so your build will be broken if I haven't yet addressed your code. I plan to port all relevant temporal repos as a part of this effort.

Checklist

Closes gogo/protobuf is unmaintained #658
How was this tested: all sdk tests pass (with the exception of integration tests, many of which time out on our official unaltered repo)
Any docs updates needed? Yes. Once all of these changes are merged our history export format will be incompatible with older SDKs due to the JSON changes; we will need to inform customers of this

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

go.mod

internal/common/serializer/jsonpb.go

cretz · 2023-10-09T15:09:20Z

internal/common/serializer/serializer.go

 )

 // SerializeBatchEvents serializes batch events into a datablob proto
 func SerializeBatchEvents(events []*historypb.HistoryEvent, encodingType enumspb.EncodingType) (*commonpb.DataBlob, error) {
 	return serialize(&historypb.History{Events: events}, encodingType)
 }

-func serializeProto(p proto.Marshaler, encodingType enumspb.EncodingType) (*commonpb.DataBlob, error) {
+func serializeProto(p Marshaler, encodingType enumspb.EncodingType) (*commonpb.DataBlob, error) {


Would also accept proto.Message here too (since this is private and it's only on our protos), but what you have here is fine too

If we accept things with a Marshal method then I can sneak vtprotobuf under the hood by swapping out the implementation of Marshal. That way nothing else needs to care about what we're using

That is only for this one part, it's not like you'll "sneak" it in everywhere. Are there plans to discourage the use of proto.Marshal for this one reason?

We'll find out: once we have the server working we'll run E2E load and see if there's a measurable performance difference. If "No" (our assumption) then no and I'll skip including vtproto

internal/error.go

internal/grpc_dialer_test.go

internal/workflow.go

internal/internal_activity.go

internal/internal_event_handlers.go

internal/internal_schedule_client.go

internal/internal_worker_test.go

Quinn-With-Two-Ns · 2023-10-13T15:09:26Z

internal/protocol/util.go

 )

+var ErrProtoNameNotFound = errors.New("protocol name not found")
+
 // NameFromMessage extracts the name of the protocol to which the supplied
 // message belongs.
 func NameFromMessage(msg *protocolpb.Message) (string, error) {


Why are we changing the behavior here?

A quick and uninformed decision while first learning the proto library. Looks like the anypb.Any type provides MessageName() which would let us have the same behavior

Also if anypb.Body.MessageName() doesn't return a message name then we've no name to report.

The old types package doesn't work with google protobuf; if we want the same semantics I'd probably have to allocate and do something like

_, err := msg.Body.UnmarshalNew() if err != nil { return "", fmt.Errorf("unrecognized message type %q: %w", msg.Body.MessageName(), err) }

cretz

Overall looks good, just very minor comments before I hit approve

cretz · 2023-10-30T13:02:31Z

.github/workflows/ci.yml

@@ -77,3 +77,5 @@ jobs:
      go-repo-path: ${{github.event.pull_request.head.repo.full_name}}
      version: ${{github.event.pull_request.head.ref}}
      version-is-repo-ref: true
+      features-repo-path: "tdeebswihart/temporal-features"


Goes without saying, just making reminder to update this and similar places when temporalio/api-go#119 is merged

client/client.go

cretz · 2023-10-30T13:11:43Z

internal/common/util/stringer.go

@@ -45,7 +48,7 @@ func anyToString(d interface{}) string {
 		buf.WriteString("(")
 		for i := 0; i < v.NumField(); i++ {
 			f := v.Field(i)
-			if f.Kind() == reflect.Invalid {
+			if f.Kind() == reflect.Invalid || privateField.MatchString(t.Field(i).Name) {


A bit overkill to use regex just to check if the first character of a string is lowercase, but meh

internal/internal_event_handlers.go

cretz · 2023-10-30T13:15:47Z

internal/internal_schedule_client.go

@@ -28,6 +28,9 @@ import (
 	"fmt"
 	"time"

+	"google.golang.org/protobuf/types/known/durationpb"
+	"google.golang.org/protobuf/types/known/timestamppb"
+


Nit - while some of the older SDK and server code required developers to manually break up non-stdlib sections by same module and separate module, modern SDK rules do not (but harmless for the most part).

internal/internal_worker.go

internal/internal_worker_test.go

cretz · 2023-10-30T13:23:45Z

internal/protocol/util.go

 )

+var ErrProtoNameNotFound = errors.New("protocol name not found")


Meh, we often don't make exported vars for singly used never otherwise-referenced errors, but no big deal

I'm a fan of them as you can use them with errors.Is, and I prefer that to string matching.

I'm happy to yield to our conventions however

We don't really have a convention here, so it's fine. I'm a fan of them when I need to use them with errors.Is, but not for every error in the SDK just because some future user may need to use them with errors.Is. Hence the "never otherwise-referenced". By that logic, we'd never have anymore inline errors and we'd have a huge API surface which I think is a bit too hardcore of a stance to take here (though may have value for other projects).

(again, no need to change)

I've been burnt by Go' stdlib refusing to expose internal networking errors before and have made a habit of it, heh. I'll keep that in mind for future errors I add however

cretz

Approving, but please do not merge until dependencies are merged/tagged

This does not yet compile as I removed the proxy. I also still need to implement history cleaning; SDKs should still load old histories that have CamelCase enums

We've swapped the roles of GoV2 and gogo here, so we forcibly include a gogo-generated message definition.

We'll want that functionality in the CLI as well so everything has been moved to a common repo.

I need to see whether the changes will impact our metrics; it's possible given what I found in the grpc_dialer tests

We'll want these in other repos as well so I've moved and renamed them. duration.Proto is clearer than common.DurationPtr and flows better. Refactoring automated using [gopatch](): ```diff @@ @@ +import "go.temporal.io/api/types/duration" -common.DurationValue(...) +duration.Value(...) @@ @@ +import "go.temporal.io/api/types/duration" -common.DurationPtr(...) +duration.Proto(...) @@ @@ +import "go.temporal.io/api/types/duration" -common.MinDurationPtr(...) +duration.MinProto(...) @@ @@ +import "go.temporal.io/api/types/timestamp" -common.TimeValue(...) +timestamp.Value(...) @@ @@ +import "go.temporal.io/api/types/timestamp" -common.TimePtr(...) +timestamp.Proto(...) ```

This way CI can run and others can function

jsonpb is dead, long live jsonpb!

Our error messages and metric tags are back to their old values, so our users shouldn't notice that we changed our proto library.

Let's see if this works

We don't need to specify `resp` twice...

I'll remove the usage of my fork of features for CI in a second pass

**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

cretz reviewed Oct 9, 2023

View reviewed changes

Quinn-With-Two-Ns reviewed Oct 13, 2023

View reviewed changes

internal/internal_worker_test.go Outdated Show resolved Hide resolved

Quinn-With-Two-Ns reviewed Oct 13, 2023

View reviewed changes

tdeebswihart mentioned this pull request Oct 26, 2023

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

Merged

tdeebswihart force-pushed the replace-gogo-protobuf branch from e1d8372 to d234e15 Compare October 26, 2023 23:38

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

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

cretz reviewed Oct 30, 2023

View reviewed changes

cretz approved these changes Nov 3, 2023

View reviewed changes

Quinn-With-Two-Ns approved these changes Nov 9, 2023

View reviewed changes

tdeebswihart added 20 commits November 19, 2023 15:39

Begin converting the SDK off of gogo/protobuf

d507797

This does not yet compile as I removed the proxy. I also still need to implement history cleaning; SDKs should still load old histories that have CamelCase enums

Convert more over to google's protobuf

becd938

Repoint all other modules at my forks

789c142

Fix copying of retry policies

94c5bff

Update converter tests and explicitly test gogo

4e3f025

We've swapped the roles of GoV2 and gogo here, so we forcibly include a gogo-generated message definition.

Move history loading to api-go repo and update tests

39c3a58

We'll want that functionality in the CLI as well so everything has been moved to a common repo.

Fix more tests that relied on old enum naming

19d9a24

I need to see whether the changes will impact our metrics; it's possible given what I found in the grpc_dialer tests

Replace a new reference to gogo's any type

d26360f

Use useful replace directives and tidy all modules

720b77e

This way CI can run and others can function

Address review comments

213fd0d

Fix fmt call I made unnecessarily complex

66817b8

Remove unused helper function

5e9c4c6

Use the new api-go jsonpb code

a626c00

Use the newly renamed temporalproto code

59e8215

jsonpb is dead, long live jsonpb!

Bump api-go and fix test metrics

b642ecd

Our error messages and metric tags are back to their old values, so our users shouldn't notice that we changed our proto library.

Update go-api dep

8d4cfc0

Use my fork of our features repo

e0317be

Don't use my fork of the features repo

f407630

Let's see if this works

Bump dependencies

1309c0b

tdeebswihart added 17 commits November 19, 2023 15:39

Fix how we check if times are specified

d26cde7

I can't replicate this locally so need to do so in CI...

3717ab9

See previous commit...

f3b6f59

This will be a long project

191aee1

Use my features repo once more

b48ff6d

Let's see if this is enough

40c1f96

Fix time bugs

bc3a74f

Those aren't times but durations

7ca2c2a

Don't print private fields in proto objects

60326cd

I flipped the logic without flipping the regexp...

2c2c5d2

Add some debugging

73c3aaa

Try a new format

0d40b87

Update how we print

c1bfc6b

Bump api-go dependency

f42a9db

Specify DiscardUnknown on our history json decoders

c47df1f

Address review comments

d083a5f

Fix replay workflow error message

99988e4

We don't need to specify `resp` twice...

tdeebswihart force-pushed the replace-gogo-protobuf branch from 77a077b to 39f695a Compare November 19, 2023 23:40

Bump go-api version

49eced6

tdeebswihart force-pushed the replace-gogo-protobuf branch from 39f695a to 49eced6 Compare November 19, 2023 23:46

tdeebswihart added 7 commits November 20, 2023 15:45

Bump api-go dependency

9e29e05

I forgot the test directory

31d0607

Bump all go versions to 1.20 and tidy

edc4bbe

math/rand has been deprecated and errcheck fails on it

e73f11e

Handle the error from rand.Read

d440028

Bump version of api-go

07a2c7e

Remove references to my fork of api-go

a11ee59

I'll remove the usage of my fork of features for CI in a second pass

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

tdeebswihart merged commit f9d73bf into temporalio:master Nov 21, 2023
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace gogo protobuf #1256

Replace gogo protobuf #1256

tdeebswihart commented Oct 3, 2023 •

edited

Loading

cretz Oct 9, 2023

tdeebswihart Oct 9, 2023

cretz Oct 9, 2023 •

edited

Loading

tdeebswihart Oct 9, 2023

Quinn-With-Two-Ns Oct 13, 2023

tdeebswihart Oct 13, 2023

tdeebswihart Oct 13, 2023

cretz left a comment

cretz Oct 30, 2023

cretz Oct 30, 2023

cretz Oct 30, 2023

cretz Oct 30, 2023

tdeebswihart Oct 30, 2023

cretz Oct 30, 2023 •

edited

Loading

tdeebswihart Oct 30, 2023

cretz left a comment

		)

		var ErrProtoNameNotFound = errors.New("protocol name not found")

Replace gogo protobuf #1256

Replace gogo protobuf #1256

Conversation

tdeebswihart commented Oct 3, 2023 • edited Loading

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cretz Oct 9, 2023 • edited Loading

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

cretz 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

cretz Oct 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cretz left a comment

Choose a reason for hiding this comment

tdeebswihart commented Oct 3, 2023 •

edited

Loading

cretz Oct 9, 2023 •

edited

Loading

cretz Oct 30, 2023 •

edited

Loading