Cleanup snmp plugins #9055

Hipska · 2021-03-26T12:50:38Z

Required for all PRs:

Updated associated README.md.
Wrote appropriate unit tests.

Are there any more tests or parts of the plugin that could need cleanup?

telegraf-tiger

🤝 ✅ CLA has been signed. Thank you!

telegraf-tiger

Looks like new artifacts were built from this PR. Get them here!

Artifact URLs

ssoroka · 2021-04-13T21:14:24Z

Looks okay, I'm not sure that the regular expression is any clearer, but it's fine. Looks like we don't have test coverage of this conditional branch? Are you able to add a test for this?

Hipska · 2021-04-20T08:41:14Z

@ssoroka Tests that use this part of the code:

TestFieldInit
TestTableInit
TestSnmpInit
TestSnmpTranslateCache_miss (but that one is not testing the value on correctness)

Hipska · 2021-05-11T13:36:36Z

@ssoroka @reimda What about moving SnmpTranslate and related methods into internal/snmp?

reimda · 2021-05-11T14:43:29Z

@ssoroka @reimda What about moving SnmpTranslate and related methods into internal/snmp?

That's not a bad idea as long as snmp and snmp_trap end up using the same translate code. I don't know exactly why I didn't do it when I wrote snmp_trap.

Hipska · 2021-05-11T14:47:01Z

Great, will try to do that. It will also be good as preparation to switch to using gosmi.

telegraf-tiger · 2021-05-12T16:11:28Z

Looks like new artifacts were built from this PR. Get them here!

Artifact URLs

srebhan

Not sure I'm of help here. Code change looks sensible, but I've not been much into SNMP. :-)

reimda · 2021-05-25T21:27:26Z

Hi @Hipska is this meant to be a draft still? Maybe you're keeping it as a draft until you can move things to internal/snmp?

Hipska · 2021-05-26T05:07:03Z

Yes indeed, but don’t have much time for it now.

Hipska · 2021-07-01T08:32:22Z

As I understood @MyaLongmire will work on implementing gosmi, and so also move SnmpTranslate and other related methods to internal/snmp namespace. So I can make this PR as ready to review. Please do and merge.

plugins/inputs/snmp/snmp.go

reimda · 2021-09-02T20:43:11Z

plugins/inputs/snmp/snmp.go

-			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]


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.

plugins/inputs/snmp/snmp_test.go

Hipska · 2021-10-14T07:36:48Z

Closing since it will mostly be irrelevant when #9518 is merged

Hipska · 2021-11-08T09:26:17Z

Okay, I will reopen since that PR is now creating a new snmp plugin, so my improvements here are again relevant. @reimda can you finally merge please?

zak-pawel · 2021-11-08T10:43:19Z

plugins/inputs/snmp/snmp.go

@@ -9,6 +9,7 @@ import (
 	"math"
 	"net"
 	"os/exec"
+	"regexp"


@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

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

Hipska · 2021-12-08T09:43:58Z

Closing again since #9518 got merged and did a lot of cleanup as well

Hipska added 2 commits March 26, 2021 13:42

Parse OID with regex instead of substring lookups

498c9d3

Cleanup snmp tests

e564d92

Hipska added discussion Topics for discussion feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin area/snmp plugin/input 1. Request for new input plugins 2. Issues/PRs that are related to input plugins labels Mar 26, 2021

telegraf-tiger bot approved these changes Mar 26, 2021

View reviewed changes

Hipska marked this pull request as draft March 26, 2021 12:50

Hipska changed the title ~~Cleanup snmp input~~ Cleanup snmp plugin Mar 26, 2021

telegraf-tiger bot reviewed Mar 26, 2021

View reviewed changes

ivorybilled approved these changes Mar 26, 2021

View reviewed changes

Hipska marked this pull request as ready for review April 20, 2021 06:52

Hipska requested review from reimda and ssoroka April 20, 2021 08:53

Hipska requested a review from srebhan May 5, 2021 10:26

Hipska removed the discussion Topics for discussion label May 11, 2021

Hipska marked this pull request as draft May 11, 2021 14:44

Hipska removed the request for review from srebhan May 11, 2021 14:44

Hipska changed the title ~~Cleanup snmp plugin~~ Cleanup snmp plugins May 11, 2021

Cleanup snmp walk error and comments

dc53047

srebhan reviewed May 18, 2021

View reviewed changes

Hipska marked this pull request as ready for review July 1, 2021 08:32

Hipska requested review from reimda and removed request for reimda and ssoroka July 1, 2021 08:32

reimda reviewed Sep 2, 2021

View reviewed changes

Hipska closed this Oct 14, 2021

Hipska reopened this Nov 8, 2021

Hipska requested a review from reimda November 8, 2021 09:27

zak-pawel reviewed Nov 8, 2021

View reviewed changes

Hipska closed this Dec 8, 2021

Hipska deleted the snmp/cleanup branch December 8, 2021 09:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup snmp plugins #9055

Cleanup snmp plugins #9055

Hipska commented Mar 26, 2021

telegraf-tiger bot left a comment

telegraf-tiger bot left a comment

ssoroka commented Apr 13, 2021

Hipska commented Apr 20, 2021 •

edited

Loading

Hipska commented May 11, 2021

reimda commented May 11, 2021

Hipska commented May 11, 2021

telegraf-tiger bot commented May 12, 2021

Artifact URLs

srebhan left a comment

reimda commented May 25, 2021

Hipska commented May 26, 2021

Hipska commented Jul 1, 2021

reimda Sep 2, 2021

Hipska commented Oct 14, 2021

Hipska commented Nov 8, 2021

zak-pawel Nov 8, 2021

Hipska Nov 8, 2021

Hipska commented Dec 8, 2021

Cleanup snmp plugins #9055

Cleanup snmp plugins #9055

Conversation

Hipska commented Mar 26, 2021

Required for all PRs:

telegraf-tiger bot left a comment

Choose a reason for hiding this comment

telegraf-tiger bot left a comment

Choose a reason for hiding this comment

Artifact URLs

ssoroka commented Apr 13, 2021

Hipska commented Apr 20, 2021 • edited Loading

Hipska commented May 11, 2021

reimda commented May 11, 2021

Hipska commented May 11, 2021

telegraf-tiger bot commented May 12, 2021

Artifact URLs

srebhan left a comment

Choose a reason for hiding this comment

reimda commented May 25, 2021

Hipska commented May 26, 2021

Hipska commented Jul 1, 2021

reimda Sep 2, 2021

Choose a reason for hiding this comment

Hipska commented Oct 14, 2021

Hipska commented Nov 8, 2021

zak-pawel Nov 8, 2021

Choose a reason for hiding this comment

Hipska Nov 8, 2021

Choose a reason for hiding this comment

Hipska commented Dec 8, 2021

Hipska commented Apr 20, 2021 •

edited

Loading