From b58926dd2658653cc3f9c84dfc68d18301379891 Mon Sep 17 00:00:00 2001 From: Patrick Hemmer Date: Thu, 1 Dec 2016 23:45:37 -0500 Subject: [PATCH] snmp: use a shared global translation cache Prevents the same data from being looked up multiple times. Also prevents multiple simultaneous lookups. closes #2115 closes #2104 --- CHANGELOG.md | 1 + plugins/inputs/snmp/snmp.go | 179 ++++++++++++++++++++++--------- plugins/inputs/snmp/snmp_test.go | 65 +++++++++++ 3 files changed, 196 insertions(+), 49 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a4a76e269..c3d3ef5f5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,7 @@ ### Bugfixes - [#2007](https://github.com/influxdata/telegraf/issues/2007): Make snmptranslate not required when using numeric OID. +- [#2104](https://github.com/influxdata/telegraf/issues/2104): Add a global snmp translation cache. ## v1.1.1 [2016-11-14] diff --git a/plugins/inputs/snmp/snmp.go b/plugins/inputs/snmp/snmp.go index 629f526bb..5426bf28c 100644 --- a/plugins/inputs/snmp/snmp.go +++ b/plugins/inputs/snmp/snmp.go @@ -8,6 +8,7 @@ import ( "os/exec" "strconv" "strings" + "sync" "time" "github.com/influxdata/telegraf" @@ -199,65 +200,22 @@ func (t *Table) init() error { return nil } -// init() populates Fields if a table OID is provided. +// initBuild initializes the table if it has an OID configured. If so, the +// net-snmp tools will be used to look up the OID and auto-populate the table's +// fields. func (t *Table) initBuild() error { if t.Oid == "" { return nil } - mibName, _, oidText, _, err := snmpTranslate(t.Oid) + _, _, oidText, fields, err := snmpTable(t.Oid) if err != nil { - return Errorf(err, "translating %s", t.Oid) + return Errorf(err, "initializing table %s", t.Oid) } if t.Name == "" { t.Name = oidText } - mibPrefix := mibName + "::" - oidFullName := mibPrefix + oidText - - // first attempt to get the table's tags - tagOids := map[string]struct{}{} - // We have to guess that the "entry" oid is `t.Oid+".1"`. snmptable and snmptranslate don't seem to have a way to provide the info. - if out, err := execCmd("snmptranslate", "-Td", oidFullName+".1"); err == nil { - lines := bytes.Split(out, []byte{'\n'}) - for _, line := range lines { - if !bytes.HasPrefix(line, []byte(" INDEX")) { - continue - } - - i := bytes.Index(line, []byte("{ ")) - if i == -1 { // parse error - continue - } - line = line[i+2:] - i = bytes.Index(line, []byte(" }")) - if i == -1 { // parse error - continue - } - line = line[:i] - for _, col := range bytes.Split(line, []byte(", ")) { - tagOids[mibPrefix+string(col)] = struct{}{} - } - } - } - - // this won't actually try to run a query. The `-Ch` will just cause it to dump headers. - out, err := execCmd("snmptable", "-Ch", "-Cl", "-c", "public", "127.0.0.1", oidFullName) - if err != nil { - return Errorf(err, "getting table columns for %s", t.Oid) - } - cols := bytes.SplitN(out, []byte{'\n'}, 2)[0] - if len(cols) == 0 { - return fmt.Errorf("unable to get columns for table %s", t.Oid) - } - for _, col := range bytes.Split(cols, []byte{' '}) { - if len(col) == 0 { - continue - } - col := string(col) - _, isTag := tagOids[mibPrefix+col] - t.Fields = append(t.Fields, Field{Name: col, Oid: mibPrefix + col, IsTag: isTag}) - } + t.Fields = append(t.Fields, fields...) return nil } @@ -841,8 +799,131 @@ func fieldConvert(conv string, v interface{}) (interface{}, error) { return nil, fmt.Errorf("invalid conversion type '%s'", conv) } +type snmpTableCache struct { + mibName string + oidNum string + oidText string + fields []Field + err error +} + +var snmpTableCaches map[string]snmpTableCache +var snmpTableCachesLock sync.Mutex + +// snmpTable resolves the given OID as a table, providing information about the +// table and fields within. +func snmpTable(oid string) (mibName string, oidNum string, oidText string, fields []Field, err error) { + snmpTableCachesLock.Lock() + if snmpTableCaches == nil { + snmpTableCaches = map[string]snmpTableCache{} + } + + var stc snmpTableCache + var ok bool + if stc, ok = snmpTableCaches[oid]; !ok { + stc.mibName, stc.oidNum, stc.oidText, stc.fields, stc.err = snmpTableCall(oid) + snmpTableCaches[oid] = stc + } + + snmpTableCachesLock.Unlock() + return stc.mibName, stc.oidNum, stc.oidText, stc.fields, stc.err +} + +func snmpTableCall(oid string) (mibName string, oidNum string, oidText string, fields []Field, err error) { + mibName, oidNum, oidText, _, err = snmpTranslate(oid) + if err != nil { + return "", "", "", nil, Errorf(err, "translating") + } + + mibPrefix := mibName + "::" + oidFullName := mibPrefix + oidText + + // first attempt to get the table's tags + tagOids := map[string]struct{}{} + // We have to guess that the "entry" oid is `oid+".1"`. snmptable and snmptranslate don't seem to have a way to provide the info. + if out, err := execCmd("snmptranslate", "-Td", oidFullName+".1"); err == nil { + lines := bytes.Split(out, []byte{'\n'}) + for _, line := range lines { + if !bytes.HasPrefix(line, []byte(" INDEX")) { + continue + } + + i := bytes.Index(line, []byte("{ ")) + if i == -1 { // parse error + continue + } + line = line[i+2:] + i = bytes.Index(line, []byte(" }")) + if i == -1 { // parse error + continue + } + line = line[:i] + for _, col := range bytes.Split(line, []byte(", ")) { + tagOids[mibPrefix+string(col)] = struct{}{} + } + } + } + + // this won't actually try to run a query. The `-Ch` will just cause it to dump headers. + out, err := execCmd("snmptable", "-Ch", "-Cl", "-c", "public", "127.0.0.1", oidFullName) + if err != nil { + return "", "", "", nil, Errorf(err, "getting table columns") + } + cols := bytes.SplitN(out, []byte{'\n'}, 2)[0] + if len(cols) == 0 { + return "", "", "", nil, fmt.Errorf("could not find any columns in table") + } + for _, col := range bytes.Split(cols, []byte{' '}) { + if len(col) == 0 { + continue + } + col := string(col) + _, isTag := tagOids[mibPrefix+col] + fields = append(fields, Field{Name: col, Oid: mibPrefix + col, IsTag: isTag}) + } + + return mibName, oidNum, oidText, fields, err +} + +type snmpTranslateCache struct { + mibName string + oidNum string + oidText string + conversion string + err error +} + +var snmpTranslateCachesLock sync.Mutex +var snmpTranslateCaches map[string]snmpTranslateCache + // snmpTranslate resolves the given OID. func snmpTranslate(oid string) (mibName string, oidNum string, oidText string, conversion string, err error) { + snmpTranslateCachesLock.Lock() + if snmpTranslateCaches == nil { + snmpTranslateCaches = map[string]snmpTranslateCache{} + } + + var stc snmpTranslateCache + var ok bool + if stc, ok = snmpTranslateCaches[oid]; !ok { + // This will result in only one call to snmptranslate running at a time. + // We could speed it up by putting a lock in snmpTranslateCache and then + // returning it immediately, and multiple callers would then release the + // snmpTranslateCachesLock and instead wait on the individual + // snmpTranlsation.Lock to release. But I don't know that the extra complexity + // is worth it. Especially when it would slam the system pretty hard if lots + // of lookups are being perfomed. + + stc.mibName, stc.oidNum, stc.oidText, stc.conversion, stc.err = snmpTranslateCall(oid) + snmpTranslateCaches[oid] = stc + } + + snmpTranslateCachesLock.Unlock() + + return stc.mibName, stc.oidNum, stc.oidText, stc.conversion, stc.err +} + +func snmpTranslateCall(oid string) (mibName string, oidNum string, oidText string, conversion string, err error) { var out []byte if strings.ContainsAny(oid, ":abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ") { out, err = execCmd("snmptranslate", "-Td", "-Ob", oid) diff --git a/plugins/inputs/snmp/snmp_test.go b/plugins/inputs/snmp/snmp_test.go index f7ac913a5..ed01508f2 100644 --- a/plugins/inputs/snmp/snmp_test.go +++ b/plugins/inputs/snmp/snmp_test.go @@ -648,6 +648,71 @@ func TestFieldConvert(t *testing.T) { } } +func TestSnmpTranslateCache_miss(t *testing.T) { + snmpTranslateCaches = nil + oid := "IF-MIB::ifPhysAddress.1" + mibName, oidNum, oidText, conversion, err := snmpTranslate(oid) + assert.Len(t, snmpTranslateCaches, 1) + stc := snmpTranslateCaches[oid] + require.NotNil(t, stc) + assert.Equal(t, mibName, stc.mibName) + assert.Equal(t, oidNum, stc.oidNum) + assert.Equal(t, oidText, stc.oidText) + assert.Equal(t, conversion, stc.conversion) + assert.Equal(t, err, stc.err) +} + +func TestSnmpTranslateCache_hit(t *testing.T) { + snmpTranslateCaches = map[string]snmpTranslateCache{ + "foo": snmpTranslateCache{ + mibName: "a", + oidNum: "b", + oidText: "c", + conversion: "d", + err: fmt.Errorf("e"), + }, + } + mibName, oidNum, oidText, conversion, err := snmpTranslate("foo") + assert.Equal(t, "a", mibName) + assert.Equal(t, "b", oidNum) + assert.Equal(t, "c", oidText) + assert.Equal(t, "d", conversion) + assert.Equal(t, fmt.Errorf("e"), err) + snmpTranslateCaches = nil +} + +func TestSnmpTableCache_miss(t *testing.T) { + snmpTableCaches = nil + oid := ".1.0.0.0" + mibName, oidNum, oidText, fields, err := snmpTable(oid) + assert.Len(t, snmpTableCaches, 1) + stc := snmpTableCaches[oid] + require.NotNil(t, stc) + assert.Equal(t, mibName, stc.mibName) + assert.Equal(t, oidNum, stc.oidNum) + assert.Equal(t, oidText, stc.oidText) + assert.Equal(t, fields, stc.fields) + assert.Equal(t, err, stc.err) +} + +func TestSnmpTableCache_hit(t *testing.T) { + snmpTableCaches = map[string]snmpTableCache{ + "foo": snmpTableCache{ + mibName: "a", + oidNum: "b", + oidText: "c", + fields: []Field{{Name: "d"}}, + err: fmt.Errorf("e"), + }, + } + mibName, oidNum, oidText, fields, err := snmpTable("foo") + assert.Equal(t, "a", mibName) + assert.Equal(t, "b", oidNum) + assert.Equal(t, "c", oidText) + assert.Equal(t, []Field{{Name: "d"}}, fields) + assert.Equal(t, fmt.Errorf("e"), err) +} + func TestError(t *testing.T) { e := fmt.Errorf("nested error") err := Errorf(e, "top error %d", 123)