Upgrade gRPC to 1.38.x #3056

pavolloffay · 2021-06-04T15:06:56Z

Builds on top of #2857

Resolves #2771
Resolves #2172

proto-gen/api_v2/codec.go

pavolloffay · 2021-06-04T15:22:19Z

proto-gen/api_v2/codec.go

+)
+
+func init() {
+	encoding.RegisterCodec(newCodec())


@bogdandrutu or @tigrannajaryan could you please loop in here? This PR updates to the latest gRPC and protobuf.

Jaeger uses gogo proto as OTELcol and it uses the same features - custom type for IDs and nonullable fields. To make this with the latest dependencies this codes registration is needed. I could not find the custom codec registration in OTELcol. Do you know why it is not needed?

Do you know why it is not needed?

I am not very familiar with this specific functionality of gogo, I don't know the answer to this.

thanks for looking at this. The answer why it works in the collector is here #3056 (comment)

codecov · 2021-06-05T14:00:06Z

Codecov Report

Merging #3056 (3122fab) into master (013ecf2) will decrease coverage by 0.10%.
The diff coverage is 94.73%.

@@            Coverage Diff             @@
##           master    #3056      +/-   ##
==========================================
- Coverage   95.97%   95.86%   -0.11%     
==========================================
  Files         229      233       +4     
  Lines        9956    10057     +101     
==========================================
+ Hits         9555     9641      +86     
- Misses        330      345      +15     
  Partials       71       71

Impacted Files	Coverage Δ
cmd/collector/app/zipkin/http_handler.go	`97.97% <ø> (ø)`
cmd/query/app/grpc_handler.go	`97.33% <ø> (ø)`
plugin/storage/grpc/shared/plugin.go	`0.00% <ø> (ø)`
plugin/storage/memory/memory.go	`100.00% <ø> (ø)`
cmd/agent/app/reporter/grpc/builder.go	`94.23% <80.00%> (-1.61%)`	⬇️
pkg/gogocodec/codec.go	`100.00% <100.00%> (ø)`
pkg/discovery/grpcresolver/grpc_resolver.go	`98.21% <0.00%> (-1.79%)`	⬇️
...ugin/sampling/strategystore/adaptive/aggregator.go	`54.76% <0.00%> (ø)`
plugin/sampling/strategystore/adaptive/span.go	`93.54% <0.00%> (ø)`
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 013ecf2...3122fab. Read the comment docs.

pavolloffay · 2021-06-07T12:52:01Z

This is ready for review/merge

Signed-off-by: Pavol Loffay <[email protected]>

jpkrohling

It would be good to have a review from someone with more experience with gRPC/Gogo, as I don't really understand how proto-gen/codec.go fixes our problems.

In any case, is this something we'd want for the long-term, or do you think we still need a better solution in the future?

jpkrohling · 2021-06-07T12:57:11Z

cmd/agent/app/reporter/grpc/builder.go

@@ -123,3 +125,12 @@ func (b *ConnBuilder) CreateConnection(logger *zap.Logger, mFactory metrics.Fact

 	return conn, nil
 }
+
+// generateAndRegisterManualResolver was removed from grpc.


Did they mention why it was removed?

It seems they leave the work for the caller.

Perhaps we should move to WithResolver?

grpc/grpc-go#3960

Not much info about why it was removed here.

proto-gen/codec.go

Signed-off-by: Pavol Loffay <[email protected]>

pavolloffay · 2021-06-07T14:29:18Z

In any case, is this something we'd want for the long-term, or do you think we still need a better solution in the future?

correct, this is rather a short-term solution. Since the gogo is compatible only with go proto V1 and it will not be made compatible with v2 we will have to migrate. It seems that solutions like https://github.com/planetscale/vtprotobuf (based on gogo/protobuf#691 (comment)) will be way forward. It is a plugin for proto compiler that generates serialization methods, it does not modify generated model classes.
This has a couple of implications for us:

no support for nonnullable fields
no support for custom types e.g. (TraceID)

Signed-off-by: Pavol Loffay <[email protected]>

pavolloffay · 2021-06-07T14:35:54Z

@joe-elliott / @yurishkuro could you please review?

yurishkuro

Can you try running a ser/deser benchmark before this upgrade and after?

go.mod

yurishkuro · 2021-06-07T14:47:57Z

proto-gen/codec_test.go

+	"github.com/jaegertracing/jaeger/model"
+)
+
+func TestCodecMarshallAndUnmarshall_jaeger_type(t *testing.T) {


are you sure tests are being executed in this proto-gen package? I thought we were excluding them (you have a fmt issue above so pretty sure linter is not running here). I think it's better not to mix generated code with manual code.

To what package should we move this then? I can think of model only.

I think it's better not to mix generated code with manual code.

We already do mix packages e.g. model package.

in the model we do it out of necessity, since gen-ed code doubles as domain model and the custom types ser/deset methods need to be there. But we usually don't write manual code in ***-gen packages.

You can create a new package like pkg/gogocodec

The best would be get rid of this, I cannot find custom codec in OTELcol, so perhaps it's doable to do it without it.

The custom codec is needed only if a custom type (e.g. TaceID) is used directly in the request (probably also response) object - https://github.com/jaegertracing/jaeger-idl/blob/master/proto/api_v2/query.proto#L38 via https://github.com/jaegertracing/jaeger-idl/blob/master/proto/api_v2/query.proto#L128. If the TraceID` was wrapped into another message the serialization would work without the custom codec.

@yurishkuro I have moved the codec to ./pkg/gogocodec package and imported it in query grpc handler and grpc plugin code.

Signed-off-by: Pavol Loffay <[email protected]>

pavolloffay · 2021-06-08T06:45:31Z

Benchmark results look good to me. See the results below. The benchmark code is in this PR in the model package.

This PR:

GOMAXPROCS=1 go test -bench=. -test.benchtime=10s -benchmem -cpuprofile profile_cpu.out ./model                                                                                             
goos: linux
goarch: amd64
pkg: github.com/jaegertracing/jaeger/model
cpu: Intel(R) Core(TM) i7-9850H CPU @ 2.60GHz
BenchmarkKeyValueIsLessAndEquals 	313754542	        38.84 ns/op	       0 B/op	       0 allocs/op
BenchmarkKeyValuesSort           	83939259	       154.1 ns/op	      24 B/op	       1 allocs/op
BenchmarkKeyValuesWrapping       	1000000000	         0.2424 ns/op	       0 B/op	       0 allocs/op
BenchmarkSpanHash                	  552618	     21807 ns/op	    2695 B/op	     112 allocs/op
BenchmarkBatchSerialization/marshal         	15824212	       749.9 ns/op	     480 B/op	       7 allocs/op
BenchmarkBatchSerialization/unmarshal       	 7336722	      1737 ns/op	    1592 B/op	      25 allocs/op
PASS
ok  	github.com/jaegertracing/jaeger/model	68.924s

Master with github.com/gogo/protobuf/proto

GOMAXPROCS=1 go test -bench=. -test.benchtime=10s -benchmem -cpuprofile profile_cpu.out ./model                                                                                               
go: downloading github.com/golang/protobuf v1.3.5
goos: linux
goarch: amd64
pkg: github.com/jaegertracing/jaeger/model
cpu: Intel(R) Core(TM) i7-9850H CPU @ 2.60GHz
BenchmarkKeyValueIsLessAndEquals 	306750854	        39.29 ns/op	       0 B/op	       0 allocs/op
BenchmarkKeyValuesSort           	84167293	       151.4 ns/op	      24 B/op	       1 allocs/op
BenchmarkKeyValuesWrapping       	1000000000	         0.2390 ns/op	       0 B/op	       0 allocs/op
BenchmarkSpanHash                	  544084	     21888 ns/op	    2695 B/op	     112 allocs/op
BenchmarkBatchSerialization/marshal         	11489486	      1047 ns/op	     624 B/op	      10 allocs/op
BenchmarkBatchSerialization/unmarshal       	 7310154	      1633 ns/op	    1592 B/op	      25 allocs/op
PASS
ok  	github.com/jaegertracing/jaeger/model	68.099s

Master with ithub.com/golang/protobuf/proto

GOMAXPROCS=1 go test -bench=. -test.benchtime=10s -benchmem -cpuprofile profile_cpu.out ./model                                                                                                 
goos: linux
goarch: amd64
pkg: github.com/jaegertracing/jaeger/model
cpu: Intel(R) Core(TM) i7-9850H CPU @ 2.60GHz
BenchmarkKeyValueIsLessAndEquals 	306549897	        42.74 ns/op	       0 B/op	       0 allocs/op
BenchmarkKeyValuesSort           	77749569	       158.2 ns/op	      24 B/op	       1 allocs/op
BenchmarkKeyValuesWrapping       	1000000000	         0.2391 ns/op	       0 B/op	       0 allocs/op
BenchmarkSpanHash                	  553023	     21951 ns/op	    2695 B/op	     112 allocs/op
BenchmarkBatchSerialization/marshal         	11297779	      1064 ns/op	     624 B/op	      10 allocs/op
BenchmarkBatchSerialization/unmarshal       	 7264723	      1635 ns/op	    1592 B/op	      25 allocs/op
PASS
ok  	github.com/jaegertracing/jaeger/model	68.930s

jpkrohling · 2021-06-08T08:36:13Z

Do I read it right that golang/protobuf and gogo/protobuf are on the same ballpark? If this benchmark is correct, is it even worth using gogo at all in our case?

Signed-off-by: Pavol Loffay <[email protected]>

pavolloffay · 2021-06-08T08:47:03Z

Do I read it right that golang/protobuf and gogo/protobuf are on the same ballpark? If this benchmark is correct, is it even worth using gogo at all in our case?

We do not use gogo only for perf but also for using custom types (e.g. TraceID) and minimize allocations by not using nullable fields - this is not reflected in serialization benchmark. Also even when using golang/proto serialization the gogo generated code is most likely used. To properly compare gogo and stdlib the benchmark would have to generate model twice. For purpose of this PR ignore the golang/protobuf result.

Signed-off-by: Pavol Loffay <[email protected]>

joe-elliott · 2021-06-08T17:06:41Z

plugin/storage/integration/grpc_test.go

@@ -44,6 +44,8 @@ func (s *GRPCStorageIntegrationTestSuite) initialize() error {
 	err := command.ParseFlags([]string{
 		"--grpc-storage-plugin.binary",
 		s.pluginBinaryPath,
+		"--grpc-storage-plugin.log-level",


I think I added this when I was trying to determine why the plugin wasn't working. It's not necessary but I don't think it hurts anything either.

pavolloffay commented Jun 4, 2021

View reviewed changes

proto-gen/api_v2/codec.go Outdated Show resolved Hide resolved

pavolloffay commented Jun 4, 2021

View reviewed changes

pavolloffay marked this pull request as ready for review June 5, 2021 12:21

pavolloffay requested a review from a team as a code owner June 5, 2021 12:21

pavolloffay requested a review from vprithvi June 5, 2021 12:21

pavolloffay force-pushed the grpc1.36 branch 2 times, most recently from e09a071 to 1e614d2 Compare June 5, 2021 12:28

pavolloffay mentioned this pull request Jun 7, 2021

Error: invalid Go type model.TraceID for field jaeger.api_v2.Span.trace_id (Badger storage) #2367

Closed

pavolloffay added 2 commits June 7, 2021 15:00

Upgrade gRPC to 1.38.x

d77bf20

Signed-off-by: Pavol Loffay <[email protected]>

ui

425b1b1

Signed-off-by: Pavol Loffay <[email protected]>

pavolloffay force-pushed the grpc1.36 branch from fe745a4 to 425b1b1 Compare June 7, 2021 13:00

pavolloffay added 2 commits June 7, 2021 15:02

Fix thrift

5c030ca

Signed-off-by: Pavol Loffay <[email protected]>

fix ugly thrift

d6a9a4c

Signed-off-by: Pavol Loffay <[email protected]>

jpkrohling reviewed Jun 7, 2021

View reviewed changes

pavolloffay added 3 commits June 7, 2021 15:20

Add test

6e2ee65

Signed-off-by: Pavol Loffay <[email protected]>

Add wire compatibility test

83cc17e

Signed-off-by: Pavol Loffay <[email protected]>

import fmt

ee925af

Signed-off-by: Pavol Loffay <[email protected]>

more tests

6572164

Signed-off-by: Pavol Loffay <[email protected]>

yurishkuro reviewed Jun 7, 2021

View reviewed changes

pavolloffay added 2 commits June 8, 2021 08:16

Bump mux

22735fe

Signed-off-by: Pavol Loffay <[email protected]>

Add benchmark

7e18bd8

Signed-off-by: Pavol Loffay <[email protected]>

move package

76cc415

Signed-off-by: Pavol Loffay <[email protected]>

lint

3122fab

Signed-off-by: Pavol Loffay <[email protected]>

yurishkuro approved these changes Jun 8, 2021

View reviewed changes

yurishkuro merged commit a2b2e4b into jaegertracing:master Jun 8, 2021

pavolloffay mentioned this pull request Jun 8, 2021

WIP - Upgrade GRPC/Proto to Latest #2857

Closed

joe-elliott reviewed Jun 8, 2021

View reviewed changes

pavolloffay mentioned this pull request Jun 9, 2021

Use withResolver API in gRPC reporter #3078

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade gRPC to 1.38.x #3056

Upgrade gRPC to 1.38.x #3056

pavolloffay commented Jun 4, 2021 •

edited

Loading

pavolloffay Jun 4, 2021

tigrannajaryan Jun 8, 2021

pavolloffay Jun 8, 2021

codecov bot commented Jun 5, 2021 •

edited

Loading

pavolloffay commented Jun 7, 2021

jpkrohling left a comment

jpkrohling Jun 7, 2021

pavolloffay Jun 7, 2021

joe-elliott Jun 8, 2021

pavolloffay commented Jun 7, 2021

pavolloffay commented Jun 7, 2021

yurishkuro left a comment

yurishkuro Jun 7, 2021

pavolloffay Jun 7, 2021

yurishkuro Jun 7, 2021

pavolloffay Jun 7, 2021

pavolloffay Jun 7, 2021

pavolloffay Jun 8, 2021

pavolloffay commented Jun 8, 2021

jpkrohling commented Jun 8, 2021

pavolloffay commented Jun 8, 2021

joe-elliott Jun 8, 2021

Upgrade gRPC to 1.38.x #3056

Upgrade gRPC to 1.38.x #3056

Conversation

pavolloffay commented Jun 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jun 5, 2021 • edited Loading

Codecov Report

pavolloffay commented Jun 7, 2021

jpkrohling 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

pavolloffay commented Jun 7, 2021

pavolloffay commented Jun 7, 2021

yurishkuro 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

pavolloffay commented Jun 8, 2021

jpkrohling commented Jun 8, 2021

pavolloffay commented Jun 8, 2021

Choose a reason for hiding this comment

pavolloffay commented Jun 4, 2021 •

edited

Loading

codecov bot commented Jun 5, 2021 •

edited

Loading