-
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
fix(inputs.ipset): Parse lines with timeout #14262
Conversation
Parsing raw CLI output, and lines with a timeout shift the values over by two. This checks for timeout, and if it exists adds it as a field as well. fixes: influxdata#14259
This looks good, but is still potentially quite fragile if things change / other situations arise in the future. (I've not tested it yet) I'd suggest iterating over the elements in for i, txt := range data {
if txt == "timeout" {
val, err := strconv.ParseUint(data[i+1], 10, 64)
if err != nil {
acc.AddError(err)
}
fields["timeout"] = val
} else if txt == "packets" {
val, err := strconv.ParseUint(data[i+1], 10, 64)
if err != nil {
acc.AddError(err)
}
fields["packets_total"] = val
} else if txt == "bytes" {
val, err := strconv.ParseUint(data[i+1], 10, 64)
if err != nil {
acc.AddError(err)
}
fields["bytes_total"] = val
}
} ... skipping the first few (3x) fields would probably be okay, and skipping consumed values would help too. It's certainly more processing, but I would hope it's not considered excessive given the improved robustness. Thoughts? |
I completely agree, which is why I made my comment in the bug about the unfortunate situation of parsing CLI output.
Agreed again. I am hesitant to change any logic when the plugin is as old as it is and we have surprisingly not received any issues prior to this but can make the change. |
Download PR build artifacts for linux_amd64.tar.gz, darwin_amd64.tar.gz, and windows_amd64.zip. 👍 This pull request doesn't change the Telegraf binary size 📦 Click here to get additional PR build artifactsArtifact URLs |
Looks good to me, thank you! I hope to test this out tomorrow, and I'll give my thumbs up when confirmed... though I trust your testing. |
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.
Looks good to me. Thanks @powersj!
"tomorrow" ... works great, many thanks! |
(cherry picked from commit 7d79111)
Parsing raw CLI output, and lines with a timeout shift the values over by two. This checks for timeout, and if it exists adds it as a field as well.
fixes: #14259