Add gRPC communication between agent and collector #1165

pavolloffay · 2018-11-08T13:56:49Z

Resolves some parts of #773

Rebased and modified #903 and #1042

grpc server on the collector and reporter on the agent side
sampling strategies over grpc
grpc flags are marked as experimental. By default is used tchannel
xdock uses grpc

codecov · 2018-11-08T14:43:35Z

Codecov Report

Merging #1165 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master   #1165    +/-   ##
=======================================
  Coverage     100%    100%            
=======================================
  Files         144     154    +10     
  Lines        6839    7034   +195     
=======================================
+ Hits         6839    7034   +195

Impacted Files	Coverage Δ
cmd/agent/app/reporter/grpc/reporter.go	`100% <100%> (ø)`
cmd/agent/app/reporter/grpc/flags.go	`100% <100%> (ø)`
...el/converter/thrift/jaeger/sampling_from_domain.go	`100% <100%> (ø)`
cmd/agent/app/httpserver/server.go	`100% <100%> (ø)`	⬆️
cmd/agent/app/reporter/grpc/sampling_manager.go	`100% <100%> (ø)`
cmd/collector/app/builder/span_handler_builder.go	`100% <100%> (ø)`	⬆️
cmd/collector/app/builder/builder_flags.go	`100% <100%> (ø)`	⬆️
cmd/collector/app/grpc_handler.go	`100% <100%> (ø)`
cmd/collector/app/grpcserver/grpc_server.go	`100% <100%> (ø)`
cmd/agent/app/flags.go	`100% <100%> (ø)`	⬆️
... and 14 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 f2eb7d1...955ab84. Read the comment docs.

pavolloffay · 2018-11-08T14:55:55Z

model/proto/api_v2.proto

+    bool ok = 1;
+}
+
+service CollectorService {


For now I have excluded query service. Let's do it in a separate PR.

Makefile

yurishkuro · 2018-11-08T15:11:13Z

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

+
+const (
+	gRPCPrefix        = "reporter.grpc."
+	collectorHostPort = "collector.host-port"


Is this different from the old reporter? I thought that one supported a list of addresses.

grpc dial supports only one address by default. An option is to use with balancer https://stackoverflow.com/a/50302295/4158442 not sure what that implies though.

the load balancer implementation can be done later, I'm just asking whether the cmd line should support a list of addresses (it can fail for now on >1 address).

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

cmd/all-in-one/main.go

Makefile

yurishkuro · 2018-11-08T21:51:36Z

cmd/all-in-one/main.go

-	if err != nil {
-		logger.Fatal("Unable to start listening on channel", zap.Error(err))
+
+	{


There is a lot of duplication with the collector. It would be good to have all verbose initialization code in a shared package where it can be tested rather than in main.

I will try to move it in some package in collector. The same applies for tchannel there is duplication even in the current master.

cmd/all-in-one/main.go

yurishkuro · 2018-11-08T21:53:43Z

cmd/collector/app/grpc_handler.go

+}
+
+// PostSpans implements gRPC CollectorService.
+func (g *GRPCHandler) PostSpans(ctx context.Context, r *api_v2.PostSpansRequest) (*api_v2.PostSpansResponse, error) {


do we need new metrics here?

cmd/collector/app/grpc_handler_test.go

yurishkuro · 2018-11-08T21:59:28Z

model/proto/api_v2.proto

+}
+
+service CollectorService {
+    rpc PostSpans(PostSpansRequest) returns (PostSpansResponse) {


I feel like we should be using streaming in this API... Unless we expect the agent to resubmit to another collector on error (worth checking what OC agent is planning to do, those folks have a lot more experience with grpc).

PS can be addressed in another PR, we should merge the infra first.

If it can I would prefer.

pavolloffay · 2018-11-09T15:33:15Z

Outstanding tasks:

metrics
rpc/stream for posting spans
dedupe process - pass process to reporter.send() in agent - avoid converting every process
Revisit postSpans response
close grpc server and connections

I would prefer to do it in a separate PR if possible.

yurishkuro · 2018-11-09T18:44:17Z

I would prefer to do it in a separate PR if possible.

That's fine with me (I'd prefer that too, this diff is already too large).

But the main question we need to answer is what to do w.r.t. releasing 1.8. If there are any user-facing API changes (like supporting multiple addresses may change the cli flag name), then once we merge this PR we'll need to finish the outstanding tasks before cutting 1.8. Thoughts?

pavolloffay · 2018-11-12T09:35:17Z

Originally I wanted to cut 1.8 without gRPC to have more baking time in master before releasing it. I will have a look at the list of addresses.

pavolloffay · 2018-11-12T14:52:05Z

@yurishkuro I have added load balancer support in the last commit. If you prefer I can revert it, supporting a list of collectors in the same flag is back compatible anyways.

yurishkuro · 2018-11-12T17:23:47Z

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

+			resolvedAddrs = append(resolvedAddrs, resolver.Address{Addr: addr})
+		}
+		r.InitialAddrs(resolvedAddrs)
+		conn, _ = grpc.Dial(r.Scheme()+":///jaeger.collector", grpc.WithInsecure(), grpc.WithBalancerName(roundrobin.Name))


what is the significance of ":///jaeger.collector" string?

Actually I don't know, It seems that it can be any string after :///

https://github.com/grpc/grpc-go/blob/master/balancer/roundrobin/roundrobin_test.go#L136

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

cmd/agent/main.go

pavolloffay · 2018-11-12T19:28:38Z

I will merge after 1.8 is out

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

pavolloffay requested review from black-adder, jpkrohling, objectiser, tiffon, vprithvi and yurishkuro as code owners November 8, 2018 13:56

ghost assigned pavolloffay Nov 8, 2018

ghost added the review label Nov 8, 2018

pavolloffay commented Nov 8, 2018

View reviewed changes

isaachier reviewed Nov 8, 2018

View reviewed changes

Makefile Show resolved Hide resolved

yurishkuro reviewed Nov 8, 2018

View reviewed changes

pavolloffay force-pushed the grpc-post-spans-rebased branch from 4375b29 to 1d31e81 Compare November 12, 2018 09:37

yurishkuro reviewed Nov 12, 2018

View reviewed changes

cmd/agent/app/reporter/grpc/flags.go Outdated Show resolved Hide resolved

yurishkuro approved these changes Nov 12, 2018

View reviewed changes

cmd/agent/main.go Show resolved Hide resolved

pavolloffay added 9 commits November 13, 2018 12:14

Add gRPC communication between agent and collector

e238edf

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

uncomment make and fix comment

a5b633d

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

Fix grpc handler initialization

3eeab86

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

Copy process if missing

582e4d7

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

log only warn and error from grpc

6afafb0

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

Fix review comments

ba542e5

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

Add todo

b530e87

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

Add Roundrobin grpc load balancer

0066cdd

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

Fix comment

0d198be

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

pavolloffay added 4 commits November 13, 2018 12:14

Fix review comments

86e4765

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

Change grpc target name

dff8e52

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

Defaul return error

9a89d4d

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

Fix back tchannel collector addr

b78e314

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

pavolloffay force-pushed the grpc-post-spans-rebased branch from 0416497 to b78e314 Compare November 13, 2018 11:14

Remove collector from grpc reporter flag

955ab84

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

pavolloffay merged commit 9635a33 into jaegertracing:master Nov 13, 2018

ghost removed the review label Nov 13, 2018

This was referenced Nov 13, 2018

WIP: Sample gRPC server #903

Closed

gRPC post spans #1042

Closed

Dedupe process in grpc reporter #1181

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add gRPC communication between agent and collector #1165

Add gRPC communication between agent and collector #1165

pavolloffay commented Nov 8, 2018 •

edited

Loading

codecov bot commented Nov 8, 2018 •

edited

Loading

pavolloffay Nov 8, 2018

yurishkuro Nov 8, 2018

pavolloffay Nov 9, 2018

yurishkuro Nov 9, 2018

yurishkuro Nov 8, 2018

pavolloffay Nov 9, 2018

yurishkuro Nov 8, 2018

yurishkuro Nov 8, 2018

yurishkuro Nov 9, 2018

pavolloffay Nov 9, 2018

pavolloffay commented Nov 9, 2018 •

edited

Loading

yurishkuro commented Nov 9, 2018

pavolloffay commented Nov 12, 2018

pavolloffay commented Nov 12, 2018

yurishkuro Nov 12, 2018

pavolloffay Nov 12, 2018

pavolloffay commented Nov 12, 2018

Add gRPC communication between agent and collector #1165

Add gRPC communication between agent and collector #1165

Conversation

pavolloffay commented Nov 8, 2018 • edited Loading

codecov bot commented Nov 8, 2018 • edited Loading

Codecov Report

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

pavolloffay commented Nov 9, 2018 • edited Loading

yurishkuro commented Nov 9, 2018

pavolloffay commented Nov 12, 2018

pavolloffay commented Nov 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavolloffay commented Nov 12, 2018

pavolloffay commented Nov 8, 2018 •

edited

Loading

codecov bot commented Nov 8, 2018 •

edited

Loading

pavolloffay commented Nov 9, 2018 •

edited

Loading