fix: Add option to select translator #10802

reimda · 2022-03-10T15:45:19Z

This PR adds back the ability to call netsnmp's snmptranslate and snmptable to translate names and OIDs.

Telegraf recently switched translation to use gosmi to address performance concerns. (see #9518, released in 1.21.0) This resulted in translation not being backward compatible with the previous release, 1.20.4.

This PR makes gosmi translation a config option, but defaults to netsnmp translation.

Users of 1.21.0 and later who have transitioned to gosmi will need to set translator = "gosmi" in their agent config. Users who have not upgraded past 1.20.4 in order to keep using netsnmp translation should be able to use this without config changes.

Hipska

Some remarks before continuing..

plugins/inputs/snmp/snmp.go

Hipska · 2022-03-15T16:14:25Z

plugins/inputs/snmp/netsnmp.go

+func NewNetsnmpTranslator() *netsnmpTranslator {
+	return &netsnmpTranslator{}
+}


Could maybe return error when net-snmp commands are not found?

Could be a good idea, but this isn't how it worked before so I'm not going to do it in this PR. I'd hate to break things for people who (for example) only use numeric oids.

You know telegraf is doing lookups in that case as well, right? (in order to get a name and conversion)

Hipska · 2022-03-15T16:16:36Z

plugins/inputs/snmp/snmp.go

+	if s.Translator == "gosmi" {
+		s.translator, err = NewGosmiTranslator(s.Path, s.Log)
+		if err != nil {
+			return err
+		}
+	} else {
+		s.translator = NewNetsnmpTranslator()
 	}


This could maybe better done in a separate method? func (s *Snmp) initTranslator() error { Then this Init method could return error fmt.Errorf("initialising translator %s: %w", s.Translator, err)

It's only done once so I don't think it needs a separate method. There's a similar pattern in snmp_trap.go too, but it uses a different interface.

Hipska · 2022-03-15T16:25:36Z

plugins/processors/ifname/ifname.go

+	if d.Translator == "gosmi" {
+		var err error
+		d.translator, err = si.NewGosmiTranslator(d.Path, d.Log)
+		if err != nil {
+			return err
+		}
+	} else {
+		d.translator = si.NewNetsnmpTranslator()
+	}
+


This doesn't make much sense, since I made the plugin independent from any MIBs.

Although it's independent from MIBS because it hard codes numeric OIDs, the table and field initialization code still needs a translator. Inside those init functions, I don't expect any translation to happen. I did change ifname's code so it doesn't switch between translators.

You made the init functions depenant on a translator interface in this PR, so it can still be fixed.

Hipska · 2022-03-17T08:11:29Z

plugins/inputs/snmp/snmp.go

+func (s *Snmp) SetTranslator(name string) {
+	s.Translator = name
+}


What about calling this InitTranslator and initiating the translator, returning error on failure?

This is kind of an ugly hack to share the agent's translator setting with the plugins. I didn't want to add more error handling code to the agent to handle an error where it calls this function. There's already a well defined way for inputs to report config errors to the agent- Init func's return value.

Hipska · 2022-03-17T08:12:25Z

internal/snmp/config.go

@@ -13,7 +13,7 @@ type ClientConfig struct {
 	// Path to mib files
 	Path []string `toml:"path"`
 	// Translator implementation
-	Translator string `toml:"translator"`
+	Translator string `toml:"-"`


Not needed to be exported.

Suggested change

Translator string `toml:"-"`

translatorType string

plugins/inputs/snmp/snmp.go

plugins/inputs/snmp/snmp_test.go

Hipska · 2022-03-17T08:17:52Z

plugins/processors/ifname/ifname.go

+	// Since OIDs in this plugin are always numeric there is no need
+	// to translate.
+	d.translator = si.NewNetsnmpTranslator()


Then why initiating it anyway?

Just because it needs to be something. I don't expect it to be used. We should consider changing ifname so it doesn't call table Init() at all. Then we won't need a translator at all.

As said before, better would be the table.Init not expect a translator as argument.

What do you think about that @reimda?

I don't see how we can make table.Init() not expect a translator. Here in ifname, Init() shouldn't need to look up anything in the mibs because we only use numeric oids, but in the main use case in the snmp input allows names so it does need to use mibs.

Maybe here in ifname we should change the code to manually initialize the table and then not call Init() at all. That would probably mean removing makeTableNoMock() completely and changing makeTable() to fill in all the fields of snmp.Table that table.Init() would.

Easiest way would be by adding translator to the struct. But don't know if that is a good thing memory-wise.

Edit: It looks like the call of Init shouldn't even be needed, as we already initialise all necessary parameters. The only problem remaining would be the call of Table.Build that expects a translator, but it would not be called in case of ifName plugin usage, so I'm wondering if you can just pass a null value?

Hipska · 2022-03-17T08:19:00Z

Hi @reimda, great work! Could you please give some feedback on my comments?

config/config.go

telegraf-tiger · 2022-03-17T21:18:21Z

Download PR build artifacts for linux_amd64.tar.gz, darwin_amd64.tar.gz, and windows_amd64.zip.
Downloads for additional architectures and packages are available below.

☺️ This pull request doesn't significantly change the Telegraf binary size (less than 1%)

📦 Click here to get additional PR build artifacts

Artifact URLs

DEB	RPM	TAR GZ	ZIP
amd64.deb	aarch64.rpm	darwin_amd64.tar.gz	windows_amd64.zip
arm64.deb	armel.rpm	darwin_arm64.tar.gz	windows_i386.zip
armel.deb	armv6hl.rpm	freebsd_amd64.tar.gz
armhf.deb	i386.rpm	freebsd_armv7.tar.gz
i386.deb	ppc64le.rpm	freebsd_i386.tar.gz
mips.deb	riscv64.rpm	linux_amd64.tar.gz
mipsel.deb	s390x.rpm	linux_arm64.tar.gz
ppc64el.deb	x86_64.rpm	linux_armel.tar.gz
riscv64.deb		linux_armhf.tar.gz
s390x.deb		linux_i386.tar.gz
		linux_mips.tar.gz
		linux_mipsel.tar.gz
		linux_ppc64le.tar.gz
		linux_riscv64.tar.gz
		linux_s390x.tar.gz
		static_linux_amd64.tar.gz

reimda · 2022-03-17T22:25:21Z

Hi @reimda, great work! Could you please give some feedback on my comments?

Hi @Hipska, thanks for taking a look at the code. I just got it ready enough to review and marked it not a draft. A few of the things you the things you asked about were just because it wasn't ready yet.

You shared some improvements that could be good ideas on their own, but I don't want to add to this PR. This PR's only job is to revert to using netsnmp by default. My focus is to go back to behavior of 1.20.4 because of the compatibility issues that came up when we switched to gosmi. I also want to keep the option of using gosmi for others who found it an improvement, but require them to explicitly choose it. Making other changes while doing this makes everything more confusing for users and risks adding bugs.

reimda · 2022-03-18T03:42:24Z

Since PR builds are not available at the moment I'm going to merge this even though it still needs more review, so it will be in the nightly build and available for testing. I will make a follow-up PR for any changes required by reviews and testing.

Hipska · 2022-03-18T14:20:28Z

internal/snmp/translator.go

+
+type TranslatorPlugin interface {
+	SetTranslator(name string) // Agent calls this on inputs before Init
+}


This could be added in the translate.go file?

fix: Add option to select translator

2796f50

telegraf-tiger bot added the fix pr to fix corresponding bug label Mar 10, 2022

Hipska added area/snmp plugin/input 1. Request for new input plugins 2. Issues/PRs that are related to input plugins labels Mar 10, 2022

reimda added 5 commits March 10, 2022 11:14

Clean up and disable linter

5f73405

use translator in ifname processor

5c8e2ea

Use exported gosmi translator constructor

6f664a5

disable linter

dac215c

Add translator setting to snmp_trap plugin

c51e199

Hipska reviewed Mar 15, 2022

View reviewed changes

Hipska self-assigned this Mar 15, 2022

reimda added 5 commits March 15, 2022 12:51

Add missing file

bf61b86

gofmt

cdaadd9

Remove unused test functions

997ad84

ignore confusing naming linter warning

8c83ee8

Switch to agent level translator setting instead of per-plugin setting

8c01814

Hipska suggested changes Mar 17, 2022

View reviewed changes

Hipska reviewed Mar 17, 2022

View reviewed changes

config/config.go Show resolved Hide resolved

reimda added 3 commits March 17, 2022 09:03

Add missing file

73bb2dd

fake change to trigger github after its outage

83929a5

Use switch to make the linter happy

a3a4c1c

reimda marked this pull request as ready for review March 17, 2022 22:25

reimda requested a review from powersj March 17, 2022 22:44

reimda merged commit 77040ef into master Mar 18, 2022

reimda deleted the snmp-translator-setting branch March 18, 2022 03:43

Hipska reviewed Mar 18, 2022

View reviewed changes

This was linked to issues Mar 23, 2022

SNMP not working since upgrading to 1.21.4 #10810

Closed

MIBS not Loaded after Update to 1.21.2 #10444

Closed

SNMP tables are not collected #10445

Closed

reimda mentioned this pull request Apr 11, 2022

snmp gloriously broken in 1.22 again. #10955

Closed

MyaLongmire pushed a commit that referenced this pull request Jul 6, 2022

fix: Add option to select translator (#10802)

78ae3d9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Add option to select translator #10802

fix: Add option to select translator #10802

reimda commented Mar 10, 2022 •

edited

Loading

Hipska left a comment

Hipska Mar 15, 2022

reimda Mar 17, 2022

Hipska Mar 18, 2022

Hipska Mar 15, 2022

reimda Mar 17, 2022

Hipska Mar 15, 2022

reimda Mar 17, 2022

Hipska Mar 18, 2022

Hipska Mar 17, 2022

reimda Mar 17, 2022

Hipska Mar 17, 2022

Hipska Mar 17, 2022

reimda Mar 17, 2022

Hipska Mar 18, 2022 •

edited

Loading

Hipska Apr 4, 2022

reimda Apr 4, 2022

Hipska Apr 5, 2022 •

edited

Loading

Hipska commented Mar 17, 2022

telegraf-tiger bot commented Mar 17, 2022

Artifact URLs

reimda commented Mar 17, 2022

reimda commented Mar 18, 2022

Hipska Mar 18, 2022

fix: Add option to select translator #10802

fix: Add option to select translator #10802

Conversation

reimda commented Mar 10, 2022 • edited Loading

Hipska left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hipska Mar 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hipska Apr 5, 2022 • edited Loading

Choose a reason for hiding this comment

Hipska commented Mar 17, 2022

telegraf-tiger bot commented Mar 17, 2022

Artifact URLs

reimda commented Mar 17, 2022

reimda commented Mar 18, 2022

Choose a reason for hiding this comment

reimda commented Mar 10, 2022 •

edited

Loading

Hipska Mar 18, 2022 •

edited

Loading

Hipska Apr 5, 2022 •

edited

Loading