Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Influxdb incorrect storage types #3354

Merged
merged 4 commits into from Apr 19, 2016
Merged

Influxdb incorrect storage types #3354

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a988e83
Version 2 of sending the correct data to influx. Also catching the ex…
geordish Apr 12, 2016
f9fda59
Corrected formatting.
geordish Apr 19, 2016
933e163
Corrected more formatting. I don't like your coding style! :)
geordish Apr 19, 2016
761f55b
Removed large unrequired loop.
geordish Apr 19, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
24 changes: 21 additions & 3 deletions includes/functions.php
View file
Open in desktop
Expand Up @@ -1252,10 +1252,28 @@ function function_check($function) {
return function_exists($function);
}

function force_influx_data($type,$data) {
if ($type == 'f' || $type == 'float') {
return(sprintf("%.1f",$data));
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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you change any } else {

to

}
else {

return floatval($data);
}
} else {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another } else { :)

return $data;
}

}// end force_influx_data

/**
Expand Down
12 changes: 10 additions & 2 deletions includes/influxdb.inc.php
View file
Open in desktop
Expand Up @@ -58,7 +58,10 @@ function influx_update($device,$measurement,$tags=array(),$fields) {
$tmp_tags[$k] = $v;
}
foreach ($fields as $k => $v) {
$tmp_fields[$k] = force_influx_data('f',$v);
$tmp_fields[$k] = force_influx_data($v);
if( $tmp_fields[$k] === null) {
unset($tmp_fields[$k]);
}
}

d_echo("\nInfluxDB data:\n");
Expand All @@ -76,7 +79,12 @@ function influx_update($device,$measurement,$tags=array(),$fields) {
$tmp_fields // optional additional fields
)
);
$result = $influxdb->writePoints($points);
try {
$result = $influxdb->writePoints($points);
} catch (Exception $e) {
d_echo("Caught exception: ", $e->getMessage(), "\n");
d_echo($e->getTrace());
}
}
else {
print $console_color->convert('[%gInfluxDB Disabled%n] ', false);
Expand Down
2 changes: 1 addition & 1 deletion includes/polling/netstats-snmp.inc.php
View file
Open in desktop
Expand Up @@ -54,7 +54,7 @@
$value = $data_array[0][$oid];
}
else {
$value = 'U';
$value = null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason this is changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
$fields[$oid] = $value;
}
Expand Down