Conversation
| return &Collector{ | ||
| traceIDs: make(map[int64]int, capacity), | ||
| traces: make([]trace, capacity, capacity), | ||
| traceIDs: make(map[uint64]int, capacity), |
There was a problem hiding this comment.
fyi zipkin trace ids since a few months ago can be 128bit. in thrift format, the representation is high/low. in json it is simply a longer hex
| } | ||
| } | ||
|
|
||
| func TestCodec(t *testing.T) { |
There was a problem hiding this comment.
when replacing codecs (ex squashing out a 3rd party dep like you have). I typically keep a test dep to ensure it is interoperable, at least for a while. That way you can tell if your endpoint works with zipkin or not (ex it accepts json or thrift, and your local codec can test against something else)
| repeated Span spans = 2 [(gogoproto.nullable) = false]; | ||
| } | ||
|
|
||
| message Span { |
There was a problem hiding this comment.
interesting.. since the format is different it might be good to clarify your README statement
ex. "Internally Loki is really just an opinionated reimplementation of OpenZipkin."
Reimplementation typically uses the same model. Seems this is more like jaeger which has a different model, and supports interop to some degree. You might consider adding a section to the README explaining to users who find this tool what sort of interop is expected to work within zipkin. For example, it the api still compatible? if so which endpoints (is it just the collector or do the other endpoints still work)? If it is just collector, which of json or thrift work? If neither, then no existing zipkin instrumentation would work. On that note, since B3 isn't used, the tracer side of this isn't a reimplementation rather (again similar to jaeger) a different format and library.
Maybe least confusing is to stick with the first part of the README "inspired by" if interop isn't a goal.
There was a problem hiding this comment.
Thats a good point - with this change we're no longer zipkin compatible on the most part (the API will still be, as I don't want to implement my own UI)...
I've tried to make these protos more closely match the OpenTracing concepts & keywords. I'll include an update to the readme in this PR.
Thanks for the feedback Adrian!
|
@tomwilkie I am working in https://github.com/sipian/loki/tree/protos branch. [EDIT] |
Mainly due to feedback from the Prometheus team about too many dependencies.