fix for 'rrdtool plugin: illegal attempt to update ... (minimum one second step)' #4060
Conversation
| * past too much. */ | ||
| rf->rf_next_read = now; | ||
| /* Check, if `rf_next_read' is on the same second as 'now' or in the past */ | ||
| if (CDTIME_T_TO_TIME_T(rf->rf_next_read) <= CDTIME_T_TO_TIME_T(now)) { |
There was a problem hiding this comment.
Why the addition of CDTIME_T_TO_TIME_T()?
There was a problem hiding this comment.
CDTIME_T_TO_TIME_T() to round timestamp to nearest integer second.
E.g.:
now = 1667983304.2
rf->rf_next_read = 1667983304.4
It means, that current metric is collected at 1667983304.2 and next one will be collected at 1667983304.4
Technically, condition rf->rf_next_read < now is false but if we don't increment rf->rf_next_read, server gets metrics with these timestamps in sequence: 1667983304.2, 1667983304.4. rrdtool storage "resolution" is 1 second and the both timestamps are rounded to 1667983304. So rrdtool rejects whole batch of data, causing gap on graph and the error.
To avoid this we need to be sure that all sent timestamps round to different seconds.
There was a problem hiding this comment.
Collectd supports intervals smaller than one second. With this change, we'd cut that off.
| /* `rf_next_read' is on the same second as `now' or earlier. it can cause | ||
| * 'rrdtool plugin: illegal attempt to update...(minimum one second step)' | ||
| * on server so move it to the next second. */ | ||
| rf->rf_next_read = now + rf->rf_effective_interval; |
There was a problem hiding this comment.
Effective interval could be temporarily up to max interval, so use of rf_interval could be better.
There was a problem hiding this comment.
As I understand rf->rf_effective_interval is calculated dynamically on each iteration of read loop according to callback status. It reflects current state of plugin more accurately. Thus I decided to use it.
|
Should I take any actions or make changes for approval? |
|
Thank you for your PR! I would prefer to round the timestamps down in the rrd write plugin[1]; other write targets do support timestamps in sub-seconds resolution. [1] https://github.com/collectd/collectd/blob/main/src/rrdtool.c |
|
How are you going to handle situation if |
|
I'd then use either the first or the last metrics in rrd_write and document that behavior. The current behavior sort of uses the behavior of using the first data and to ignore the following within the same second (or: complain about receiving more data than supported). |
|
Current behavior of rrd_write is to ignore whole bunch of data (not only data with timestamps within the same second). In our clients we flush data to server once per 120 secs. So it leads to gaps of up to 120 secs of data. |
ChangeLog: collectd: 'rrdtool plugin: illegal attempt to update ... (minimum one second step)'
When rrdtool is used on collectd server for data storage it is crucial for clients to send metrics with strictly increasing timestamps. Equal adjacent timestamps cause error similar to this:
rrdtool plugin: rrd_update_r (/opt/collectd/rrd/testhost1/GenericJMX-jetty/gauge-idleThreads.rrd) failed: /opt/collectd/rrd/testhost1/GenericJMX-jetty/gauge-idleThreads.rrd: illegal attempt to update using time 1667670455 when last update time is 1667670455 (minimum one second step)