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.

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

HRULE objects broken in some cases #2662

Closed
eschoeller opened this issue May 1, 2019 · 13 comments
Closed

HRULE objects broken in some cases #2662

eschoeller opened this issue May 1, 2019 · 13 comments
Labels
bug Undesired behaviour resolved A fixed issue
Milestone

Comments

@eschoeller
Copy link

This was working on whichever version I was on previously (~1.2.0). The problem is that SNMP is returning "percent" since I include the MIB by default in all queries. This is a regression of several of my other bug reports, and likely the exact opposite behavior of what I'm looking for in my other open issue (aka I do need "percent" to come through)

I really need to go back to the way things were working previously, which SNMP commands in Cacti fetched MIB resolvable units and which ones didn't.

RRDtool Command:
/usr/local/rrdtool/bin/rrdtool graph - \
--imgformat=PNG \
--start='-86400' \
--end='-300' \
--pango-markup  \
--title='device - CD_73,75,77 - Out of Balance' \
--vertical-label='percent' \
--slope-mode \
--base=1000 \
--height=130 \
--width=550 \
--tabwidth '30' \
--rigid \
--alt-autoscale \
COMMENT:"From 2019/04/29 20\:55\:01 To 2019/04/30 20\:50\:01\c" \
COMMENT:"  \n" \
--color BACK#F3F3F3 \
--color CANVAS#FDFDFD \
--color SHADEA#CBCBCB \
--color SHADEB#999999 \
--color FONT#000000 \
--color AXIS#2C4D43 \
--color ARROW#2C4D43 \
--color FRAME#2C4D43 \
--border 1 --font TITLE:10: \
--font AXIS:8: \
--font LEGEND:8: \
--font UNIT:8: \
--font WATERMARK:8: \
--slope-mode \
DEF:a='/cacti/cacti-1.2.3-prod/rra/device_cordoutofbalance_70099.rrd':'CordOutOfBalance':AVERAGE \
CDEF:cdefa='a,10,/' \
LINE1:cdefa#2175D9FF:'Circuit(BA)'  \
GPRINT:cdefa:LAST:'Current\:%8.2lf %s'  \
GPRINT:cdefa:MIN:'Min\:%8.2lf %s'  \
GPRINT:cdefa:AVERAGE:'Avg\:%8.2lf %s'  \
GPRINT:cdefa:MAX:'Max\:%8.2lf %s\n'  \
HRULE:15 percent#FF5F00FF:'High Warning\: 15 percent' \
HRULE:20 percent#FF0000FF:'High Alarm\: 20 percent'
RRDtool Says:
ERROR: 'percent#FF5F00FF' is not a valid function name in percent#FF5F00FF:High Warning\: 15 percent
@eschoeller
Copy link
Author

OR, I guess I should say, Cacti should always be accepting an integer and textual response from SNMP, and storing it, but knowing when to strip out the text and when not to, is important. So in the case of an HRULE, obviously we only need an integer. But for displaying information on a data query in the graph creation page, the text is important. Not having that is also breaking some of my automation rules since I rely on "found(0)" instead of just "0".

@cigamit
Copy link
Member

cigamit commented May 5, 2019

Where are those hrule values coming from. Show the 'raw' value from the template. Seems like a thold issue more so than a Cati issue as we are talking 'High Warning' and 'High Alarm'. Show the thold edit screen for the thold here. Not sure where percent is coming from.

@cigamit
Copy link
Member

cigamit commented May 5, 2019

Take that back. It's a graph template. Let's see your text format for the HRULE...

@eschoeller
Copy link
Author

image
image

How does that look?

@eschoeller
Copy link
Author

Many of these graphs still work. Here's an example:
image

After I re-run the data query behind this graph, it breaks. So something with the way 1.2.3 is storing the data for HRULEs, it's bringing along the "percent" which it shouldn't be.
image

@eschoeller
Copy link
Author

Ah, yuck and check this out:
image

I think a lot of my current issues are related.

@cigamit
Copy link
Member

cigamit commented May 6, 2019

Looks like the regex trim is not working as it should in lib/snmp.php. Did you merge correctly, or hand edit? If you look at the other ticket, you will see the issue. For the HRULE, you will likely need to extend your XML to include a value regex for the numeric data, and a normal walk for something that you want to display on the graph as text.

@cigamit
Copy link
Member

cigamit commented May 11, 2019

I'm going to close now. However, you will need to include two versions of the percent column, one that uses a regex to remove the 'percent' and the other that includes the full text including the percent. The lib/snmp.php has been updated to fix this other issue. So, after you make this change, your hrules will work again.

@cigamit cigamit closed this as completed May 11, 2019
@eschoeller
Copy link
Author

eschoeller commented May 11, 2019 via email

@cigamit
Copy link
Member

cigamit commented May 11, 2019

So, the issue is that we were performing trim's on the walks which was leading to bad results. We assumed that since a walk does not have to be numeric, we did not trim the data of non-numeric trailing data.

I guess, we could add an XML attribute to say "If the data could be numeric, make it so", which would essentially revert the legacy behavior. However, we will not make that the default. So, you will have to update some XML files.

How does that sound?

@cigamit cigamit reopened this May 11, 2019
@eschoeller
Copy link
Author

eschoeller commented May 12, 2019

So I assume this data is all being kept in the host_snmp_cache table, specifically in the field_value column. I do like having the "12 percent" entered here instead of just "12" so this numeric/text data can be displayed in an rrdtool COMMENT as well as in the data query output table in the "create graphs" page. It's more informative and functional in my opinion.

My thought is that Cacti should do its best to pass valid data over to rrdtool wherever it can, otherwise throw a Cacti error instead of an rrdtool error. I realize this isn't always possible, but generally speaking I think the Cacti errors can be more informative than the rrdtool errors usually are.

An HRULE is always going to need a plain integer value, so in the case of a field_value having non-integer data present Cacti would check for this and filter it out and send just the integer to the HRULE directive. If it couldn't, then indicate that through a more comprehensive error "hey you're throwing garbage at the HRULE and I can't fix it, please look into that". I can't see a situation where this behavior would backfire against someone, but I'd certainly want to give it careful thought to avoid any regression against something else.

This logic could really be applied to a lot of rrdtool directives, so I realize that's a lot of extra work and more things that could go wrong. But I've been confronted by this type of issue several times over the years and it seems like doing some minor input validation on the inputs to rrdtool (and some very basic data filtering where possible) could really improve the overall flexibility and ease of use, especially for template developers.

Sorry I don't actually contribute any code to this effort, if I had a bit more time and better familiarity with the code base I could do it. Hopefully someday:) And then I'd have a much better idea of what's really possible and logical to implement.

@cigamit
Copy link
Member

cigamit commented May 12, 2019

Okay, I'm going to introduce something very targeted right now. It'll involve lib/functions.php and lib/rrd.php. Let me know how it works. This way, we can sort of restore legacy behavior. Sorry about all the troubles. The more I've given thought to this, the more I've realized what is missing both from Cacti and php-snmp. There is just not enough time to fix it all.

@cigamit cigamit changed the title [1.2.3] HRULE objects broken in some cases HRULE objects broken in some cases May 12, 2019
cigamit added a commit that referenced this issue May 12, 2019
* HRULE objects broken in some cases
* Also two missed escapeshell* change
@cigamit cigamit added bug Undesired behaviour resolved A fixed issue labels May 12, 2019
cigamit added a commit that referenced this issue May 13, 2019
@cigamit cigamit closed this as completed Jun 8, 2019
@netniV netniV added this to the v1.2.4 milestone Jun 8, 2019
@eschoeller
Copy link
Author

Confirmed working 1.2.4

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Undesired behaviour resolved A fixed issue
Projects
None yet
Development

No branches or pull requests

3 participants