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

Ipmi.v2.schema #4450

Merged
merged 4 commits into from
Jul 31, 2018
Merged

Ipmi.v2.schema #4450

merged 4 commits into from
Jul 31, 2018

Conversation

jhg03a
Copy link
Contributor

@jhg03a jhg03a commented Jul 22, 2018

Required for all PRs:

  • Signed CLA.
  • Associated README.md updated.
  • Has appropriate unit tests.

Fixes #3416

@danielnelson danielnelson added the feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin label Jul 26, 2018
Copy link
Contributor

@danielnelson danielnelson left a comment

Choose a reason for hiding this comment

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

Thanks for taking care of this @jhg03a, here are a few suggestions.

@envy How does the proposed output look to you?

@@ -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
Copy link
Contributor

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.

// each line will look something like
// Planar VBAT | 3.05 Volts | ok
lines := strings.Split(string(out), "\n")
lines := strings.Split(cmdOut, "\n")
Copy link
Contributor

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.

}
}

acc.AddFields("ipmi_sensor", fields, tags, time.Now())
Copy link
Contributor

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.


// 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)
Copy link
Contributor

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.

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"])
Copy link
Contributor

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.

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
Copy link
Contributor

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.

@envy
Copy link

envy commented Jul 31, 2018

Hi, sorry for the late reply.

The solution with the entity ids looks good!

@glinton glinton added this to the 1.8.0 milestone Jul 31, 2018
@glinton glinton merged commit b93460d into influxdata:master Jul 31, 2018
rgitzel pushed a commit to rgitzel/telegraf that referenced this pull request Oct 17, 2018
otherpirate pushed a commit to otherpirate/telegraf that referenced this pull request Mar 15, 2019
@sspaink sspaink mentioned this pull request Feb 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants