uber-go / tally

A Go metrics interface with fast buffered metrics and third party reporters
MIT License
850 stars 116 forks source link

m3/thriftudp: bump SO_RCVBUF on the UDP server socket #164

Closed masiulaniec closed 3 years ago

masiulaniec commented 3 years ago

Metric submissions that microburst may momentarily overflow the in-kernel receive buffer. Reduce data loss risk by expanding the receive buffer to the maximum allowed size.

Ref M3-1034

CLAassistant commented 3 years ago

CLA assistant check
All committers have signed the CLA.

masiulaniec commented 3 years ago

136 also works except it omits the auto tuning idea, which of course may be implemented at a higher layer.

I disagree this approach does not work. An experimental test program reports an 82x improvement. Calling SetBufferSize enables 7585 UDP writes to be observable by a later read, compared to only 92 without SetBufferSize.

It also works in theory:

http://man.he.net/man7/tcp mentions the SO_RCVBUF-before-listen restriction. http://man.he.net/man7/udp does not.

It is understandable that window scaling in TCP might benefit from SO_RCVBUF-before-listen. But UDP has no flow control so the restriction does not apply.

If UDPConn.SetReadBuffer really could never be used correctly due to inherent ordering problem between listen(2) and setsockopt(2), then its godoc should contain a warning. However, the conventional "Deprecated: (guidance)" comment is absent.

vdarulis commented 3 years ago

136 exposes far more flexibility to server, and doesn't expose our internal requirements.

Also, with full control over connection, we'd also have the ability to spawn multiple listeners (with SO_REUSEPORT) and having kernel load-balance writes, which would be helpful in cases where the max receive buffer size is not tuned from Linux defaults.