Support gRPC for query service #1307

annanay25 · 2019-01-29T19:22:31Z

Which problem is this PR solving?

Initial work to add GRPC support for the query-service.

Short description of the changes

Use grpc-gateway to multiplex HTTP and GRPC on the same port
Create GRPC server and handler for GetTrace()

Signed-off-by: Annanay annanay.a@media.net

cmd/query/app/grpc_handler.go

yurishkuro · 2019-01-30T18:36:47Z

model/proto/api_v2.proto

@@ -64,6 +64,22 @@ service CollectorService {
    }
 }

+message GetTraceRequest {


this proto file is getting very busy. Could you try to move the query service to api_v2_query.proto? Or even better api_v2/query.proto

Sure, will move this out.

cmd/query/app/grpc_handler.go

cmd/query/main.go

annanay25

Thanks for the review! @yurishkuro

I first wanted to ensure all the necessary proto definitions are in place.

cmd/query/app/grpc_handler.go

annanay25 · 2019-01-31T07:59:58Z

model/proto/api_v2.proto

@@ -64,6 +64,22 @@ service CollectorService {
    }
 }

+message GetTraceRequest {


Sure, will move this out.

model/proto/api_v2.proto

cmd/query/main.go

yurishkuro · 2019-02-22T15:49:06Z

model/proto/api_v2.proto

+
+message GetTraceResponse {
+    Trace trace = 1;
+}


check the discussion in #1323. We have an internal grpc implementation of GetTrace, and our users have run into the issue of large traces exceeding grpc default message size. So I think we need to go with streaming APIs for methods that can return large data sets.

I am not sure what it would do to grpc_gateway, how does it map streaming calls to HTTP?

Also, it's been a while since the last time I checked, but if gRPC for JavaScript in the browser has advanced enough, then we won't even need grpc_gateway.

I am not sure what it would do to grpc_gateway, how does it map streaming calls to HTTP?

Seems like they have an open issue for sending chunked JSON responses - grpc-ecosystem/grpc-gateway/issues/562

Also, it's been a while since the last time I checked, but if gRPC for JavaScript in the browser has advanced enough, then we won't even need grpc_gateway.

Apart from streaming-support (which is on their roadmap) what other features would we need in the grpc-web library?

Update: Went through the grpc-web readme again, and it mentions that server-side streaming is already supported. This makes it a candidate to be considered for the query UI.

Great! We can always put a blocking/sync facade in front of grpc service if streaming to web is not supported, but since it's there already, we should go with streaming for the main API.

annanay25 · 2019-02-26T19:00:46Z

I would like a second review round @yurishkuro

Changes in the last commit -

Dropped grpc-gateway
Used cmux to multiplex HTTP and GRPC
Used stream response for GetTraceRequest

Todo -

Add all endpoints including FindTraceIDs etc.
Add tests.

model/proto/api_v2.proto

model/proto/api_v2/query.proto

annanay25 · 2019-03-31T18:46:59Z

model/proto/api_v2/query.proto

+
+// FIXME: Pass all TraceQueryParameters in SearchRequest?
+message SearchRequest {
+  string query = 1 [


Question: Should parameters be parsed from a string query or should the SearchRequest have all TraceQueryParameters as in -

jaeger/storage/spanstore/interface.go

Lines 45 to 54 in 65a499a

type TraceQueryParameters struct {

ServiceName string

OperationName string

Tags map[string]string

StartTimeMin time.Time

StartTimeMax time.Time

DurationMin time.Duration

DurationMax time.Duration

NumTraces int

}

I guess the former because protobuf supports just basic data types and not time/maps etc?

it should be structured, as in #1323

Thanks! I've made the changes.

yurishkuro

please try to match the naming & ordering with https://github.com/jaegertracing/jaeger/pull/1323/files#diff-ac4433b4ef3a403411d1d1b5141eb9ccR121

model/proto/api_v2/query.proto

Signed-off-by: Annanay <annanay.a@media.net>

…er doc Signed-off-by: Annanay <annanay.a@media.net>

Signed-off-by: Annanay <annanay.a@media.net>

cmd/query/main.go

model/proto/api_v2/query.proto

model/prototest/model_test.pb.go

Signed-off-by: Annanay <annanay.a@media.net>

annanay25 · 2019-04-06T18:56:23Z

Thanks for the review @yurishkuro. I've addressed comments and updated.

Signed-off-by: Annanay <annanay.a@media.net>

Signed-off-by: Yuri Shkuro <ys@uber.com>

codecov · 2019-04-08T02:14:37Z

Codecov Report

Merging #1307 into master will decrease coverage by 0.02%.
The diff coverage is 97.29%.

@@            Coverage Diff             @@
##           master    #1307      +/-   ##
==========================================
- Coverage   99.83%   99.81%   -0.03%     
==========================================
  Files         179      180       +1     
  Lines        8557     8631      +74     
==========================================
+ Hits         8543     8615      +72     
- Misses          7        8       +1     
- Partials        7        8       +1

Impacted Files	Coverage Δ
cmd/query/app/grpc_handler.go	`97.29% <97.29%> (ø)`

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 7009c98...8dfc90a. Read the comment docs.

Signed-off-by: Annanay <annanay.a@media.net>

annanay25 · 2019-04-09T04:52:47Z

Thanks for the changes @yurishkuro!

About tests, should I use a mock query service (as in the current commit) or an instance of query service with mock span readers / dependency readers?

yurishkuro · 2019-04-09T16:29:11Z

mock QS is fine

Signed-off-by: Annanay <annanay.a@media.net>

annanay25 · 2019-04-16T18:03:59Z

@yurishkuro could you please review this? I think the last remaining lines from coverage are stream sends, which I'm not sure how to test.

Signed-off-by: Yuri Shkuro <ys@uber.com>

yurishkuro · 2019-04-22T14:34:35Z

🎉

There is a bit of a clean-up I'd like to do in the test, I think it's leaking goroutines, but didn't want to block the PR on that.

yurishkuro

@annanay25 post-merge comments / improvements

yurishkuro · 2019-04-22T14:35:18Z

cmd/query/app/grpc_handler_test.go

+)
+
+var (
+	grpcServerPort    = ":14251"


we should be using :0 port

yurishkuro · 2019-04-22T14:35:39Z

cmd/query/app/grpc_handler_test.go

+	go func() {
+		err := grpcServer.Serve(lis)
+		require.NoError(t, err)
+	}()


goroutine leak? Or will defer server.Stop() in the tests stop it?

yurishkuro · 2019-04-22T14:41:07Z

cmd/query/app/grpc_handler_test.go

+	return grpcServer, addr, readStorage, dependencyStorage
+}
+
+func initializeTestServerGRPCWithOptions(t *testing.T) (*grpc.Server, net.Addr, *spanstoremocks.Reader, *depsmocks.Reader, *spanstoremocks.Reader, *spanstoremocks.Writer) {


a) any reason not to merge this with the above function?
b) instead of returning many objects it's better to return a single struct
c) to avoid server life cycle management, we typically do this

func withGRPCServer(t *testing.T, actualTest func(server *server)) { server := ... create ... defer server.close() // other life cycle mgmt actualTest(server) }

Booked #1483

yurishkuro · 2019-04-22T14:43:16Z

cmd/query/app/grpc_handler_test.go

+		Return(mockTrace, nil).Once()
+
+	client, conn := newGRPCClient(t, addr)
+	defer conn.Close()


similar to above, withServerAndClient(t *testing.T, func actualTest(server *server, client *client))

this way it would remove a lot of boilerplate code from the tests

yurishkuro · 2019-04-22T14:47:11Z

cmd/query/main.go

+			grpcL := s.Match(
+				cmux.HTTP2HeaderField("content-type", "application/grpc"),
+				cmux.HTTP2HeaderField("content-type", "application/grpc+proto"))
+			httpL := s.Match(cmux.Any())


Booked #1482 to refactor this out of main

annanay25 · 2019-04-23T10:53:50Z

Thanks @yurishkuro! 🎊

I've been traveling but should get a chance to look at this soon :)

Edit: We should now revisit the implementation roadmap in #773

annanay25 requested review from black-adder, jpkrohling, objectiser, pavolloffay, tiffon, vprithvi and yurishkuro as code owners January 29, 2019 19:22

yurishkuro reviewed Jan 31, 2019

View reviewed changes

annanay25 commented Jan 31, 2019

View reviewed changes

vprithvi reviewed Jan 31, 2019

View reviewed changes

model/proto/api_v2.proto Outdated Show resolved Hide resolved

annanay25 mentioned this pull request Feb 1, 2019

Separate query-service functionality from http handler #1312

Merged

pavolloffay mentioned this pull request Feb 12, 2019

Release 1.10 #1321

Closed

4 tasks

annanay25 commented Feb 20, 2019

View reviewed changes

cmd/query/main.go Outdated Show resolved Hide resolved

yurishkuro reviewed Feb 22, 2019

View reviewed changes

annanay25 force-pushed the query-service-grpc branch from 287fb43 to 2d276fa Compare February 26, 2019 18:55

annanay25 commented Feb 26, 2019

View reviewed changes

model/proto/api_v2.proto Outdated Show resolved Hide resolved

annanay25 mentioned this pull request Mar 1, 2019

Define project layout for Protobuf / gRPC / generated files #904

Closed

ghost assigned yurishkuro Mar 15, 2019

ghost added the review label Mar 15, 2019

yurishkuro reviewed Mar 27, 2019

View reviewed changes

model/proto/api_v2/query.proto Show resolved Hide resolved

annanay25 commented Mar 31, 2019

View reviewed changes

yurishkuro reviewed Apr 2, 2019

View reviewed changes

model/proto/api_v2/query.proto Outdated Show resolved Hide resolved

model/proto/api_v2/query.proto Outdated Show resolved Hide resolved

Annanay added 7 commits April 3, 2019 22:39

Initial commit, grpc for query-service

ab4d728

Signed-off-by: Annanay <annanay.a@media.net>

Re-evaluate fields in GRPCHandler

650fc55

Signed-off-by: Annanay <annanay.a@media.net>

Use cmux, use stream in GetTraceResponse, regenerate *pb.go and swagg…

30033df

…er doc Signed-off-by: Annanay <annanay.a@media.net>

Edit GetTrace HTTP verb

6467d44

Signed-off-by: Annanay <annanay.a@media.net>

Re-add query definitions in a new file

5a65f9f

Signed-off-by: Annanay <annanay.a@media.net>

Address comments, correct API endpoint for query grpc

9dcfe08

Signed-off-by: Annanay <annanay.a@media.net>

Add definitions for other query functions

114ddba

Signed-off-by: Annanay <annanay.a@media.net>

yurishkuro reviewed Apr 4, 2019

View reviewed changes

Addressed comments, fix proto tool versions

4a85f6f

Signed-off-by: Annanay <annanay.a@media.net>

Annanay and others added 7 commits April 7, 2019 17:29

Fix build

b03b497

Signed-off-by: Annanay <annanay.a@media.net>

Fix build

6ea3b82

Signed-off-by: Annanay <annanay.a@media.net>

Remove grpc-gateway code generation; use streaming

64b57da

Signed-off-by: Yuri Shkuro <ys@uber.com>

Implement streaming

8a6f286

Signed-off-by: Yuri Shkuro <ys@uber.com>

make fmt lint

63d5638

Signed-off-by: Yuri Shkuro <ys@uber.com>

Merge branch 'master' into query-service-grpc

8d2535d

dep update

289385b

Signed-off-by: Yuri Shkuro <ys@uber.com>

Start adding tests

86a771f

Signed-off-by: Annanay <annanay.a@media.net>

Add tests for GRPC handler

86d770b

Signed-off-by: Annanay <annanay.a@media.net>

annanay25 force-pushed the query-service-grpc branch from b3b446b to 86d770b Compare April 12, 2019 20:12

Annanay added 4 commits April 13, 2019 16:43

Fix grpc handler tests

e7f0e0d

Signed-off-by: Annanay <annanay.a@media.net>

WIP fixing tests

612dc13

Signed-off-by: Annanay <annanay.a@media.net>

Fix tests

9c3f1ee

Signed-off-by: Annanay <annanay.a@media.net>

Fix tests

c1d9abe

Signed-off-by: Annanay <annanay.a@media.net>

annanay25 changed the title ~~WIP - gRPC for query service~~ Support gRPC for query service Apr 16, 2019

Yuri Shkuro and others added 3 commits April 16, 2019 19:07

Add test

5a32ba0

Signed-off-by: Yuri Shkuro <ys@uber.com>

Merge branch 'master' into query-service-grpc

b61fc18

Signed-off-by: Yuri Shkuro <ys@uber.com>

Merge branch 'master' into query-service-grpc

8dfc90a

yurishkuro merged commit 5b8c1f4 into jaegertracing:master Apr 22, 2019

yurishkuro reviewed Apr 22, 2019

View reviewed changes

yurishkuro mentioned this pull request Apr 22, 2019

[query] Simplify gRPC server tests #1483

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support gRPC for query service #1307

Support gRPC for query service #1307

annanay25 commented Jan 29, 2019

yurishkuro Jan 30, 2019

annanay25 Jan 31, 2019

annanay25 left a comment

annanay25 Jan 31, 2019

yurishkuro Feb 22, 2019

annanay25 Feb 23, 2019

annanay25 Feb 25, 2019

yurishkuro Feb 25, 2019

annanay25 commented Feb 26, 2019

annanay25 Mar 31, 2019

yurishkuro Mar 31, 2019

annanay25 Apr 2, 2019

yurishkuro left a comment

annanay25 commented Apr 6, 2019

codecov bot commented Apr 8, 2019 •

edited

Loading

annanay25 commented Apr 9, 2019

yurishkuro commented Apr 9, 2019 •

edited

Loading

annanay25 commented Apr 16, 2019

yurishkuro commented Apr 22, 2019

yurishkuro left a comment

yurishkuro Apr 22, 2019

yurishkuro Apr 22, 2019

yurishkuro Apr 22, 2019

yurishkuro Apr 22, 2019

yurishkuro Apr 22, 2019

yurishkuro Apr 22, 2019

annanay25 commented Apr 23, 2019 •

edited

Loading

	type TraceQueryParameters struct {
	ServiceName string
	OperationName string
	Tags map[string]string
	StartTimeMin time.Time
	StartTimeMax time.Time
	DurationMin time.Duration
	DurationMax time.Duration
	NumTraces int
	}

Support gRPC for query service #1307

Support gRPC for query service #1307

Conversation

annanay25 commented Jan 29, 2019

Which problem is this PR solving?

Short description of the changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annanay25 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

annanay25 commented Feb 26, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurishkuro left a comment

Choose a reason for hiding this comment

annanay25 commented Apr 6, 2019

codecov bot commented Apr 8, 2019 • edited Loading

Codecov Report

annanay25 commented Apr 9, 2019

yurishkuro commented Apr 9, 2019 • edited Loading

annanay25 commented Apr 16, 2019

yurishkuro commented Apr 22, 2019

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

annanay25 commented Apr 23, 2019 • edited Loading

codecov bot commented Apr 8, 2019 •

edited

Loading

yurishkuro commented Apr 9, 2019 •

edited

Loading

annanay25 commented Apr 23, 2019 •

edited

Loading