Fix buffer size counting bug and add logging to UDP sender #151
Conversation
| // OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN | ||
| // THE SOFTWARE. | ||
|
|
||
| declare class dgram$Socket { |
There was a problem hiding this comment.
I've never seen this naming convention. Is this part of ES6's style?
| if (bufferResult.err) { | ||
| console.log('err', bufferResult.err); | ||
| if (writeResult.err) { | ||
| this._logger.error(`error writing Thrift object: ${writeResult.err}`); |
There was a problem hiding this comment.
Do we want to emit a metric here?
There was a problem hiding this comment.
the sender is not integrated with metrics currently. Also the two new errors are a sign of misconfiguration, not something that can keep happening repeatedly, thus I think the logs are a lot more useful.
| // that's why in the constructor we also add a general on('error') handler. | ||
| this._client.send(thriftBuffer, 0, thriftBuffer.length, this._port, this._host, (err, sent) => { | ||
| if (err) { | ||
| this._logger.error(`error sending spans over UDP: ${err}, packet size: ${writeResult.offset}, bytes sent: ${sent}`); |
src/_flow/socket.js
Outdated
| @@ -0,0 +1,33 @@ | |||
| // @flow | |||
| // Copyright (c) 2016 Uber Technologies, Inc. | |||
There was a problem hiding this comment.
we should update the license.sh to say 2017
| this._batch.spans.push(span); | ||
| if (this._byteBufferSize < this._maxSpanBytes) { | ||
| this._totalSpanBytes += spanSize; | ||
| if (this._totalSpanBytes < this._maxSpanBytes) { |
There was a problem hiding this comment.
why is this extra logic is needed? ie if totalspanbytes == maxspanbytes, the next time round it will be flushed on L118. Do we really need this extra check that has only a 1 byte tolerance?
There was a problem hiding this comment.
- if
total + spanSizeis less than max packet size, we don't flush (diff could be 1 byte or 50Kb, whichever) - if
total + spanSizeis exactly the max packet size, we can flush the buffer including the current span, rather than flushing a smaller buffer and keeping the current span for the next flush
minor optimization, but seems worth it given that it doesn't complicate the code that much.
|
For the most part looks good to me. The only real concern is whether or not we should emit a metric on error logs. |
Resolves #150