Add support for converting integers encoded as Hex-STRING

[Soldrym]

Bug report

Relevant telegraf.conf:

[[inputs.snmp]]
agents = ["10.41.16.16"]
community = "public"
interval = "60s"
[[inputs.snmp.table]]
name = "testing"
[[inputs.snmp.table.field]]
name = "connUnitPortStatIndex"
oid = "FCMGMT-MIB::connUnitPortStatIndex"
is_tag = true
[[inputs.snmp.table.field]]
oid = "FCMGMT-MIB::connUnitPortStatCountTxElements"
conversion = "int"

System info:

Server:
Telegraf v1.5.1 (git: release-1.5 0605af7)
Red Hat Enterprise Linux Server release 7.4 (Maipo)
Switches tested:
Brocade 7800, DCX8510 and DCX-4S
Fabric OS v7.4.1e and v8.0.2a

Steps to reproduce:

1. Run snmpwalk on the above OID
2. Compare results using Telegraf --test

Expected behavior:

snmpwalk shows Hex-STRING results. Telegraf would convert the hex-string to a decimal integer. Example:

Snmpwalk:

snmpwalk -v2c -cpublic 10.41.16.16 FCMGMT-MIB::connUnitPortStatCountTxElements
FCMGMT-MIB::connUnitPortStatCountTxElements.'....31.$........'.1 = Hex-STRING: 00 09 3E E3 F6 D5 3B 60
FCMGMT-MIB::connUnitPortStatCountTxElements.'....31.$........'.2 = Hex-STRING: 00 16 F7 B2 C1 07 C5 30

Telegraf (truncated to 2 ports):

telegraf --test --config telegraf.conf

• Plugin: inputs.snmp, Collection 1
• Internal: 1m0s

testing,connUnitPortStatIndex=1,agent_host=10.41.16.16,host=testhost connUnitPortStatCountTxElements=2602423610063712 1516743861000000000
testing,connUnitPortStatIndex=2,agent_host=10.41.16.16,host=testhost connUnitPortStatCountTxElements=6464796602385712 1516743861000000000

Actual behavior:

Telegraf converts the data to a '0i'

Snmpwalk:

snmpwalk -v2c -cpublic 10.41.16.16 FCMGMT-MIB::connUnitPortStatCountTxElements
FCMGMT-MIB::connUnitPortStatCountTxElements.'....31.$........'.1 = Hex-STRING: 00 09 3E E3 F6 D5 3B 60
FCMGMT-MIB::connUnitPortStatCountTxElements.'....31.$........'.2 = Hex-STRING: 00 16 F7 B2 C1 07 C5 30

Telegraf (truncated to 2 ports):

telegraf --test --config telegraf.conf

• Plugin: inputs.snmp, Collection 1
• Internal: 1m0s

testing,connUnitPortStatIndex=1,agent_host=10.41.16.16,host=testhost connUnitPortStatCountTxElements=0i 1516743861000000000
testing,connUnitPortStatIndex=2,agent_host=10.41.16.16,host=testhost connUnitPortStatCountTxElements=0i 1516743861000000000

Additional info:

Many of the port counters for FC equipment return data in the Hex-STRING format. These should be converted directly to decimal integers. In the case of connUnitPortStatCountTxElements, it would be a count of transmitted octets (bytes). In the example above, index 1 should have 2602423610063712 total octets transferred, but Telegraf converts it to '0i'. Converting to a float just results in a '0' value. Not converting it displays non printable characters such as:

testing,host=testhost,connUnitPortStatIndex=1,agent_host=10.41.16.16 connUnitPortStatCountTxElements=" ?
▒L▒" 1516745362000000000

[phemmer]

The issue here is that the value is not a string. It's a big-endian 64-bit integer which the device is claiming is a string, thus telegraf can't decode it properly.
Support could probably be added for this, but it would be a new feature. Changing the string-to-int decoding would break users that are currently using it.

[danielnelson]

@thechristschn I think this is also your issue we talked about a few weeks back.

[thechristschn]

Yes, I have exactly the same problem. I found a fix for me, but I'm sure this would change the behavior for other users.

index 7d3cb72..3b45d1c 100644
--- a/plugins/inputs/snmp/snmp.go
+++ b/plugins/inputs/snmp/snmp.go
@@ -11,6 +11,7 @@ import (
        "strings"
        "sync"
        "time"
+       "encoding/binary"
 
        "github.com/influxdata/telegraf"
        "github.com/influxdata/telegraf/internal"
@@ -770,7 +771,7 @@ func fieldConvert(conv string, v interface{}) (interface{}, error) {
                case uint64:
                        v = int64(vt)
                case []byte:
-                       v, _ = strconv.Atoi(string(vt))
+                       v = binary.BigEndian.Uint64(vt)
                case string:
                        v, _ = strconv.Atoi(vt)
                }```

[Soldrym]

I did something similar on Friday. I just added a 'hextoint' conversion by copying the blank conversion. I don't know Go, so I'm sure this could be cleaned up:

    if conv == "hextoint" {
        if bs, ok := v.([]byte); ok {
            qq := binary.BigEndian.Uint64(bs)
            return qq, nil
        }
        return v, nil
    }

[ghost]

Is this still something can be fixed with a new feature? I would prefer not to build from source as a work around if possible!

[phemmer]

Yes, this can be added as a new feature. But this feature would need to be implemented in a generic fashion if it's to be supported. There are lots of binary encoding types, and we'd be playing whack-a-mole addressing them one by one.

I'd recommend adding this as a new conversion format such as binary(littleEndian,int64). Exact syntax of the format specifier is up for debate. It'll need to support both endiannesses and all the different integer types (int8, uint8, int16, uint16, ...).

[wiardvanrij]

This is now included in the 1.17 release. I believe this can be closed.

[phemmer]

Little bit late, but my $0.02 on #8426: "hextoint" is not the correct name for the conversion. The data isn't in hex, it's in binary. "Hex" is an encoding format representing for numbers as strings using ASCII characters 0-9 A-F. That's not what this data being converted there is. The data is in raw binary, hence why there's no code to actually convert from a string in that PR.

If we were to ever actually add support for converting hex, we'd have to give it a different name, and it'd be extremely confusing to have hextoint which sounds like it does hex conversion, but doesn't, and something else that does.

[wiardvanrij]

Well, that's a fair point. lol.

To be honest, I don't "use" this, but I had colleagues who had this issue so I sort of just wrote the code. Based on this issue. So I haven't put that much thought into the whole 'what is this data', although I should have figured the hint when creating the test-cases.

So regardless of the naming, I believe this issue is resolved. Yet I would agree that a refactor should be in place. (perhaps a new issue).

[MyaLongmire]

snmp has gone through a major rewrite due to performance issues. This ticket also hasn't seen activity in a while so I am going to close it for now. If you are having issues with the updated snmp or would like a new feature added to it please open a new ticket with all relevant information.