Replace domain model with Protobuf/gogo-generated model #856

yurishkuro · 2018-06-01T04:03:14Z

There are a lot of changes to the JSON files that contain the representation of previous domain model. Because gogo/protpbuf/jsonpb renders the model slightly differently, they had to be changed. However, I kept the model/json fixtures intact everywhere, so that this PR does not have any impact on the UI. In other words we can continue developing the gRPC & REST API using the new model, and once that's available the UI can be upgraded to the new model, then the model/json can be moved inside Elasticsearch storage plugin (I figured it's safer not to change that format and keep the converters, but scope them to ES only).

TODO:

the durations are rendered as strings, like "5000ns". We haven't reached the final consensus about what we want to do with those, working proposal - microseconds as float values (to allow for nanos).
because we need various extension methods on the domain types, the auto-generated jaeger.pb.go file is placed under model, but it has a lot less coverage. Makefile can grep its lines out of the coverage summary.
a bunch of code was commented out, need to clean it up

yurishkuro · 2018-06-01T04:19:20Z

model/keyvalue.go

+// 	Key   string    `json:"key"`
+// 	VType ValueType `json:"vType"`
+// 	VStr  string    `json:"vStr,omitempty"`
+// 	VNum  int64     `json:"vNum,omitempty"`


the KeyValue struct is now less memory efficient because vNum or split into vBool/vInt64/vFloat64

isaachier · 2018-06-01T18:15:48Z

Makefile

@@ -92,7 +92,8 @@ cover: nocover
 	@echo pre-compiling tests
 	@time go test -i $(ALL_PKGS)
 	@./scripts/cover.sh $(shell go list $(TOP_PKGS))
-	go tool cover -html=cover.out -o cover.html
+	egrep -v 'jaeger.pb.*.go' cover.out > cover-nogen.out


egrep is deprecated on many platforms. Use grep -E instead.

isaachier · 2018-06-01T18:16:57Z

examples/proto/main.go

+	s := grpc.NewServer(
+	// grpc.Creds(credentials.NewServerTLSFromCert(&insecure.Cert)),
+	// grpc.UnaryInterceptor(grpc_validator.UnaryServerInterceptor()),
+	// grpc.StreamInterceptor(grpc_validator.StreamServerInterceptor()),


What's up here?

example is WIP, I may remove it from the final PR, just experimenting with service JSON from grpc endpoint.

isaachier · 2018-06-01T18:17:33Z

examples/proto/main.go

+		context.Background(),
+		dialAddr,
+		grpc.WithInsecure(),
+		// grpc.WithTransportCredentials(credentials.NewClientTLSFromCert(insecure.CertPool, "")),


Same question.

isaachier · 2018-06-01T18:17:57Z

examples/proto/main.go

+	// err = serveOpenAPI(mux)
+	// if err != nil {
+	// 	log.Fatalln("Failed to serve OpenAPI UI")
+	// }


Same question.

isaachier · 2018-06-01T18:18:37Z

examples/proto/server/server.go

+			pbJaeger.Span{
+				TraceID: pbJaeger.TraceID{Low: 123},
+				// SpanID:  []byte{1, 2, 3, 4, 5, 6, 7, 8},
+				// SpanID:        456,


Why all of this repetition of SpanID?

isaachier · 2018-06-01T18:19:42Z

model/converter/json/fixtures/domain_01.json

    {
      "traceID": "1",
      "spanID": "2",
      "operationName": "test-general-conversion",
      "startTime": "2017-01-26T16:46:31.639875139-05:00",
-      "duration": 5000,
+      "duration": "5000ns",


Not a huge fan of strings for duration. double/float64 is good.

isaachier · 2018-06-01T18:21:38Z

model/converter/thrift/jaeger/fixtures/model_02.json

+        {
+          "key": "peer.service",
+          "vType": "BINARY",
+          "vBinary": "AAAAAAAAMDk="


Sort of random question: Is all binary base64 encoded or can it be complete junk?

It's always a base64 string according to proto3-lang. I still prefer usage of oneof which should remove the vType field from JSON since the (de-)serializer knows that there can only be one field active.

isaachier · 2018-06-01T18:24:11Z

model/keyvalue_test.go

-	err = json.Unmarshal([]byte(`{"key":"x","vType":"BAD","vNum":123}`), &kv3)
-	assert.EqualError(t, err, "not a valid ValueType string BAD")
-}
+// func TestValueTypeToFromJSON(t *testing.T) {


Is this going to be uncommented?

Falco20019

I would love to see this replace thrift in the long run :) Having to vendor thrift is pretty ugly right now since there is no Apache-managed release for C#, only for 0.9.3 which does not support .NET Core.

Falco20019 · 2018-06-06T07:43:46Z

model/proto/jaeger.proto

+  BINARY  = 4;
+};
+
+message KeyValue {


This reminds me somewhat of Value from struct.proto. Please use the oneof variant. It's easier to maintain and a lot better to work with in most languages.

Oneof is terrible in Go in both usability and performance. Why do you find them easier to use? You need a switch statement either way, and in most cases you need an additional cast.

In C# you have the switch in both cases, either doing your own enum or using the oneof, and you get the enum generated and maintained by the code generator. Performance-wise it's the same as every other non-oneof field (also on the wire), just that the code sets you a second field which enum was selected, which you would have set yourself either way.

What casts do you need in Go? This is how it looks in C#: https://developers.google.com/protocol-buffers/docs/reference/csharp-generated#oneof

You get an additional enum AvatarOneofCase, you get the property AvatarCase which is baiscally your vType, you get ImageUrl and ImageData as if they would have been regular fields (no difference at all).

In Go the oneof is modeled as an interface field (example: gogo/protobuf#168), which means it requires memory allocation even for primitive values, with a fairly ugly syntax like

kv := KeyValue{ Key: "key", Value: Value_Int{Int: 123}, }

and on reading you need to switch on the type (I was wrong, it does not require additional cast).

The memory allocations are the main turn-off of oneof for me, especially for such frequently used thing as KeyValue type. For ProcessID/ProcessMap I could see us using a oneof, but again the syntax is quite ugly.

I agree, in C# it looks like this:

var kv = new KeyValue{ Key = "key", Int = 123, }

For reading:

switch(kv.ValueCase) { case ValueOneofCase.Int: var int = kv.Int; ... }

No extra types or anything :) Would be interesting how other languages handle this.

Falco20019 · 2018-06-06T07:54:56Z

model/proto/jaeger.proto

+// message KeyValue2 {
+//   string key = 1;
+//   oneof value {
+//     string vStr = 2;


Not compliant to naming scheme. proto uses snake_case see documentation. I would also drop the v and just use speaking names. The json-gateway should make them camelCase automatically I assume. Also all proto-implementations to naming automatically. But it's best to stay with the default for proto.

Where is the naming scheme defined? I only see examples in snake case.

See the link in the documentation. Everything from Google uses snake_case (with underscores). vStr is camelCase.

Falco20019 · 2018-06-06T07:57:30Z

model/proto/jaeger.proto

+};
+
+message SpanRef {
+  TraceID traceID = 1 [


Not compliant to naming scheme: trace_id

Falco20019 · 2018-06-06T07:57:41Z

model/proto/jaeger.proto

+  TraceID traceID = 1 [
+    (gogoproto.nullable) = false
+  ];
+  uint64 spanID = 2 [


Not compliant to naming scheme: span_id

Maybe to it as message? I don't know if other languages also have the customtype notation.

Falco20019 · 2018-06-06T08:11:41Z

model/proto/jaeger.proto

+    (gogoproto.nullable) = false,
+    (gogoproto.customtype) = "SpanID" // alias to uint64
+  ];
+  SpanRefType refType = 3;


Not compliant to naming scheme: ref_type

Falco20019 · 2018-06-06T08:12:09Z

model/proto/jaeger.proto

+  ];
+}
+
+message Span {


Here it's all mixy with the naming :) trace_id, span_id, start_time and process_id. operation_name is the only outlier here, but the correct one.

Falco20019 · 2018-06-06T08:13:10Z

model/proto/jaeger.proto

+
+message Trace {
+  message ProcessMapping {
+      string processID = 1;


Not compliant to naming scheme: process_id

Falco20019 · 2018-06-06T08:13:23Z

model/proto/jaeger.proto

+      ];
+  }
+  repeated Span spans = 1;
+  repeated ProcessMapping processMap = 2 [


Not compliant to naming scheme: process_map

Falco20019 · 2018-06-06T08:36:30Z

model/proto/jaeger.proto

+  repeated Log logs = 9 [
+    (gogoproto.nullable) = false
+  ];
+  Process process = 10;


process and processID are XOR, so it should be a oneof. Has no impact on JSON but makes using them easier.

isaachier · 2018-06-06T08:37:51Z

If we end up using this in the clients, we need a better way to bundle the new protobuf dependencies. The so-called "well-known types" are not included in the default protobuf package. Installing Go packages is never ideal for C# or any other language. We should provide them ourselves seeing as that is what all these Go packages are doing anyway.

Falco20019 · 2018-06-06T12:41:14Z

Is gogoproto a separate compiler that is compatbile to regular proto or is it a incompatible fork? In C# and Java, the well-knowns are included in the base package. The protos are only in the Google.Protobuf.Tools as part of the build chain. But Google.Protobuf, the main library has implementations and also the protoc-csharp-plugin does skip generation of code for them.

Very interesting that that's not the case for all languages. They sould be at least part of the build chain.

yurishkuro · 2018-06-06T14:15:32Z

@Falco20019 appreciate the review. Left a couple questions for you.

I don't mind using snake_case in protobuf there's an established convention. I just need to see how that would affect the json, I'd rather avoid major changes there if we have a chance of using it with the UI.

Falco20019 · 2018-06-06T14:48:34Z

I would assume that you can use options to tell the JSON name when using grpc-gateway. Haven't used it though, only the C# implementation of the JSON serializer...

Just had a look at the go oneof implementation. Looks like it generates a separate struct for each field, which is, I assume, what you meant with worse performance. From the usability and readability perspective, it seems not too much worse I think. You would have to do the switch either way (using oneof or an enum) and since it generates the getter functions, you can access it just like you would have before. Could it be, that gogoproto is just doing this differently?

vprithvi · 2018-06-06T15:23:35Z

model/proto/jaeger.proto

+  option (gogoproto.compare) = true;
+
+  string    key      = 1;
+  ValueType vType    = 2;


Do we need valueType? Could we simply check which field is set when deserializing?

It probably depends on the language. Have we considered using the Google well known protobuf type Any? I assume that is an optimized format.

Falco20019 · 2018-06-06T16:12:34Z

Any serializes as byte array with type string. So could be a lot bigger for small data like int

isaachier · 2018-06-06T16:23:22Z

I wonder if the builtin types would need a url. Maybe it is optimized.

isaachier · 2018-06-06T16:40:18Z

Sorry I guess I should have said Value. Our spans can just contain an instance of Struct for tags and that would work as well.

List of the well-known types: https://developers.google.com/protocol-buffers/docs/reference/google.protobuf.

Falco20019 · 2018-06-06T17:25:11Z

Yep, I mentioned in one review comment that it reminds me a lot of that proto. But that’s a oneof too, so same „problem“ with performance and usability in Go. Not sure how costly those structs are since Google uses Go internally. They build the language and the format, so I would guess it’s not that memory heavy. But I don’t know too much about Go to judge that

isaachier · 2018-06-06T17:38:48Z

@Falco20019 Go tries to avoid unsafe language constructs, so there are no unions. Hence the use of multiple struct fields. However, I surprised gogoprotobof didn't generate pointers to the field types. Maybe the premise is that allocation is more expensive than unused space? IDK.

isaachier · 2018-06-06T17:41:20Z

Also, Google's support for Go using Protobuf is questionable at best. The standard Protobuf package leaves a lot to be desired. That's the whole reason gogoprotobuf, a fork of the original Go Protobuf package, has become more popular than the original. If you look at the Protobuf code, it is clear that the real performance optimizations occur in C++ clients (best example is arena allocation).

isaachier · 2018-06-06T17:57:04Z

Never mind, I see the actual code is more complex to mimic a union. Sort of resembles the pointer solution.

yurishkuro · 2018-06-06T21:03:09Z

Too many comments about oneof, which doesn't concern me too much. Since it results in very inefficient Go types, I am not planning to use it.

I changed the naming to snake_case, one negative side effect is that traceID/spanID in JSON are now camelCase traceId/spanId (more UI impact). But at least it's canonical proto JSON format.

The bigger open question for me is using bytes for IDs, which will change the representation from our custom rendering as hex strings to base64 strings. But if we want to keep standard proto JSON format, I don't see what else we can do. We can add some syntactic sugar in the UI to accept both base64 and hex strings, but it's certainly confusing that on-the-wire representation in trace context is hex, but JSON is base64.

isaachier · 2018-06-06T22:07:58Z

IMHO it is worth reviewing the options available to generate efficient code for oneof seeing as it is axiomatic to Protobuf and is more efficient in pretty much every other language.

I don't think using bytes makes sense for trace ID because the type is fixed. Bytes makes sense for binary strings.

EDIT: Reread and now understand the issue with integers slightly better. Again worth looking into options.

yurishkuro · 2018-06-06T22:47:31Z

@isaachier not following you.

What do you propose about oneof?

What do you mean by "bytes ... are fixed"? I was going to change Span like this:

message Span {
  bytes trace_id = 1 [
    (gogoproto.nullable) = false,
    (gogoproto.customname) = "TraceID",
    (gogoproto.customtype) = "TraceID"
  ];
  bytes span_id = 2 [
    (gogoproto.nullable) = false,
    (gogoproto.customname) = "SpanID",
    (gogoproto.customtype) = "SpanID"
  ];

and

// TraceID is a random 128bit identifier for a trace
type TraceID [16]byte

// SpanID is a random 64bit identifier for a span.
type SpanID [8]byte

This avoids extra memory allocations. Unfortunately, it also requires me to re-implement marshaling methods that are otherwise available for bytes type - the PB methods are easy, just copying the bytes, the JSON methods would have to use base64.

codecov · 2018-06-10T16:08:30Z

Codecov Report

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

@@          Coverage Diff          @@
##           master   #856   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files         121    121           
  Lines        5997   5959   -38     
=====================================
- Hits         5997   5959   -38

Impacted Files	Coverage Δ
model/converter/thrift/zipkin/to_domain.go	`100% <ø> (ø)`	⬆️
...lugin/storage/cassandra/spanstore/dbmodel/model.go	`100% <ø> (ø)`	⬆️
model/trace.go	`100% <ø> (ø)`	⬆️
model/process.go	`100% <ø> (ø)`	⬆️
model/span.go	`100% <ø> (ø)`	⬆️
model/spanref.go	`100% <100%> (ø)`	⬆️
model/converter/json/from_domain.go	`100% <100%> (ø)`	⬆️
cmd/collector/app/sanitizer/utf8_sanitizer.go	`100% <100%> (ø)`	⬆️
model/keyvalue.go	`100% <100%> (ø)`	⬆️
model/ids.go	`100% <100%> (ø)`	⬆️

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 fdd8e12...29a1709. Read the comment docs.

yurishkuro · 2018-06-10T16:08:57Z

Ready for review

Falco20019 · 2018-06-11T09:26:01Z

Only checked the proto and looks ok for me.

isaachier · 2018-06-12T10:55:08Z

Somewhat separate issue, but if we use this format, would it be possible to have the backend accept both Protobuf and JSON? That way a new client can choose to use Protobuf as a dependency or just to use pure JSON.

isaachier · 2018-06-12T10:59:05Z

Also, is there any reason not to move jaeger.proto to the jaeger-idl repo?

isaachier · 2018-06-12T11:02:37Z

Makefile

+		--go_out=$$GOPATH/src/github.com/jaegertracing/jaeger/model/prototest/ \
+		model/proto/jaeger_test.proto
+
+proto-install:


isaachier · 2018-06-12T11:03:11Z

Makefile

+	go install \
+		./vendor/github.com/gogo/protobuf/protoc-gen-gogo \
+		./vendor/github.com/grpc-ecosystem/grpc-gateway/protoc-gen-grpc-gateway \
+		./vendor/github.com/grpc-ecosystem/grpc-gateway/protoc-gen-swagger


Get this on my Debian desktop when trying this command:

$ make proto-install go install \ ./vendor/github.com/gogo/protobuf/protoc-gen-gogo \ ./vendor/github.com/grpc-ecosystem/grpc-gateway/protoc-gen-grpc-gateway \ ./vendor/github.com/grpc-ecosystem/grpc-gateway/protoc-gen-swagger vendor/github.com/grpc-ecosystem/grpc-gateway/protoc-gen-grpc-gateway/main.go:18:2: cannot find package "github.com/golang/glog" in any of: /home/ihier/proj/go/src/github.com/jaegertracing/jaeger/vendor/github.com/golang/glog (vendor tree) /usr/local/go/src/github.com/golang/glog (from $GOROOT) /home/ihier/proj/go/src/github.com/golang/glog (from $GOPATH) make: *** [Makefile:352: proto-install] Error 1

After finally getting it to work and generating Python code, I get this clearly invalid import:

from github.com.gogo.protobuf.gogoproto import gogo_pb2 as github_dot_com_dot_gogo_dot_protobuf_dot_gogop roto_dot_gogo__pb2

Same with C++:

#include "github.com/grpc-ecosystem/grpc-gateway/protoc-gen-swagger/options/annotations.pb.h" #include "github.com/gogo/protobuf/gogoproto/gogo.pb.h"

we may have to strip them post-generation. E.g. in python these two extra imports are not actually used.

yurishkuro · 2018-06-12T18:14:19Z

@isaachier

Somewhat separate issue, but if we use this format, would it be possible to have the backend accept both Protobuf and JSON? That way a new client can choose to use Protobuf as a dependency or just to use pure JSON.

That is the intent.

Also, is there any reason not to move jaeger.proto to the jaeger-idl repo?

In the future, too early for it now, introduces unnecessary dev friction. I would like to get a gRPC channel between agent & collector working first before moving proto files to idl.

isaachier · 2018-06-13T16:47:49Z

As long as this works for Go, LGTM. Other languages can be solved later or use JSON.

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

black-adder

this PR was too small

yurishkuro requested review from black-adder, jpkrohling, pavolloffay and vprithvi as code owners June 1, 2018 04:03

ghost assigned yurishkuro Jun 1, 2018

ghost added the review label Jun 1, 2018

yurishkuro commented Jun 1, 2018

View reviewed changes

yurishkuro force-pushed the gogoproto-model branch from cc4e0e4 to bbb46db Compare June 1, 2018 17:14

isaachier reviewed Jun 1, 2018

View reviewed changes

jaegertracing deleted a comment from coveralls Jun 1, 2018

yurishkuro mentioned this pull request Jun 6, 2018

Use Protobuf and gRPC for internal communications #773

Open

24 tasks

Falco20019 reviewed Jun 6, 2018

View reviewed changes

vprithvi reviewed Jun 6, 2018

View reviewed changes

yurishkuro force-pushed the gogoproto-model branch 2 times, most recently from 82bb5f9 to 17f0397 Compare June 6, 2018 17:46

yurishkuro changed the title ~~WIP: Gogoproto model~~ Replace domain model with Protobuf/gogo-generated model Jun 10, 2018

isaachier mentioned this pull request Jun 11, 2018

Depends on protobuf-c 1.3 isaachier/jaeger-client-c#4

Open

isaachier reviewed Jun 12, 2018

View reviewed changes

yurishkuro mentioned this pull request Jun 13, 2018

MarshalJSONPB is not called for custom type if it's an alias to a primitive gogo/protobuf#411

Closed

isaachier approved these changes Jun 13, 2018

View reviewed changes

Yuri Shkuro added 12 commits June 13, 2018 13:09

Prototype protobuf / gRPC / REST

c33f06d

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

Use proto-generated types as domain model

2057431

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

Change trace/span ID in proto to bytes

1566e53

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

Fix model tests

50e6b0c

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

move test to ids_test.go

f13e5b6

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

Convert IDs to base64 in integration test fixtures

23d6d66

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

Fix linter

2e55f17

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

remove header

2d43545

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

exclude test proto from updateLicenses.sh

c32d17b

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

remove grpc example

3203ca3

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

Cleanup before merge

3e1bb00

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

Add phony label

29a1709

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

yurishkuro force-pushed the gogoproto-model branch from 4094b14 to 29a1709 Compare June 13, 2018 17:09

black-adder approved these changes Jun 13, 2018

View reviewed changes

yurishkuro merged commit e9eed5a into master Jun 14, 2018

ghost removed the review label Jun 14, 2018

yurishkuro mentioned this pull request Jun 22, 2018

Decouple DB and JSON conversions from protobuf strings #882

Merged

pavolloffay deleted the gogoproto-model branch November 5, 2018 12:51

Replace domain model with Protobuf/gogo-generated model #856

Replace domain model with Protobuf/gogo-generated model #856

Conversation

yurishkuro commented Jun 1, 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

isaachier Jun 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Falco20019 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

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

isaachier commented Jun 6, 2018

Falco20019 commented Jun 6, 2018

yurishkuro commented Jun 6, 2018

Falco20019 commented Jun 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Falco20019 commented Jun 6, 2018

isaachier commented Jun 6, 2018

isaachier commented Jun 6, 2018 • edited Loading

Falco20019 commented Jun 6, 2018

isaachier commented Jun 6, 2018

isaachier commented Jun 6, 2018

isaachier commented Jun 6, 2018

yurishkuro commented Jun 6, 2018

isaachier commented Jun 6, 2018 • edited Loading

yurishkuro commented Jun 6, 2018

codecov bot commented Jun 10, 2018 • edited Loading

Codecov Report

yurishkuro commented Jun 10, 2018

Falco20019 commented Jun 11, 2018

isaachier commented Jun 12, 2018

isaachier commented Jun 12, 2018

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 Jun 12, 2018

isaachier commented Jun 13, 2018

black-adder left a comment

Choose a reason for hiding this comment

yurishkuro commented Jun 1, 2018 •

edited

Loading

isaachier Jun 1, 2018 •

edited

Loading

isaachier commented Jun 6, 2018 •

edited

Loading

isaachier commented Jun 6, 2018 •

edited

Loading

codecov bot commented Jun 10, 2018 •

edited

Loading