logstash-plugins/logstash-codec-collectd

Wrong assumptions decoding protocol

Closed this issue · 6 comments

octo commented

Hi, this is a follow-up to elastic/logstash#2014 and #6, which I don't believe to be fixed.

The coment in line 424 in collectd.rb reads:

We've reached a new plugin, delete everything except for the the host field, because there's only one per packet and the timestamp field, because that one goes in front of the plugin

Pretty much everything in this comment is wrong:

  • The packet is fully incremental, i.e. you cannot clear any field while in the middle of parsing it.
  • The end of one metric and beginning of the following metric is indicated by the VALUES part, not the PLUGIN part. At this point all the fields of that metric are known and the metric should be copied to somewhere. You must not clear any fields afterwards though (with the exception of the values themselves, should your code set them in the struct / object / $state).
  • The order of the fields is not defined, so long as sequential building of the state arrives at the right value. I.e. PLUGIN may well come before the TIME – or not at all if it has the same value as the previous metric.
  • If there are metrics from multiple hosts in one plugin, e.g. because when using the network plugin in proxy (forwarding) mode or using the SNMP plugin, then HOST may well be repeated.

Creating a set of test vectors for the binary protocol is on my todo list and I'll happily drop a reference here once I have that.

Thanks for all your awesome work!

jordansissel commented

@octo Thanks for helping clarify this! <3 <3 <3

untergeek commented

Question: If true, doesn't this indicate that multithreading is a no-no? We could potentially read packets out of order and therefore overwrite fields out of band...

octo commented

@untergeek what I outlined above only applies to parsing a single packet. State does not carry over to other packets, i.e. each packet starts with a clean slate / empty state.

untergeek commented

I believe these issues are addressed in #16

Care to comment, @octo?

octo commented

@untergeek the Ruby is weak in me, but as far as I can tell the patch looks reasonable. I think we can close this bug for now and trust in actual users opening a new one if a corner case slipped through ;) Thanks for taking care of this!

untergeek commented

Thanks @octo