-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Added text collector conversion for ipmitool output. #746
Conversation
Oh, nice. Awk script, bold choice. 😄 |
text_collector_examples/ipmitool
Outdated
|
||
# $3 is type field | ||
$3 ~ /degrees C/ { | ||
printf("node_physical_temperature_celcius{sensor=\"%s\"} %f\n", $1, $2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think node_ipmi_
might be a better choice here.
Looks good, my only worry is that the output of If the sensor output wasn't sorted,
|
Oh, I tested one my systems, it looks like the hex conversion doesn't work right:
This returns:
|
I don't understand why the order is important. I might be able to fix it if I do. Wouldn't it be up to the query/report to make sense of the context of the readings? Are you able to send me your platform details? Seems |
Seems |
Order is important because Prometheus will drop the scrape if metric names come in out of order. It seems like the order is ok for the one system I tested, but it's a concern I have. If you look at a typical metric, we also have the The platform I tested on was Ubuntu 16.04, |
text_collector_examples/ipmitool
Outdated
delete a["metric_count"] | ||
|
||
if (name != "status") { | ||
printf("# TYPE %s%s gauge\n", namespace, name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also needs to print a # HELP metric_name Something
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy to add something. Was going to ask about this... Is a comment like , "Readings from the {voltage,temperature,etc} sensors via ipmitool, values are machine-dependent" valuable? (What I would add, if left to my own devices.) I don't think I can interpret very well beyond this. Is it desirable to have a comment that adds no additional information?
Additionally, I'd like to mention about the shebang... I've defaulted it to work in GAWK, because that's the most likely target, but the shebang syntax doesn't work on BSD. It's common to patch this in the ports tree though, and alternate syntax is provided.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Readings from the {voltage,temperature,etc} sensors via ipmitool
should be sufficient. We don't really do anything with this data, yet, but having it there is useful for future-proofing.
I think the shebang is fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about skipping the type on status? I feel that because there is no linear relationship between possible values, it isn't really a gauge (nor any of the other possible types, of course). Is it simply that the value goes both up and down that makes it a gauge?
text_collector_examples/ipmitool
Outdated
|
||
printf("# HELP %s%s %s sensor reading from ipmitool\n", namespace, name, friendly[name]); | ||
|
||
if (name != "status") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this not printing the type? The status is a gauge. From a Prometheus perspective, basically anything that isn't a monotonic increasing counter is considered a gauge.
I would remove the if
so that you always get a TYPE
line.
Tested on one of my servers, output looks great.
Also passes promtool check metrics mode. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks for the review! 👍 |
* Added text collector conversion for ipmitool output. * Sort metrics before exporting, add namespace. * Added HELP string, tidy up a bit. * Make status a gauge.
Converts output of
ipmitool sensor
to prometheus format.e.g.:
ipmitool sensor | ./ipmitool > ipmitool.prom