gRPC post spans #1042

isaachier · 2018-09-04T00:40:45Z

Add PostSpans functionality to gRPC branch.

Signed-off-by: Isaac Hier <isaachier@gmail.com>

codecov · 2018-09-04T01:15:20Z

Codecov Report

Merging #1042 into grpc will increase coverage by 0.13%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##             grpc    #1042      +/-   ##
==========================================
+ Coverage   99.67%   99.81%   +0.13%     
==========================================
  Files         140      139       -1     
  Lines        6424     6403      -21     
==========================================
- Hits         6403     6391      -12     
+ Misses         21       12       -9

Impacted Files	Coverage Δ
cmd/collector/app/grpc_handler.go	`55.55% <100%> (+55.55%)`	⬆️
cmd/ingester/app/flags.go

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 aa9059d...1222ead. Read the comment docs.

black-adder · 2018-09-05T01:52:14Z

cmd/collector/app/grpc_handler_test.go

+	lis, err := net.Listen("tcp", "localhost:0")
+	require.NoError(t, err)
+	go func() {
+		err := server.Serve(lis)


doesn't this introduce a potential race condition where the client is initialized before the server is ready?

Good point.

Actually, I believe that this is somewhat unavoidable. The issue is that Serve will block indefinitely. Using a sync.WaitGroup would help, if I could call the Done method before the the server blocks, but there is no good place to do that.

black-adder · 2018-09-05T01:53:15Z

cmd/collector/app/grpc_handler_test.go

+	defer server.Stop()
+	client, conn := newClient(t, addr)
+	defer conn.Close()
+	ctx, cancel := context.WithTimeout(context.Background(), time.Second)


can't we just use context.Background()?

Maybe timeout helps with the race when client starts before server. Although, the conn car above, is it already open?

Fair enough. I think this originally helped me when I had an issue with client hanging forever. Now that it's fixed, can remove this.

black-adder · 2018-09-05T01:55:54Z

cmd/collector/app/grpc_handler_test.go

+func (p *mockSpanProcessor) ProcessSpans(spans []*model.Span, spanFormat string) ([]bool, error) {
+	p.mux.Lock()
+	defer p.mux.Unlock()
+	p.spans = append(p.spans, spans...)


is this all necessary? It looks like your tests call ProcessSpans just once, the extra complexity (mutex, keeping track of spans, etc) doesn't look required.

I think it's needed since read and write are in different goroutines

Yes. Luckily for me, the only code I had to write here was essentially copied from the equivalent Thrift test. It does locking the same way IIRC.

black-adder · 2018-09-05T01:57:01Z

cmd/collector/app/grpc_handler_test.go

+	})
+	require.NoError(t, err)
+	require.False(t, r.GetOk())
+	require.Len(t, processor.getSpans(), 1)


so when you do this, you're essentially testing a function on the mock, kinda weird.

It tests sending spans over the wire, no?

Ya again, Thrift handler does this.

I mean if spans go over the wire then we're not just testing mock, we're testing the transmission and marshalling.

Yes exactly.

yurishkuro · 2018-09-05T14:01:49Z

cmd/collector/app/grpc_handler_test.go

+	return p.spans
+}
+
+func initializeGRPCTestServer(t *testing.T, err error) (*grpc.Server, *mockSpanProcessor, net.Addr) {


Nit: expectedError

yurishkuro · 2018-09-05T14:04:59Z

cmd/collector/app/grpc_handler.go

 	}
-	return &api_v2.PostSpansResponse{Ok: true}, nil
+	return &api_v2.PostSpansResponse{Ok: success}, nil


We may want to rethink this API. What are we trying to communicate back to the caller with this Boolean flag? At least (accepted, rejected) counts would make more sense. When can the processor return not ok? Should it be returning specific errors instead?

I agree that the API as it stands isn't exactly perfect. If anything, a boolean vector (or even bit vector) might make more sense here. Count works too if we don't care which spans failed. An optional error message might be better than the boolean response.

Any thoughts @yurishkuro?

The question is: what is the sender going to do with the errors?

Overall, I agree with you here. The only reason to provide an error is if we believe in some far-fetched scenario it would be retried. Also, an empty response sort of seems weird, but IDK what else to do here.

if we want to support retries we should be using gRPC error code for that. Can you check what openCensus is doing in this case?

yurishkuro · 2018-09-05T14:06:18Z

cmd/collector/app/grpc_handler_test.go

+func (p *mockSpanProcessor) ProcessSpans(spans []*model.Span, spanFormat string) ([]bool, error) {
+	p.mux.Lock()
+	defer p.mux.Unlock()
+	p.spans = append(p.spans, spans...)


I think it's needed since read and write are in different goroutines

yurishkuro · 2018-09-05T14:10:14Z

cmd/collector/app/grpc_handler_test.go

+
+func TestPostSpansWithError(t *testing.T) {
+	processorErr := errors.New("test-error")
+	server, processor, addr := initializeGRPCTestServer(t, processorErr)


We should simplify this. You can return a test server struct with fields instead of multiple values. And we can rewrite this in FP style withServer(params, func) to hide all the client/server complexity from the test.

Sure. Again, sort of copied this stuff. Can easily clean this up a bit.

yurishkuro · 2018-09-05T14:11:51Z

cmd/collector/app/grpc_handler_test.go

+	defer server.Stop()
+	client, conn := newClient(t, addr)
+	defer conn.Close()
+	ctx, cancel := context.WithTimeout(context.Background(), time.Second)


Maybe timeout helps with the race when client starts before server. Although, the conn car above, is it already open?

yurishkuro · 2018-09-05T14:13:05Z

cmd/collector/app/grpc_handler_test.go

+	})
+	require.NoError(t, err)
+	require.False(t, r.GetOk())
+	require.Len(t, processor.getSpans(), 1)


It tests sending spans over the wire, no?

Signed-off-by: Isaac Hier <ihier@uber.com>

yurishkuro · 2018-10-08T23:01:26Z

cmd/collector/app/grpc_handler.go

-	fmt.Printf("PostSpans(%+v)\n", *r)
-	for _, s := range r.Batch.Spans {
-		println(s.OperationName)
+	oks, err := g.spanProcessor.ProcessSpans(r.GetBatch().Spans, JaegerFormatType)


I think this is losing Batch.Process data.

So the spans have pointers to the process. I'm not sure when that back-reference is set. If it is already set, there should not be a problem.

by convention when a bunch of spans arrive in a Batch and logically they are all from the same Process then Span.Process will be nil, but Batch.Process will be defined. This is done to minimize the amount of data sent over the wire. So you need to loop through spans and copy the Process pointer from Batch if Span.Process == nil

pavolloffay · 2018-11-13T11:42:54Z

done in #1165

isaachier requested review from black-adder, jpkrohling, pavolloffay, vprithvi and yurishkuro as code owners September 4, 2018 00:40

Implement PostSpans for collector gRPC handler

0db4f12

Signed-off-by: Isaac Hier <isaachier@gmail.com>

isaachier force-pushed the grpc-post-spans branch from eb0a65a to 0db4f12 Compare September 4, 2018 00:42

Fix formatting

98a8e6c

Signed-off-by: Isaac Hier <isaachier@gmail.com>

black-adder reviewed Sep 5, 2018

View reviewed changes

yurishkuro reviewed Sep 5, 2018

View reviewed changes

Isaac Hier added 2 commits September 28, 2018 13:07

Remove timeout

5bd52b4

Signed-off-by: Isaac Hier <ihier@uber.com>

Use expectedError variable

1222ead

Signed-off-by: Isaac Hier <ihier@uber.com>

yurishkuro reviewed Oct 8, 2018

View reviewed changes

This was referenced Nov 7, 2018

Use Protobuf and gRPC for internal communications #773

Open

Add gRPC communication between agent and collector #1165

Merged

pavolloffay closed this Nov 13, 2018

isaachier deleted the grpc-post-spans branch November 28, 2018 00:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gRPC post spans #1042

gRPC post spans #1042

isaachier commented Sep 4, 2018

codecov bot commented Sep 4, 2018 •

edited

Loading

black-adder Sep 5, 2018

isaachier Sep 17, 2018

isaachier Sep 17, 2018

black-adder Sep 5, 2018

yurishkuro Sep 5, 2018

isaachier Sep 17, 2018

black-adder Sep 5, 2018

yurishkuro Sep 5, 2018

isaachier Sep 17, 2018

black-adder Sep 5, 2018

yurishkuro Sep 5, 2018

isaachier Sep 17, 2018

yurishkuro Oct 8, 2018 •

edited

Loading

isaachier Oct 9, 2018

yurishkuro Sep 5, 2018

isaachier Sep 17, 2018

yurishkuro Sep 5, 2018

isaachier Sep 17, 2018

isaachier Sep 30, 2018

yurishkuro Oct 8, 2018

isaachier Oct 9, 2018

yurishkuro Oct 9, 2018

yurishkuro Sep 5, 2018

yurishkuro Sep 5, 2018

isaachier Sep 17, 2018

yurishkuro Sep 5, 2018

yurishkuro Sep 5, 2018

yurishkuro Oct 8, 2018 •

edited

Loading

isaachier Oct 9, 2018

yurishkuro Oct 9, 2018

pavolloffay commented Nov 13, 2018

gRPC post spans #1042

gRPC post spans #1042

Conversation

isaachier commented Sep 4, 2018

codecov bot commented Sep 4, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurishkuro Oct 8, 2018 • 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

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

yurishkuro Oct 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavolloffay commented Nov 13, 2018

codecov bot commented Sep 4, 2018 •

edited

Loading

yurishkuro Oct 8, 2018 •

edited

Loading

yurishkuro Oct 8, 2018 •

edited

Loading