implement stream decoding #227
Conversation
galdor
force-pushed
the
nicolas/stream-decoding
branch
from
84ecb4f to
f86e6ec
Compare
|
Small update to add more tests for the |
| } | ||
| default: | ||
| HTTPEndpointNotSupported([]string{tagServiceHandler, fmt.Sprintf("v:%s", v)}, w) | ||
| contentType := req.Header.Get("Content-Type") |
There was a problem hiding this comment.
I'm mixed here. One one hand, doing it this way makes the code more compact, and slightly more permissive, but this is fine. OTOH, doing this, we loose the information that "v01 and v02 are JSON only while v03 is JSON & MsgPack and has to decide depending on Content-Type". As I understand it, this code will accept any protocol on any enpoint. Maybe just state with a comment that MsgPack should only be for v03? Because for now, we know it, but we can still "forget it" when we migrate.
There was a problem hiding this comment.
We filter msgpack payloads for v0.1 and v0.2 in httpHandleWithVersion. Since decoding is handled in decodeReceiverPayload, handlers don't have to think about the format of the payload. The only exception is the manual JSON decoding for v0.1, but we're going to deprecate v0.1 anyway.
There was a problem hiding this comment.
OK, my bad, I missed the fact httpHandleWithVersion was doing the filtering, sorry for the noise.
| // Some libraries (e.g. msgp) will ignore read data if err is not nil. | ||
| // We reset err if something was read, and the next read will return | ||
| // io.EOF with no data. | ||
| if err == io.EOF && n > 0 { |
bytes.Buffer already implements io.Reader.
It's perfectly normal for a reader to return n>0 and EOF:
Callers should always process the n > 0 bytes returned before
considering the error err. Doing so correctly handles I/O errors
that happen after reading some bytes and also both of the allowed
EOF behaviors.
-- https://golang.org/pkg/io/#Reader
Infortunately, github.com/tinylib/msgp does not handle this case, and
return immediately if err != nil, without checking whether data were
read.
This workaround set the error to nil if there are data available. The
next read will return n=0 and EOF.
galdor
force-pushed
the
nicolas/stream-decoding
branch
from
f86e6ec to
af6f9d5
Compare
| default: | ||
| HTTPEndpointNotSupported([]string{tagTraceHandler, fmt.Sprintf("v:%s", v)}, w) | ||
| return | ||
| } | ||
|
|
||
| HTTPOK(w) | ||
|
|
||
| if contentLength > 0 { | ||
| atomic.AddInt64(&r.stats.TracesBytes, int64(contentLength)) | ||
| bytesRead := req.Body.(*model.LimitedReader).Count |
|
|
||
| r.decoderPool.Release(dec) | ||
| case v02: | ||
| fallthrough |
There was a problem hiding this comment.
nitpick: i don't like fallthrough because it sort of hinders local reasoning (oh ... this is handled later, keep reading).
why not just case v01, v02
| case "application/msgpack": | ||
| return msgp.Decode(r, dest) | ||
|
|
||
| case "application/json": |
| @@ -46,10 +49,9 @@ const ( | |||
| // HTTPReceiver is a collector that uses HTTP protocol and just holds | |||
| // a chan where the spans received are sent one by one | |||
| type HTTPReceiver struct { | |||
| traces chan model.Trace | |||
| services chan model.ServicesMetadata | |||
| decoderPool *model.DecoderPool | |||
There was a problem hiding this comment.
this is the second time i've made this comment over the last year, but the decoder pool was added to a fix a performance bottleneck (i think it's so we could re-use the decoding buffers, but it's been a while).
can you verify this has no impact?
See commit messages for more information.