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.

Sign up for GitHub

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

Cleanup snmp plugins #9055

Closed
wants to merge 3 commits into from
Closed

Cleanup snmp plugins #9055

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
498c9d3
Parse OID with regex instead of substring lookups
Hipska Mar 26, 2021
e564d92
Cleanup snmp tests
Hipska Mar 26, 2021
dc53047
Cleanup snmp walk error and comments
Hipska May 12, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 4 additions & 12 deletions plugins/inputs/snmp/snmp.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"math"
"net"
"os/exec"
"regexp"
Copy link
Collaborator

Choose a reason for hiding this comment

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

@Hipska maybe in this cleanup you can include fixing linters findings?

plugins/inputs/snmp/snmp.go:1:9                  revive       import-shadowing: The name 'snmp' shadows an import name
plugins/inputs/snmp/snmp.go:8:2                  revive       imports-blacklist: should not use the following blacklisted import: "log"
plugins/inputs/snmp/snmp.go:376:27               revive       flag-parameter: parameter 'walk' seems to be a control flag, avoid control coupling
plugins/inputs/snmp/snmp.go:406:21               revive       flag-parameter: parameter 'walk' seems to be a control flag, avoid control coupling
plugins/inputs/snmp/snmp.go:594:15               unconvert    unnecessary conversion
plugins/inputs/snmp/snmp.go:692:10               revive       early-return: if c {...} else {... return } can be simplified to if !c { ... return } ...
plugins/inputs/snmp/snmp.go:748:1                revive       function-result-limit: maximum number of return results per function exceeded; max 3 but got 5
plugins/inputs/snmp/snmp.go:765:1                revive       function-result-limit: maximum number of return results per function exceeded; max 3 but got 5
plugins/inputs/snmp/snmp.go:836:1                revive       function-result-limit: maximum number of return results per function exceeded; max 3 but got 5
plugins/inputs/snmp/snmp.go:862:1                revive       function-result-limit: maximum number of return results per function exceeded; max 3 but got 5
plugins/inputs/snmp/snmp_mocks_test.go:47:3      revive       unhandled-error: Unhandled error in call to function fmt.Fprintf
plugins/inputs/snmp/snmp_mocks_test.go:48:3      revive       deep-exit: calls to os.Exit only in main() or init() functions
plugins/inputs/snmp/snmp_mocks_test.go:50:2      revive       unhandled-error: Unhandled error in call to function fmt.Printf
plugins/inputs/snmp/snmp_mocks_test.go:51:2      revive       unhandled-error: Unhandled error in call to function fmt.Fprintf
plugins/inputs/snmp/snmp_mocks_test.go:53:3      revive       deep-exit: calls to os.Exit only in main() or init() functions
plugins/inputs/snmp/snmp_mocks_test.go:55:2      revive       deep-exit: calls to os.Exit only in main() or init() functions
plugins/inputs/snmp/snmp_test.go:2:9             revive       import-shadowing: The name 'snmp' shadows an import name
plugins/inputs/snmp/snmp_test.go:15:2            revive       duplicated-imports: Package "github.com/influxdata/telegraf/internal/snmp" already imported
plugins/inputs/snmp/snmp_test.go:476:2           staticcheck  SA5001: should check returned error before deferring srvr.Close()
plugins/inputs/snmp/snmp_test.go:495:16          errcheck     Error return value of `srvr.WriteTo` is not checked
plugins/inputs/snmp/snmp_test.go:515:2           revive       unhandled-error: Unhandled error in call to function srvr.Close
plugins/inputs/snmp/snmp_test.go:526:2           staticcheck  SA5001: should check returned error before deferring srvr.Close()
plugins/inputs/snmp/snmp_test.go:545:16          errcheck     Error return value of `srvr.WriteTo` is not checked
plugins/inputs/snmp/snmp_test.go:565:2           revive       unhandled-error: Unhandled error in call to function srvr.Close
plugins/inputs/snmp/snmp_test.go:748:10          errcheck     Error return value of `s.Gather` is not checked
plugins/inputs/snmp/snmp_test.go:795:10          errcheck     Error return value of `s.Gather` is not checked

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ouch, I think I'm afraid I'm not able to do that. I could give it a try if really wanted.

"strconv"
"strings"
"sync"
Expand Down Expand Up @@ -912,19 +913,10 @@ func snmpTranslateCall(oid string) (mibName string, oidNum string, oidText strin
conversion = "ipaddr"
}
} else if strings.HasPrefix(line, "::= { ") {
objs := strings.TrimPrefix(line, "::= { ")
objs = strings.TrimSuffix(objs, " }")
re := regexp.MustCompile(`(?:\w+\()?(\d+)\)?`)

for _, obj := range strings.Split(objs, " ") {
if len(obj) == 0 {
continue
}
if i := strings.Index(obj, "("); i != -1 {
obj = obj[i+1:]
oidNum += "." + obj[:strings.Index(obj, ")")]
} else {
oidNum += "." + obj
}
for _, match := range re.FindAllStringSubmatch(line, -1) {
oidNum += "." + match[1]
Comment on lines -915 to +914
Copy link
Contributor

Choose a reason for hiding this comment

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

This makes the code shorter but I'm not sure it's more maintainable to use regexp instead of manually splitting the string. Do we have unit tests that cover this? I'm not sure it does the exact same thing as before.

}
break
}
Expand Down
31 changes: 8 additions & 23 deletions plugins/inputs/snmp/snmp_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,29 +211,14 @@ func TestSnmpInit_noTranslate(t *testing.T) {
err := s.init()
require.NoError(t, err)

assert.Equal(t, ".1.1.1.1", s.Fields[0].Oid)
assert.Equal(t, "one", s.Fields[0].Name)
assert.Equal(t, true, s.Fields[0].IsTag)

assert.Equal(t, ".1.1.1.2", s.Fields[1].Oid)
assert.Equal(t, "two", s.Fields[1].Name)
assert.Equal(t, false, s.Fields[1].IsTag)

assert.Equal(t, ".1.1.1.3", s.Fields[2].Oid)
assert.Equal(t, ".1.1.1.3", s.Fields[2].Name)
assert.Equal(t, false, s.Fields[2].IsTag)

assert.Equal(t, ".1.1.1.4", s.Tables[0].Fields[0].Oid)
assert.Equal(t, "four", s.Tables[0].Fields[0].Name)
assert.Equal(t, true, s.Tables[0].Fields[0].IsTag)

assert.Equal(t, ".1.1.1.5", s.Tables[0].Fields[1].Oid)
assert.Equal(t, "five", s.Tables[0].Fields[1].Name)
assert.Equal(t, false, s.Tables[0].Fields[1].IsTag)

assert.Equal(t, ".1.1.1.6", s.Tables[0].Fields[2].Oid)
assert.Equal(t, ".1.1.1.6", s.Tables[0].Fields[2].Name)
assert.Equal(t, false, s.Tables[0].Fields[2].IsTag)
assert.Equal(t, Field{Oid: ".1.1.1.1", Name: "one", IsTag: true, initialized: true}, s.Fields[0])
assert.Equal(t, Field{Oid: ".1.1.1.2", Name: "two", IsTag: false, initialized: true}, s.Fields[1])
assert.Equal(t, Field{Oid: ".1.1.1.3", Name: ".1.1.1.3", IsTag: false, initialized: true}, s.Fields[2])

assert.Len(t, s.Tables[0].Fields, 3)
assert.Contains(t, s.Tables[0].Fields, Field{Oid: ".1.1.1.4", Name: "four", IsTag: true, initialized: true})
assert.Contains(t, s.Tables[0].Fields, Field{Oid: ".1.1.1.5", Name: "five", IsTag: false, initialized: true})
assert.Contains(t, s.Tables[0].Fields, Field{Oid: ".1.1.1.6", Name: ".1.1.1.6", IsTag: false, initialized: true})
Hipska marked this conversation as resolved.
Show resolved Hide resolved
}

func TestGetSNMPConnection_v2(t *testing.T) {
Expand Down