Add gRPC plugin proto changes #1323

chvck · 2019-02-08T16:57:10Z

Which problem is this PR solving?

Issue How to support plugins #422, part of review WIP - gRPC Plugin framework #1214

Short description of the changes

Split proto and Makefile changes out from previous review to enable further progress there.

chvck · 2019-02-11T19:01:46Z

plugin/storage/grpc/proto/storage.proto

@@ -0,0 +1,199 @@
+syntax = "proto3";
+
+package jaeger.api_v2;


Unsure that this is correct.

I think jaeger.storage.v1

chvck · 2019-02-12T17:37:51Z

plugin/storage/grpc/proto/storage.proto

+    rpc GetTrace(GetTraceRequest) returns (GetTraceResponse);
+    rpc GetServices(GetServicesRequest) returns (GetServicesResponse);
+    rpc GetOperations(GetOperationsRequest) returns (GetOperationsResponse);
+    rpc FindTraces(FindTracesRequest) returns (stream Trace);


Returning a stream here solves the issue surfaced in the other PR where grpc message limits were being hit.

hm, not sure it completely solves those issues unless we allow multiple Trace entries in the batch for the same trace. A single trace can still easily be larger than the msg limit.

Since this is a storage API, I would rather go with returning stream FindTracesResponseChunk where each chunk contains []Span

Out of curiosity what sort of size (and number of spans) would you expect to see the largest of traces reach? As traces contain more than just spans I think I'll have to give this a bit more thought.

we have seen traces in production with 100,000 spans. At average span size of around 500 bytes, that's 50M. Even 10k spans would add up to 5Mb.

I think eventually we'd want to make the chunk size configurable, but I would start with 1000 or even 100 spans.

yurishkuro · 2019-02-12T20:51:15Z

plugin/storage/grpc/proto/storage.proto

@@ -0,0 +1,199 @@
+syntax = "proto3";
+
+package jaeger.api_v2;


I think jaeger.storage.v1

yurishkuro · 2019-02-12T20:54:25Z

plugin/storage/grpc/proto/storage.proto

+
+package jaeger.api_v2;
+
+option go_package = "proto";


perhaps proto -> api_v1, so that Go package is api_v1, similar to model/proto/api_v2.proto

yurishkuro · 2019-02-12T20:56:03Z

plugin/storage/grpc/proto/storage.proto

+    google.protobuf.Timestamp end_timestamp = 1 [
+                                                (gogoproto.stdtime) = true,
+                                                (gogoproto.nullable) = false
+                                                ];


did you use some auto-formatter that creates these indentations? in the model.proto we have it more readable

message Log { google.protobuf.Timestamp timestamp = 1 [ (gogoproto.stdtime) = true, (gogoproto.nullable) = false ]; repeated KeyValue fields = 2 [ (gogoproto.nullable) = false ]; }

yurishkuro · 2019-02-12T21:02:36Z

plugin/storage/grpc/proto/storage.proto

+    rpc GetTrace(GetTraceRequest) returns (GetTraceResponse);
+    rpc GetServices(GetServicesRequest) returns (GetServicesResponse);
+    rpc GetOperations(GetOperationsRequest) returns (GetOperationsResponse);
+    rpc FindTraces(FindTracesRequest) returns (stream Trace);


hm, not sure it completely solves those issues unless we allow multiple Trace entries in the batch for the same trace. A single trace can still easily be larger than the msg limit.

Since this is a storage API, I would rather go with returning stream FindTracesResponseChunk where each chunk contains []Span

yurishkuro · 2019-02-12T21:42:47Z

plugin/storage/grpc/proto/storage.proto

+message FindTraceIDsResponse {
+    oneof response {
+        FindTraceIDsSuccess success = 1;
+        GrpcStoragePluginError error = 2;


couldn't we instead use grpc Status? It already includes an error message.

Something to note here is that the gogo plugin of Status is go 1.11+ (https://github.com/gogo/status)

yurishkuro · 2019-02-12T21:44:35Z

plugin/storage/grpc/proto/storage.proto

+//    rpc InsertProbabilitiesAndQPS(InsertProbabilitiesAndQPSRequest) returns (InsertProbabilitiesAndQPSResponse);
+//    rpc GetThroughput(GetThroughputRequest) returns (GetThroughputResponse);
+//    rpc GetProbabilitiesAndQPS(GetProbabilitiesAndQPSRequest) returns (GetProbabilitiesAndQPSResponse);
+//    rpc GetLatestProbabilities(GetLatestProbabilitiesRequest) returns (GetLatestProbabilitiesResponse);


let's remove this

yurishkuro · 2019-02-12T21:45:13Z

plugin/storage/grpc/proto/storage.proto

+    rpc WriteSpan(WriteSpanRequest) returns (WriteSpanResponse);
+}
+
+service GrpcSpanReaderStoragePlugin {


I am not sure we need Grpc and StoragePlugin prefix/suffix, they will be obvious from the package name.

yurishkuro · 2019-02-12T21:47:18Z

plugin/storage/grpc/proto/storage.proto

+    rpc FindTraceIDs(FindTraceIDsRequest) returns (FindTraceIDsResponse);
+}
+
+service GrpcDependenciesStoragePlugin {


any reason we don't want this split into reader and writer, similar to span storage?

Happy to do that. Do we consider dependency store writer as out of scope for this changeset?

we can skip it for now.

yurishkuro · 2019-02-12T23:04:54Z

to resolve Gopkg.lock conflict you may want to merge master, git checkout --theirs Gopkg.lock, then rerun dep ensure --update

chvck · 2019-02-14T08:13:52Z

plugin/storage/grpc/proto/storage.proto

+import "google/protobuf/timestamp.proto";
+import "google/protobuf/duration.proto";
+import "google/protobuf/empty.proto";
+import "google/rpc/status.proto";


This is a go 1.11+ requirement (https://github.com/gogo/status)

yurishkuro · 2019-02-14T17:05:20Z

plugin/storage/grpc/proto/storage.proto

+message WriteSpanResponse {
+    oneof response {
+        google.protobuf.Empty success = 1;
+        google.rpc.Status error = 2;


Sorry, this is not what I meant. This Status object is transported by gRPC natively, we don't need to use it in our response payloads.

Are you saying to basically remove the *Response messages and simply return *Success from the rpc functions? I've just had a play doing this and it makes the jaeger plugin code that talks to the generated proto code much cleaner.

yurishkuro · 2019-02-18T16:37:18Z

plugin/storage/grpc/proto/storage.proto

+    ];
+}
+
+message GetDependenciesSuccess {


GetDependenciesResponse ? Here and in all other responses.

yurishkuro · 2019-02-18T16:39:46Z

plugin/storage/grpc/proto/storage.proto

+message FindTracesResponseChunk {
+    repeated bytes spans = 1  [
+      (gogoproto.nullable) = false,
+      (gogoproto.customtype) = "github.com/jaegertracing/jaeger/model.Span"


I don't think this is the right dependency. The type should be Span from model.proto, the fact that it is backed by some specific Go type is irrelevant. Otherwise how could one implement a plugin in Java?

yurishkuro · 2019-02-18T16:41:23Z

plugin/storage/grpc/proto/storage.proto

+option (gogoproto.goproto_registration) = true;
+
+message GetDependenciesRequest {
+    google.protobuf.Timestamp end_timestamp = 1 [


minor, but I think we should clean up the API and use start_time and end_time (don't need to call them timestamp, it's just the type of the value, not the meaning of it).

yurishkuro · 2019-02-18T16:43:41Z

plugin/storage/grpc/proto/storage.proto

+
+service SpanWriterPlugin {
+    // spanstore/Writer
+    rpc WriteSpan(WriteSpanRequest) returns (google.protobuf.Empty);


let's define an empty WriteSpanResponse instead of google.protobuf.Empty, it is better for extensibility.

yurishkuro · 2019-02-18T16:44:32Z

looks good overall

yurishkuro · 2019-02-18T17:42:31Z

please resolve conflicts, the DependencyLink type has been extended in 1.10

yurishkuro · 2019-02-18T17:45:41Z

plugin/storage/grpc/proto/storage.proto

@@ -22,7 +22,7 @@ option (gogoproto.sizer_all) = true;
 option (gogoproto.goproto_registration) = true;

 message GetDependenciesRequest {
-    google.protobuf.Timestamp end_timestamp = 1 [
+    google.protobuf.Timestamp end_time = 1 [


I meant replace lookback with start_time (and swap the order). The Query Service API in the future will be converted to similar gRPC API, but it will need to work over JSON. It's less cognitive load to have two timestamp-formatted fields in JSON than to have a lookback in nanoseconds.

plugin/storage/grpc/proto/storage.proto

yurishkuro · 2019-02-18T17:47:09Z

plugin/storage/grpc/proto/storage.proto

@@ -32,7 +32,7 @@ message GetDependenciesRequest {
    ];
 }

-message GetDependenciesSuccess {
+message GetDependenciesResponse {
    repeated bytes dependencies = 1 [


DependencyLink?

yurishkuro · 2019-02-18T17:48:21Z

plugin/storage/grpc/proto/storage.proto

-    rpc GetTrace(GetTraceRequest) returns (GetTraceSuccess);
-    rpc GetServices(GetServicesRequest) returns (GetServicesSuccess);
-    rpc GetOperations(GetOperationsRequest) returns (GetOperationsSuccess);
+    rpc GetTrace(GetTraceRequest) returns (GetTraceResponse);


I suggest implementing this similar to FindTraces, as a stream of chunks, because even a single trace could be larger than the default message limit.

Something that I'm unsure about and may need consideration is that having a Trace as a stream of Span means that the ProcessMap and Warnings fields are lost on the Trace.

That's true, but these two concepts only apply to the current JSON API used by the UI, they don't apply to the storage API (even though the storage API returns model.Trace, it never actually populates ProcessMap or warnings, that's done post-retrieval).

yurishkuro · 2019-02-18T17:49:40Z

plugin/storage/grpc/proto/storage.proto

+
+package jaeger.storage.v1;
+
+option go_package = "api_v1";


maybe storage_v1 would be better?

yurishkuro · 2019-02-18T17:51:12Z

Makefile

+	protoc \
+		$(PROTO_INCLUDES) \
+		-I plugin/storage/grpc/proto \
+				--gogo_out=plugins=grpc,$(PROTO_GOGO_MAPPINGS):$(PWD)/plugin/storage/grpc/proto \


I would prefer folder name to match the package, e.g. plugin/storage/grpc/proto/storage_v1

model/proto/model.proto

yurishkuro · 2019-02-18T17:54:30Z

plugin/storage/grpc/proto/storage.proto

+}
+
+message FindTraceIDsResponse {
+    repeated bytes trace_id = 1 [


chvck · 2019-02-20T20:45:26Z

model/proto/model.proto

+  string child = 2;
+  uint64 call_count = 3;
+  string source = 4 [
+    (gogoproto.customtype) = "DependencyLinkSource",


I thought that this would be the best of both worlds re: the conversation above.

well, if we do this, I would recommend adding a simple marshaling/unmarshaling test, similar to model/ids_test.go

Actually, not just for this, but also for importing model.Span. I found it not particularly useful just adding gogoproto annotations without testing that serialization works as expected

I'm not following what you mean by "importing model.Span". In the plugin proto file? I don't believe that we actually use gogoproto annotations for importing Span there?

I mean this:

message GetTraceRequest { bytes trace_id = 1 [ (gogoproto.nullable) = false, (gogoproto.customtype) = "github.com/jaegertracing/jaeger/model.TraceID", (gogoproto.customname) = "TraceID" ]; }

From my previous experience with gogoproto, even though protoc may compile it, you won't know if it will serialize correctly until you try it in unit tests.

and similar for spans

I've been looking at this every which way, and it just bothers me that we're introducing this complication of custom type without any benefits whatsoever. The field should be just a string, so that an external tool (e.g. a service mesh) could submit graph links with another value of Source field. Having strong type in the model doesn't do anything useful. The only thing we need is the constant JaegerDependencyLinkSource = "jaeger" to use as default.

Signed-off-by: Charles Dixon <[email protected]>

yurishkuro · 2019-03-22T16:48:40Z

You probably don't need to rebase, just squash & merge master. One other option could be to squash only commits starting from the one where the sign-off was missed, e.g. git reset --soft C where C is the commit before the one where you missed the sign-off. Then you can commit all subsequent commits as one.

chvck · 2019-03-26T08:58:11Z

I'm unsure why make proto is changing files in proto-gen, it doesn't seem to do that locally on my machine.

yurishkuro · 2019-03-26T11:56:02Z

Are you using the same protocol? Did you run "make proto-install" locally before generating?

chvck · 2019-03-26T12:38:45Z

Are you using the same protocol? Did you run "make proto-install" locally before generating?

Thanks, that was the step I was missing. The plugin proto generated files are generating without the license header until make fmt is run which is why the build is flagging those as they've been included in the codebase with the license header. As these are generated files is it safe to leave the header off them?

yurishkuro · 2019-03-26T13:08:21Z

We avoid changing generated files, even with tools (eg https://github.com/jaegertracing/jaeger/blob/master/thrift-gen/jaeger/constants.go). We need to add them to exclusion lists in the Makefile so that go fmt and lint don't run on them.

Signed-off-by: Charles Dixon <[email protected]>

chvck · 2019-04-02T12:41:44Z

Hi @yurishkuro before I fix this up by merging master in, are there any changes here that you're waiting on me for?

yurishkuro · 2019-04-02T17:46:59Z

Makefile

+	$(PROTOC) \
+		$(PROTO_INCLUDES) \
+		-I plugin/storage/grpc/proto \
+				--gogo_out=plugins=grpc,$(PROTO_GOGO_MAPPINGS):$(PWD)/plugin/storage/grpc/proto/storage_v1 \


in order to avoid code coverage drop, we should place the generated files under proto-gen/storage_v1/

yurishkuro · 2019-04-02T17:47:18Z

Makefile

+		$(PROTO_INCLUDES) \
+		-I plugin/storage/grpc/proto \
+				--gogo_out=plugins=grpc,$(PROTO_GOGO_MAPPINGS):$(PWD)/plugin/storage/grpc/proto/storage_v1 \
+				plugin/storage/grpc/proto/storage.proto


please fix indentation in these two lines to align with L395

Signed-off-by: Charles Dixon <[email protected]>

yurishkuro

🥇 🎉

chvck requested review from black-adder, jpkrohling, objectiser, pavolloffay, tiffon, vprithvi and yurishkuro as code owners February 8, 2019 16:57

chvck commented Feb 11, 2019

View reviewed changes

chvck commented Feb 12, 2019

View reviewed changes

chvck force-pushed the proto branch from 30a0308 to 0cbf08a Compare February 12, 2019 17:46

yurishkuro reviewed Feb 12, 2019

View reviewed changes

chvck commented Feb 14, 2019

View reviewed changes

yurishkuro reviewed Feb 14, 2019

View reviewed changes

chvck force-pushed the proto branch from df0039d to 1f1a236 Compare February 15, 2019 07:50

yurishkuro reviewed Feb 18, 2019

View reviewed changes

plugin/storage/grpc/proto/storage.proto Show resolved Hide resolved

yurishkuro reviewed Feb 18, 2019

View reviewed changes

model/proto/model.proto Show resolved Hide resolved

yurishkuro reviewed Feb 18, 2019

View reviewed changes

plugin/storage/grpc/proto/storage.proto Outdated

}

message FindTraceIDsResponse {

repeated bytes trace_id = 1 [

Copy link

Member

yurishkuro Feb 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

trace_ids

chvck commented Feb 20, 2019

View reviewed changes

yurishkuro mentioned this pull request Feb 22, 2019

Support gRPC for query service #1307

Merged

ghost assigned yurishkuro Mar 14, 2019

ghost added the review label Mar 14, 2019

chvck added 10 commits March 22, 2019 16:08

Add generated storage plugin file to lint ignores

75eee97

Signed-off-by: Charles Dixon <[email protected]>

Lint ignore grpc plugin generated code by name not path

5fb2135

Signed-off-by: Charles Dixon <[email protected]>

Rename FindTracesResponseChunk to SpansResponseChunk

b503969

Signed-off-by: Charles Dixon <[email protected]>

Add marshal/unmarshal tests for DependencyLink

db7790a

Signed-off-by: Charles Dixon <[email protected]>

Add tests for storage protos with custom types

d05d800

Signed-off-by: Charles Dixon <[email protected]>

Run fmt and ignore storage_test for linting

8a301d9

Signed-off-by: Charles Dixon <[email protected]>

Remove DependencyLinkSource and use string

54df218

Signed-off-by: Charles Dixon <[email protected]>

Update headers

d46c32d

Signed-off-by: Charles Dixon <[email protected]>

Add SpansChunkResponse test

7dbde43

Signed-off-by: Charles Dixon <[email protected]>

Update makefile protoc calls

f99d320

Signed-off-by: Charles Dixon <[email protected]>

chvck force-pushed the proto branch from 6cd5030 to f99d320 Compare March 22, 2019 17:07

Merge branch 'master' into proto-master

18e47f2

chvck added 4 commits March 26, 2019 13:13

Update proto generated files and update license script

3f8ea75

Signed-off-by: Charles Dixon <[email protected]>

Merge branch 'master' into proto-master

273eaee

Update generated storage file to new proto layout

07a8b07

Signed-off-by: Charles Dixon <[email protected]>

Add storage generated files to import order cleanup ignores

94cfbae

Signed-off-by: Charles Dixon <[email protected]>

yurishkuro reviewed Apr 2, 2019

View reviewed changes

chvck added 4 commits April 3, 2019 09:45

Move storage generated file to proto-gen dir

b23cde6

Signed-off-by: Charles Dixon <[email protected]>

Remove generated plugin storage file from script ignores

7aafb1b

Signed-off-by: Charles Dixon <[email protected]>

Fix copyright headers

700e4e4

Signed-off-by: Charles Dixon <[email protected]>

Update storage_test generated file

398c5d2

Signed-off-by: Charles Dixon <[email protected]>

yurishkuro approved these changes Apr 3, 2019

View reviewed changes

yurishkuro merged commit c770640 into jaegertracing:master Apr 3, 2019

Add gRPC plugin proto changes #1323

Add gRPC plugin proto changes #1323

Conversation

chvck commented Feb 8, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chvck Feb 13, 2019 • 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

yurishkuro commented Feb 12, 2019

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 commented Feb 18, 2019

yurishkuro commented Feb 18, 2019

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 commented Mar 22, 2019

chvck commented Mar 26, 2019

yurishkuro commented Mar 26, 2019

chvck commented Mar 26, 2019

yurishkuro commented Mar 26, 2019

chvck commented Apr 2, 2019

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

chvck Feb 13, 2019 •

edited

Loading