limit perf array index length to 19 characters due to limitation in ds-name rrdtool #4731

Merged
merged 19 commits into from Nov 16, 2016

Projects

None yet

6 participants

@infotek
Contributor
infotek commented Oct 6, 2016

limit perf array index length to 19 characters due to limitation in ds-name rrdtool. nagios plugin check_mysql has performance counters that exceed a 19 character limit found in rrdtool.

infotek added some commits Oct 6, 2016
@infotek infotek limit perf array index length to 19 charaters due to limitation in ds…
…-name per rrdtool
1ed609e
@infotek infotek limit perf array index length to 19 charaters due to limitation in ds…
…-name per rrdtool
30999bf
@laf
Member
laf commented Oct 6, 2016

This needs a little more safety checks in. This at the moment has the chance that two DS' will have the same name if we just strip additional chars off. The new 19 char long name should be compared to the array to see if it exists and if it does, it will need changing again.

@laf
Member
laf commented Oct 6, 2016

p.s some code formatting issues are flagged by Travis, these will also need fixing.

infotek added some commits Oct 8, 2016
@infotek infotek Merge branch 'master' of https://github.com/librenms/librenms into is…
…sue-4729

merging in cisco updates
b709614
@infotek infotek create unique ds-name for rrdtool. avoid name collisions in perf array
e546700
@infotek infotek clean up style
c99dd97
@infotek
Contributor
infotek commented Oct 8, 2016

I thin I have address the issues concerning name collisions and style.

@infotek infotek clean up style white spaces
4f86c91
@infotek infotek declare $DS = array();
77bd88e
@infotek
Contributor
infotek commented Oct 8, 2016
@infotek infotek Merge branch 'master' of https://github.com/librenms/librenms into is…
…sue-4729
fd82ace
@laf
Member
laf commented Oct 11, 2016

I was trying to figure out how best to deal with this and the approach is right but I think it could be simplified in just using the service_id from the DB table. It's available as $service['service_id'] as the 19th char. It's always going to be unique so we avoid clashes. The only time this causes an issue is if someone removes a service and re-adds (but only if the name was > 19 chars anyway).

That would reduce the loops down and make things a bit cleaner - @librenms/reviewers ?

@infotek
Contributor
infotek commented Oct 11, 2016

After some discussion on irc. service id can be greater than 9 and does not offer sequential numbering to differentiate multiple performance counters per a single service.

@murrant
Contributor
murrant commented Oct 12, 2016

Remember that the number and order of datasets in an rrd basically needs to be fixed. The only time the dsname is used is when generating graphs.

dsnames not only need to be unique, but they need to be stable and deterministic.

@laf laf rebased
232bdd8
@laf
Member
laf commented Nov 3, 2016

I've just rebased this ready for merge but then did realise - we have no way to actually view the data in graph format as services doesn't seem to have that support or am I missing something.

@laf laf added the Service label Nov 3, 2016
@f0o
f0o approved these changes Nov 3, 2016 View changes
@infotek
Contributor
infotek commented Nov 3, 2016 edited

I would like to try and solve this in a slightly different manner. I think there is a chance that the array order will be rearranged. The problem with that is, during the update cycle the order numbers applied would go to the wrong field. example the following service perf (DS) names:

short,reallylonglonglonglong,other

the rrdcreate command will be run with a field order of

short,other,reallylonglonglong0

But the rrdupdate commands will continue to place values in the original order.
I and going to add another value to the array when dealing with the length to avoid the ordering issue.

@infotek
Contributor
infotek commented Nov 3, 2016 edited

My mistake. That is the fixed version... You can check functionality with the following code:

infotek/librenms_rrd_service_perf_test.php
https://gist.github.com/infotek/5345ecfbc09217dec4051bee2bd3cfa9

infotek added some commits Nov 5, 2016
@infotek infotek Moved ds normalization from function poll_service to function check_s…
…ervice librenms/includes/services.inc.php
bd6522b
@infotek infotek Changed echo to d_echo 902252b
@infotek infotek rebase
a5dbcbc
@infotek
Contributor
infotek commented Nov 5, 2016

php script that emulates changes to includes/services.inc.php for testing

https://gist.github.com/infotek/afca555ae101168bf91877efb32644ec

infotek added some commits Nov 8, 2016
@infotek infotek remove extra blank line :( 48b9c78
@infotek infotek extra blank line :(
ec4170a
@infotek infotek check_mysql Uptime should be rrd type GAUGE
ef9b530
@infotek
Contributor
infotek commented Nov 9, 2016
  1. during the nagios check check_service() in 'includes/services.inc.php' make sure ds_name does not exceed 19 characters while preserving the ds_name ordering.
  2. check_mysql performance output is normally ~351 characters worth of performance output. This data is placed into the database, table service, field service_ds. Alter database so that service_ds can accommodate a larger VARCHAR(400).
  3. make performance metrics with the string uptime and Uptime have the rrd type GAUGE.
  4. for check_mysql group similar data together in the same graph
@infotek infotek rebase 2016/11/12
5a16d3d
@scrutinizer-notifier

The inspection completed: 2 new issues, 1 updated code elements

@laf
Member
laf commented Nov 13, 2016

All good with me. @librenms/reviewers anyone else?

@laf laf merged commit b4693e5 into librenms:master Nov 16, 2016

2 checks passed

Auto-Deploy Build finished.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment