-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
Ipmi.v2.schema #4450
Ipmi.v2.schema #4450
Conversation
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.
plugins/inputs/ipmi_sensor/README.md
Outdated
@@ -41,19 +45,36 @@ ipmitool -I lan -H SERVER -U USERID -P PASSW0RD sdr | |||
|
|||
## Timeout for the ipmitool command to complete. Default is 20 seconds. | |||
timeout = "20s" | |||
|
|||
## Schema Version: (Optional, defaults to version 1) | |||
schemaVersion = 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.
Can you change this to be metric_version
, this will match the option I used in in the mysql
plugin for similar functionality. Make sure to use snake_case too.
plugins/inputs/ipmi_sensor/ipmi.go
Outdated
// each line will look something like | ||
// Planar VBAT | 3.05 Volts | ok | ||
lines := strings.Split(string(out), "\n") | ||
lines := strings.Split(cmdOut, "\n") |
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'll bet this is was an existing bug in Windows due to only allowing CR line endings. What would be ideal is if we switched to bufio.Scanner here.
plugins/inputs/ipmi_sensor/ipmi.go
Outdated
} | ||
} | ||
|
||
acc.AddFields("ipmi_sensor", fields, tags, time.Now()) |
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.
Call time.Now() once before the loop, then pass the same time in for all metrics. Usually doesn't matter due to timestamp rounding but it is still a good idea.
plugins/inputs/ipmi_sensor/ipmi.go
Outdated
|
||
// extractFieldsFromRegex consumes a regex with named capture groups and returns a kvp map of strings with the results | ||
func extractFieldsFromRegex(regex string, input string) map[string]string { | ||
re := regexp.MustCompile(regex) |
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.
We should compile all the regex once at the package level, then reuse the compiled regex here.
plugins/inputs/ipmi_sensor/ipmi.go
Outdated
descriptionResults := extractFieldsFromRegex(`^(?P<analogValue>[0-9.]+)\s(?P<analogUnit>.*)|(?P<status>.+)|^$`, trim(ipmiFields["description"])) | ||
// This is an analog value with a unit | ||
if descriptionResults["analogValue"] != "" && len(descriptionResults["analogUnit"]) >= 1 { | ||
fields["value"] = aToFloat(descriptionResults["analogValue"]) |
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 if the value cannot be converted to a float then we should skip the line.
plugins/inputs/ipmi_sensor/README.md
Outdated
ipmi_sensor,status_code=ok,unit=watts,status_desc=presence_detected,host=foo.bar.com,name=power_supply_1,entity_id=10.1 value=110 1517125474000000000 | ||
ipmi_sensor,host=foo.bar.com,name=power_supply_2,entity_id=10.2,status_code=ok,unit=watts,status_desc=presence_detected value=125 1517125474000000000 | ||
ipmi_sensor,name=power_supplies,entity_id=10.3,status_code=ok,status_desc=fully_redundant,host=foo.bar.com value=0 1517125474000000000 | ||
ipmi_sensor,unit=percent,status_desc=transition_to_running,host=foo.bar.com,name=fan_1,entity_id=7.1,status_code=ok value=43.12 1517125474000000000 |
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.
If you regen this output with Telegraf 1.7, it should sort the tags. You can leave out the host tag since it is added everywhere.
Hi, sorry for the late reply. The solution with the entity ids looks good! |
Required for all PRs:
Fixes #3416