{"action":"created","issue":{"url":"https://api.github.com/repos/jaegertracing/jaeger/issues/773","repository_url":"https://api.github.com/repos/jaegertracing/jaeger","labels_url":"https://api.github.com/repos/jaegertracing/jaeger/issues/773/labels{/name}","comments_url":"https://api.github.com/repos/jaegertracing/jaeger/issues/773/comments","events_url":"https://api.github.com/repos/jaegertracing/jaeger/issues/773/events","html_url":"https://github.com/jaegertracing/jaeger/issues/773","id":314431660,"number":773,"title":"Use Protobuf for internal communications","user":{"login":"yurishkuro","id":3523016,"avatar_url":"https://avatars2.githubusercontent.com/u/3523016?v=4","gravatar_id":"","url":"https://api.github.com/users/yurishkuro","html_url":"https://github.com/yurishkuro","followers_url":"https://api.github.com/users/yurishkuro/followers","following_url":"https://api.github.com/users/yurishkuro/following{/other_user}","gists_url":"https://api.github.com/users/yurishkuro/gists{/gist_id}","starred_url":"https://api.github.com/users/yurishkuro/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/yurishkuro/subscriptions","organizations_url":"https://api.github.com/users/yurishkuro/orgs","repos_url":"https://api.github.com/users/yurishkuro/repos","events_url":"https://api.github.com/users/yurishkuro/events{/privacy}","received_events_url":"https://api.github.com/users/yurishkuro/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":14,"created_at":"2018-04-15T16:49:03Z","updated_at":"2018-04-19T00:13:34Z","closed_at":null,"author_association":"OWNER","body":"We've talked about this a lot, but there are many moving pieces that must come together, so this is the meta issue to list them all out, in random order.\r\n\r\n* We want to have a single protobuf data model on the server\r\n* We want that data model to be representable in JSON so that query service API can be expressed with it instead of the current JSON model (issue #706)\r\n* We probably want to use gogoprotobuf instead of the default protobuf because gogo allows generation of the data model without too many pointers, e.g. `[]model.KeyValue` instead of `[]*model.KeyValue` ([gist][gist])\r\n* We need to decide if we want to use `oneof` for KeyValue or keep sending the Type field explicitly as we do today\r\n* We need to decide if we want to keep the existing UI model's optimization where the Process object in the Span is deduped and replaced with a [reference][1] to an entry in the map of Process objects attached to the Trace\r\n  * The current UI model supports both embedded Process and ProcessID reference, so we could keep this dual representation\r\n  * The deduping of the Process objects is more important when reporting spans from clients to the backend (which uses the [Batch type][2]) than it is for the query service API. The UI does not really rely on the deduping, it's primarily done for the purpose of reducing the JSON message size when returning the whole trace.\r\n* My proposal is to keep both Batch and Trace types. Batch doesn't need any changes.\r\n* Need to investigate how to expose JSON API for the UI as a facade on top of a gRPC API implemented by the query service\r\n  * https://github.com/grpc-ecosystem/grpc-gateway seems to work, apparently can even generate swagger\r\n  * https://github.com/yarpc/yarpc-go could be another possibility, as it also provides client side load balancing and integration with discovery. We have that in the current tchannel-based implementation, but gRPC doesn't provide it out of the box (or does it?)\r\n* The data model generated with gogoprotobuf is actually very close to the existing domain model `model.Span`. We need to decide if we want to keep them separate and do transformation or merge them into one by generating the core skeletons and then adding the additional business methods via companion hand-written source files. I am in favor of merging them to minimize data copying. The risk here is if future changes in proto/gogo would result in differently generated model, but that seems unlikely.\r\n* Current comms between agent and collectors are done with tchannel that supports client side load balancing and integration with discovery. We'd need an equivalent of that with gRPC, e.g. by using yarpc.\r\n* There are a few differences in the JSON generated from IDL in the [gist][gist] (some could be addressed via custom renderers):\r\n  * dates are rendered in UTC: \"2018-04-02T02:17:45.171667100Z\"\r\n  * durations are rendered as string: \"0.013s\"\r\n  * trace ID encoded as two integer fields instead of one string (some int64s cannot be parsed by JS)\r\n  * span ID encoded as integer instead of one string (some int64s cannot be parsed by JS)\r\n  * `KeyValue.VType=STRING` and `SpanRef.Type=CHILD_OF` are omitted by default (as zero values)\r\n\r\nPending resolution of these questions, the tentative list of work items would be:\r\n* [ ] define official proto\r\n* [ ] merge domain model with proto-generated model\r\n  * fix tests to use proto-generated JSON instead of the current `encoding/json`\r\n* [ ] redefine query service API via proto IDL and implement it with gRPC\r\n* [ ] implement a REST/JSON facade on top of gRPC\r\n* [ ] upgrade UI to the new JSON data model (if it's even different)\r\n* [ ] implement collector APIs with gRPC\r\n  * span submission\r\n  * sampling, throttling\r\n* [ ] implement agent outbounds using gRPC (and yarpc?)\r\n* [ ] implement agent APIs with gRPC\r\n* [ ] implement gRPC transport in the clients\r\n\r\n[1]: https://github.com/jaegertracing/jaeger/blob/master/model/json/model.go#L79\r\n[2]: https://github.com/jaegertracing/jaeger/blob/master/thrift-gen/jaeger/ttypes.go#L1582\r\n[gist]: https://gist.github.com/yurishkuro/d2220a386a82c91f07c13bcb664f4165"},"comment":{"url":"https://api.github.com/repos/jaegertracing/jaeger/issues/comments/382568411","html_url":"https://github.com/jaegertracing/jaeger/issues/773#issuecomment-382568411","issue_url":"https://api.github.com/repos/jaegertracing/jaeger/issues/773","id":382568411,"user":{"login":"yurishkuro","id":3523016,"avatar_url":"https://avatars2.githubusercontent.com/u/3523016?v=4","gravatar_id":"","url":"https://api.github.com/users/yurishkuro","html_url":"https://github.com/yurishkuro","followers_url":"https://api.github.com/users/yurishkuro/followers","following_url":"https://api.github.com/users/yurishkuro/following{/other_user}","gists_url":"https://api.github.com/users/yurishkuro/gists{/gist_id}","starred_url":"https://api.github.com/users/yurishkuro/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/yurishkuro/subscriptions","organizations_url":"https://api.github.com/users/yurishkuro/orgs","repos_url":"https://api.github.com/users/yurishkuro/repos","events_url":"https://api.github.com/users/yurishkuro/events{/privacy}","received_events_url":"https://api.github.com/users/yurishkuro/received_events","type":"User","site_admin":false},"created_at":"2018-04-19T00:13:34Z","updated_at":"2018-04-19T00:13:34Z","author_association":"OWNER","body":"I suppose another option would be to render int64 as a string in JSON, to avoid precision loss in Javascript."}}
{"actor":{"display_login":"yurishkuro"}}