Performance improvement - process udp packets in separate goroutine in order to minimize packet drops #511
Conversation
…ot dropped Signed-off-by: kullanici0606 <yakup.turgut@btk.gov.tr>
Signed-off-by: kullanici0606 <yakup.turgut@btk.gov.tr>
Signed-off-by: kullanici0606 <yakup.turgut@btk.gov.tr>
…t in packet read in order not to drop packets Signed-off-by: kullanici0606 <yakup.turgut@btk.gov.tr>
b301995 to
aaaf26c
Compare
There was a problem hiding this comment.
Thank you for this, and sorry for the delay, I was out for a few weeks and am still catching up.
Overall I really like the simplicity of this approach. I think it's ready to go with two minor edits. I am also wondering whether we should use this opportunity to handle the case where the queue is full explicitly, but I am happy to leave that to a future PR (or never, if it has undesirable side effects).
pkg/listener/listener.go
Outdated
| @@ -64,12 +66,26 @@ func (l *StatsDUDPListener) Listen() { | |||
| level.Error(l.Logger).Log("error", err) | |||
| return | |||
| } | |||
| l.HandlePacket(buf[0:n]) | |||
| // avoid making copies of slices since we need to minimize the time spent here in order not to drop packets | |||
There was a problem hiding this comment.
This comment is confusing to me – EnqueueUdpPacket immediately and synchronously goes on to make a copy of the packet?
There was a problem hiding this comment.
If we really wanted to avoid allocations in the hot path, I guess we would need a rotating pool of buffers, but I'm not sure the memory overhead is worth it, so I would not go there right now. I think the best way here is to delete the confusing comment and accept that we are making a copy.
There was a problem hiding this comment.
Thank you for taking your time and reviewing my PR. You are right, comment is confusing, I removed the misleading comment. I also tried pooling but I think it made thinks worse since it needs some kind of syncronization.
pkg/listener/listener.go
Outdated
| l.UDPPackets.Inc() | ||
| packetCopy := make([]byte, n) | ||
| copy(packetCopy, packet) | ||
| l.UdpPacketQueue <- packetCopy |
There was a problem hiding this comment.
What happens when the queue is full? Do we want to back pressure here, or would it be useful to drop the packet and increment a counter when this happens? For historical context, we had this idea before. The upside would be that it's easier to keep track of the dropped packets that way, instead of having to determine it from the kernel. At the time, we ended up not doing that in favor of other internal performance improvements.
Since it seems that you are still running into drops, I was wondering if this would be a good opportunity to revive that approach, especially since it looks like in your single-processor approach it would be fairly easy to do this here?
There was a problem hiding this comment.
After your comment, I added a counter for UDP packet drops and incremented it when channel is full. Could you please review it?
…g comment Signed-off-by: kullanici0606 <yakup.turgut@btk.gov.tr>
Summary of this PR
In this PR I am introducing a queue for raw statsd udp payloads and a worker to process them in another goroutine
Problem
As discussed in grafana/k6#2044, when both number of packets arriving and size of the packets are large, statds_exporter starts to drop UDP packets because processing speed cannot catch the packet arrival speed
Solution
In the past, I implemented similar UDP packet processing application in Java and best practice was there to read packet in one thread and process packets in another thread(s), therefore I introduced similar logic here: I added a queue / channel (chan []byte) so that the goroutine reading the packets from UDP socket can enqueue them in order to minimize time spent per packet and packets are processed in another goroutine which reads from the queue / channel
Result
Packet drop rates decreased, most of the time no packet drops occur while in old version packets are always dropped for the case discussed in grafana/k6#2044
Before:
packets in tcpdump (udp.dstport == 9125) 21477207 packets are dropped
After:
packets in tcpdump (udp.dstport == 9125) 21384No packets are dropped
Future TODOs
Memory benchmarks discussed in #459