Influxdb incorrect storage types #3354

Merged
merged 4 commits into from Apr 19, 2016

Projects

None yet

4 participants

@geordish
Contributor

Resubmit of PR #3320

Version 2 of sending the correct data to influx. Also catching the exception that would stop updates working.

@LaurentDumont do you want to give this fix a go?

@geordish geordish Version 2 of sending the correct data to influx. Also catching the ex…
…ception that would stop updates working.
a988e83
@LaurentDumont
LaurentDumont commented Apr 18, 2016 edited

Ouf, sorry about the delay, I was running the Lan ETS in Montreal and I'm a zombie right now ;) - We did end up using LibreNMS and your branch for the monitoring but I couldn't find the time to pull the latest PR. I can definitely test the package this week end poke back.

@laf laf and 1 other commented on an outdated diff Apr 19, 2016
includes/polling/netstats-snmp.inc.php
@@ -54,7 +54,7 @@
$value = $data_array[0][$oid];
}
else {
- $value = 'U';
+ $value = null;
@laf
laf Apr 19, 2016 Member

Any reason this is changed?

@geordish
geordish Apr 19, 2016 Contributor

TBH, this entire loop can probably go. Its duplicating what is already done in includes/rrdtool.inc.php:rrdtool_update

This was changed as this will create the influx field as a string if it is the first data to be inserted. This is no good when the actual data if it is not defined is a numeric value.

@laf
laf Apr 19, 2016 Member

The U is for rrdtools' benefit rather than influx. Think it needs to go back to U.

@geordish
geordish Apr 19, 2016 Contributor

Sorry, I wasn't too clear.

In the rrd_update function which is called a little later, the following occurs:

        foreach ($options as $k => $v) {
            if (!is_numeric($v)) {
                $v = U;
            }

            $values[] = $v;
        }

This sets U for non numeric data before inserting into the RRD. This bit of code here is doing that again, but on a specific subset of the data.

In my opinion, munging the data for inclusion into a datastore should belong in that datastores insert function.

@laf
laf Apr 19, 2016 Member

In that case either revert the change and leave it as it was or remove the code if it's not needed.

@laf laf commented on an outdated diff Apr 19, 2016
includes/functions.php
+function force_influx_data($data) {
+ /*
+ * It is not trivial to detect if something is a float or an integer, and
+ * therefore may cause breakages on inserts.
+ * Just setting every number to a float gets around this, but may introduce
+ * inefficiencies.
+ * I've left the detection statement in there for a possible change in future,
+ * but currently everything just gets set to a float.
+ */
+
+ if (is_numeric($data)) {
+ // If it is an Integer
+ if (ctype_digit($data)) {
+ return floatval($data);
+ // Else it is a float
+ } else {
@laf
laf Apr 19, 2016 Member

Can you change any } else {

to

}
else {

@laf
Member
laf commented Apr 19, 2016

Couple of comments and then we can merge

@geordish geordish Corrected formatting.
f9fda59
@laf laf commented on an outdated diff Apr 19, 2016
includes/functions.php
+ * Just setting every number to a float gets around this, but may introduce
+ * inefficiencies.
+ * I've left the detection statement in there for a possible change in future,
+ * but currently everything just gets set to a float.
+ */
+
+ if (is_numeric($data)) {
+ // If it is an Integer
+ if (ctype_digit($data)) {
+ return floatval($data);
+ // Else it is a float
+ }
+ else {
+ return floatval($data);
+ }
+ } else {
@laf
laf Apr 19, 2016 Member

Another } else { :)

@geordish geordish Corrected more formatting. I don't like your coding style! :)
933e163
@laf
Member
laf commented Apr 19, 2016

The coding standards makes it nicer when adding more } elseif { statements as less code is changed to accomplish this leading to cleaner and clearer diff. You'll get over it.

@geordish geordish Removed large unrequired loop.
An example output that shows this is still working:

...
snmpOutTraps.0 = 0
snmpEnableAuthenTraps.0 = enabled
snmpSilentDrops.0 = 0
snmpProxyDrops.0 = 0

RRD Disabled RRD update /opt/librenms/rrd/xxx/netstats-snmp.rrd N:173688066:172651761:0:50736:0:0:0:0:0:0:0:210621835:0:167870188:2601931:0:0:0:4:74625425:0:0:0:0:0:172651443:0:U:0:0
...

(Note the U 3rd from the end)
761f55b
@laf laf merged commit 4d18c42 into librenms:master Apr 19, 2016

3 checks passed

Auto-Deploy Build finished. No test results found.
Details
Scrutinizer 1 new issues
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@geordish geordish deleted the geordish:influxdb-incorect-storage-typesv2 branch Oct 20, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment