WIP - Upgrade GRPC/Proto to Latest #2857

joe-elliott · 2021-03-01T15:10:18Z

Which problem is this PR solving?

Upgrades GPRC/Proto to the latest
Fixes Upgrade google.golang.org/grpc to 1.35.0 #2771

Short description of the changes

Changes to use the latest proto builder from OTel
- we can build a jaeger image with latest and use it if desired
Copy in removed method generateAndRegisterManualResolver
- this method looks wild to me. Using a time value for the scheme? Possibly something to clean up in a future PR
Regenerated proto with the latest builder
Changed a few packages to use gogoproto over google proto
Added /proto-gen/api_v2/codec.go. This forces grpc to use gogoproto over google proto.
- I hate the location of this, but unsure if there's a better place that will be always be imported in a package that uses grpc.

Signed-off-by: Joe Elliott <[email protected]>

codecov · 2021-03-01T15:18:01Z

Codecov Report

Merging #2857 (c72be74) into master (e4cacd0) will increase coverage by 0.00%.
The diff coverage is 80.00%.

@@           Coverage Diff           @@
##           master    #2857   +/-   ##
=======================================
  Coverage   95.91%   95.91%           
=======================================
  Files         223      223           
  Lines        9695     9699    +4     
=======================================
+ Hits         9299     9303    +4     
- Misses        326      327    +1     
+ Partials       70       69    -1

Impacted Files	Coverage Δ
cmd/collector/app/zipkin/http_handler.go	`100.00% <ø> (ø)`
plugin/storage/memory/memory.go	`100.00% <ø> (ø)`
cmd/agent/app/reporter/grpc/builder.go	`94.23% <80.00%> (-1.61%)`	⬇️
pkg/config/tlscfg/cert_watcher.go	`92.20% <0.00%> (-2.60%)`	⬇️
plugin/storage/integration/integration.go	`77.34% <0.00%> (-0.56%)`	⬇️
plugin/storage/badger/spanstore/reader.go	`96.08% <0.00%> (+0.71%)`	⬆️
cmd/query/app/server.go	`97.08% <0.00%> (+1.45%)`	⬆️

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 e4cacd0...c72be74. Read the comment docs.

Signed-off-by: Joe Elliott <[email protected]>

yurishkuro

Is model/model/model.pb.go file needed?

Makefile

yurishkuro · 2021-03-01T22:47:17Z

model/codec.go

+// Unmarshal implements encoding.Codec
+func (c *gogoCodec) Unmarshal(data []byte, v interface{}) error {
+	return proto.Unmarshal(data, v.(proto.Message))
+}


I assume this is already explicitly covered by the existing tests for marshalling.

Yes, all grpc/proto marshalling/unmarshalling will now go through this file.

Signed-off-by: Joe Elliott <[email protected]>

joe-elliott · 2021-03-02T19:09:23Z

Is model/model/model.pb.go file needed?

Nope. Has been removed.

Signed-off-by: Joe Elliott <[email protected]>

yurishkuro · 2021-03-02T19:41:37Z

proto-gen/api_v2/codec.go

@@ -0,0 +1,48 @@
+// Copyright (c) 2021 The Jaeger Authors.


maybe putting this into /model is better, because no other APIs can be used without model, but not vice versa.

I also prefer model, but this is where the cracks in this approach show. At this point what I have pushed works, but it is based on the ordering of the init() in two different packages. The init method in /proto-gen/api_v2/codec.go has to occur after this init:

https://github.com/grpc/grpc-go/blob/master/encoding/proto/proto.go#L33

The only package in Jaeger that eventually includes google.golang.org/grpc/encoding/proto is api_v2 so putting codec.go in this package guarantees that we are called second and gogoproto is the registered coded for proto with grpc. See this code to see the registration process.

What this means is that future, unrelated changes to Jaeger could magically break this by accidentally calling the grpc/encoding/proto init after our init. The good news is that CI would catch this, the bad news is that this is bad.

I don't hate this change. It allows us to stay up to date with OTel and grpc, but there are obvious drawbacks. I support either moving forward (with additional comments in codec.go to explain the above issue) or being patient with gogoproto for a few months to see if things resolve.

I am also fine with this, but I suggest literally pasting your explanation into the comments.

Also, if we move codec.go into model/, can we not add an _ import into it to ensure that grpc/encoding/proto is called first?

joe-elliott · 2021-03-02T20:21:20Z

github is not updating, but currently all tests are passing aside from "CIT Memory And Badger". It is failing with

integration.go:184: plugin error: rpc error: code = Unavailable desc = connection error: desc = "transport: error while dialing: dial unix /tmp/plugin047540702: connect: connection refused"

I will look into this when I get a chance.

yurishkuro · 2021-03-02T21:55:32Z

currently all tests are passing aside from "CIT Memory And Badger"

It would be exercising gRPC storage plugin infra. A bit concerning that it fails, since wire format / handshakes should not have changed with gRPC upgrade.

yurishkuro · 2021-03-02T22:03:09Z

I wonder if this has something to do with grpc-plugin module itself and which version of gRPC it usees. There isn't good logging from the test, unfortunately, my guess is that either start-up fails of the initial handshake fails

    integration.go:89: Waiting for storage backend to update documents, iteration 1 out of 30
    integration.go:184: grpc stream error: rpc error: code = Unavailable desc = transport is closing

Signed-off-by: Joe Elliott <[email protected]>

joe-elliott · 2021-03-05T21:10:40Z

Ok, I hit what may be a real blocker that requires either serious changes in the hashicorp go plugin or gogoproto. The memstore plugin is exiting with the below error. (Others are seeing similar issues.)

panic: protobuf tag not enough fields in Empty.state:

Which comes from this line in the gogo proto unmarshalling code:

https://github.com/gogo/protobuf/blob/b03c65ea87cdc3521ede29f62fe3ce239267c1bc/proto/table_unmarshal.go#L341

This is coming from hashicorps use of the Empty type here:

https://github.com/hashicorp/go-plugin/blob/0c19a1387af146eaa3335dad0a021741344ac3e9/internal/plugin/grpc_stdio.pb.go#L9

The empty type is now a wrapper around a google.golang.org/protobuf type here:

https://github.com/protocolbuffers/protobuf-go/blob/master/types/known/emptypb/empty.pb.go#L52

Note that the empty type has 3 metadata fields: state, sizeCache and unknownFields. These fields were formerly ??, XXX_sizecache and XXX_unrecognized.

So I can actually get tests to pass by ignoring these fields, but I have to directly edit gogo proto vendored code to do it. Tests pass by extending this line to include the new fields:

https://github.com/gogo/protobuf/blob/master/proto/table_unmarshal.go#L327

if f.Name == "XXX_NoUnkeyedLiteral" || f.Name == "XXX_sizecache" || f.Name == "state" || f.Name == "sizeCache" || f.Name == "unknownFields" {
    continue
}

I don't think I can take this any farther. The above change technically works, but doesn't feel like a valuable PR to gogoproto. I don't have any real, deep understanding of what this change does in the broader context of proto parsing.

pavolloffay · 2021-06-08T15:33:01Z

done in #3056

thanks @joe-elliott and @albertteoh for the prior work

joe-elliott added 11 commits February 12, 2021 11:00

upgraded root to google.golang.org/grpc 1.35.0

383bb29

Signed-off-by: Joe Elliott <[email protected]>

Use the right proto marshal/unmarshal

2c07b20

Signed-off-by: Joe Elliott <[email protected]>

Force coded to get tests to pass

fd90d5d

Signed-off-by: Joe Elliott <[email protected]>

generate with latest

ac5e095

Signed-off-by: Joe Elliott <[email protected]>

globally register stupidcodec

7d6c946

Signed-off-by: Joe Elliott <[email protected]>

added init

53a4d9d

Signed-off-by: Joe Elliott <[email protected]>

create a simple shim codec around gogoproto

e18692e

Signed-off-by: Joe Elliott <[email protected]>

lint

18363f7

Signed-off-by: Joe Elliott <[email protected]>

Merge branch 'master' into upgrade-grpc

7d33805

go mod tidy

33d3de7

Signed-off-by: Joe Elliott <[email protected]>

use otel proto builder

c7a8adf

Signed-off-by: Joe Elliott <[email protected]>

joe-elliott requested a review from a team as a code owner March 1, 2021 15:10

joe-elliott requested a review from yurishkuro March 1, 2021 15:10

mergify bot assigned jpkrohling Mar 1, 2021

mergify bot requested a review from jpkrohling March 1, 2021 15:10

joe-elliott added 3 commits March 1, 2021 16:09

>.>

b7c1795

Signed-off-by: Joe Elliott <[email protected]>

Moved codec to a more common place

9caaac2

Signed-off-by: Joe Elliott <[email protected]>

fmt

0266a41

Signed-off-by: Joe Elliott <[email protected]>

yurishkuro reviewed Mar 1, 2021

View reviewed changes

joe-elliott added 2 commits March 1, 2021 20:13

removed unnecessary model

9f1d8d0

Signed-off-by: Joe Elliott <[email protected]>

moving codec back!

4a83a0b

Signed-off-by: Joe Elliott <[email protected]>

joe-elliott added 2 commits March 2, 2021 14:29

Added codec

7c501cc

Signed-off-by: Joe Elliott <[email protected]>

go mod tidY

a806ea0

Signed-off-by: Joe Elliott <[email protected]>

yurishkuro reviewed Mar 2, 2021

View reviewed changes

Use jaegertracing proto builder

c72be74

Signed-off-by: Joe Elliott <[email protected]>

yurishkuro mentioned this pull request Mar 4, 2021

Bump github.com/prometheus/client_golang from 1.5.1 to 1.9.0 #2852

Closed

joe-elliott mentioned this pull request Mar 5, 2021

gogo/proto marshalling is failing with latest github.com/golang/protobuf v1.4.0-rc.4 gogo/protobuf#678

Open

yurishkuro changed the title ~~Upgrade GRPC/Proto to Latest~~ WIP - Upgrade GRPC/Proto to Latest May 15, 2021

joe-elliott mentioned this pull request Jun 3, 2021

grpc: Server.Serve failed to create ServerTransport #2172

Closed

pavolloffay mentioned this pull request Jun 4, 2021

Upgrade gRPC to 1.38.x #3056

Merged

pavolloffay closed this Jun 8, 2021

leoluz mentioned this pull request Apr 5, 2022

chore: update grpc runtime library argoproj/argo-cd#8929

Merged

10 tasks

littlejawa mentioned this pull request Nov 10, 2022

Fix issue in gogo/protobuf code, causing a crash in cri-o. cri-o/cri-o#6371

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP - Upgrade GRPC/Proto to Latest #2857

WIP - Upgrade GRPC/Proto to Latest #2857

joe-elliott commented Mar 1, 2021 •

edited

Loading

codecov bot commented Mar 1, 2021 •

edited

Loading

yurishkuro left a comment

yurishkuro Mar 1, 2021

joe-elliott Mar 2, 2021

joe-elliott commented Mar 2, 2021 •

edited

Loading

yurishkuro Mar 2, 2021

joe-elliott Mar 2, 2021

yurishkuro Mar 2, 2021

joe-elliott commented Mar 2, 2021

yurishkuro commented Mar 2, 2021

yurishkuro commented Mar 2, 2021

joe-elliott commented Mar 5, 2021 •

edited

Loading

pavolloffay commented Jun 8, 2021

WIP - Upgrade GRPC/Proto to Latest #2857

WIP - Upgrade GRPC/Proto to Latest #2857

Conversation

joe-elliott commented Mar 1, 2021 • edited Loading

Which problem is this PR solving?

Short description of the changes

codecov bot commented Mar 1, 2021 • edited Loading

Codecov Report

yurishkuro left a comment

Choose a reason for hiding this comment

yurishkuro Mar 1, 2021

Choose a reason for hiding this comment

joe-elliott Mar 2, 2021

Choose a reason for hiding this comment

joe-elliott commented Mar 2, 2021 • edited Loading

yurishkuro Mar 2, 2021

Choose a reason for hiding this comment

joe-elliott Mar 2, 2021

Choose a reason for hiding this comment

yurishkuro Mar 2, 2021

Choose a reason for hiding this comment

joe-elliott commented Mar 2, 2021

yurishkuro commented Mar 2, 2021

yurishkuro commented Mar 2, 2021

joe-elliott commented Mar 5, 2021 • edited Loading

pavolloffay commented Jun 8, 2021

joe-elliott commented Mar 1, 2021 •

edited

Loading

codecov bot commented Mar 1, 2021 •

edited

Loading

joe-elliott commented Mar 2, 2021 •

edited

Loading

joe-elliott commented Mar 5, 2021 •

edited

Loading