update gosnmp and add support for opaque float/double #268
Conversation
updated vendored version of gosnmp to latest, which now supports returning values from opaque float and double updated generator to recognize opaque float/double and configure it to work as a gauge updated collector to extract these values correctly
generator/tree.go
Outdated
| // Promote Opaque Float/Double textual convention to type | ||
| walkNode(nodes, func(n *Node) { | ||
| if n.Type == "OPAQUE" { | ||
| if strings.EqualFold(n.TextualConvention, "float") { |
There was a problem hiding this comment.
We do case-sensitive comparisons everywhere else
generator/tree.go
Outdated
| walkNode(nodes, func(n *Node) { | ||
| if n.Type == "OPAQUE" { | ||
| if strings.EqualFold(n.TextualConvention, "float") { | ||
| n.Type = "FLOAT" |
There was a problem hiding this comment.
Float, as that's what's in the MIB
generator/tree_test.go
Outdated
| @@ -782,6 +791,42 @@ func TestGenerateConfigModule(t *testing.T) { | |||
| }, | |||
| }, | |||
| }, | |||
| // Opaque Float becomes gauge | |||
There was a problem hiding this comment.
These go up with the metric type case
| @@ -172,6 +172,10 @@ func getPduValue(pdu *gosnmp.SnmpPDU) float64 { | |||
| switch pdu.Type { | |||
There was a problem hiding this comment.
pduValueAsString also needs updating.
|
FORMAT.md will also need updating. |
|
What is your preferred way for subsequent changes, additional commits to this branch? Or force push amended commit? |
|
Additional commits tend to be easier to review. |
|
Can you clarify how FORMAT.md is updated/used? The text at the top indicates that it is automatically generated, but I don't see a description of how that is done. |
|
FORMAT.md is the documentation for what snmp.yml is. You're adding new types, so you need to add them in there by hand. |
|
Thanks, I'm still a bit confused, as this change results in Float and Double being generated as a gauge in the resulting snmp.yml. My understanding was that at the prometheus level everything had to turn into gauge or counter anyway. But I do see other types documented in this file, so maybe my understanding and part of this PR is wrong. |
|
I think maybe I get it now, perhaps instead of turning Float and Double into a gauge, they should stay as their own type in the snmp.yml. Then that additional type information might let you format or otherwise handle it a bit differently inside the snmp_exporter. Then at the level of prometheus scraping it, they all become gauages or counters anyway. Does this sound correct? |
|
My question would be how does gosnmp know that a value is a float? That information is only available from the MIB afaik, which gosnmp does not have access to. |
|
What I learned when I hacked the gosnmp portion was that the opaque type values are not just arbitrary bytes, but actually double-wrapped ASN.1 values. The outer wrapper has type opaque (0x44) and the inner value has types 0x78 (float) or 0x78 (double). It appears to me that opaque was included initially for extensibility, but it was under-specified and became confusing. Later, they deprecated it, but there are a few types like Float, Double and possibly a few others that are still used.
Note, both of those are expired drafts, so it's possible this is not formal, but just a convention people happen to use (but following this did allow me to successfully parse the data I found in the real world). As for your original question, both the MIB textual convention and type inside wrapped opaque value give us indication the value should be interpreted as float/double. |
|
To be consistent let's work off the textualconvention like we do elsewhere then. This will avoid being over-eager in detecting such values. |
- renamed types Float, Double (consistent with MIB) - use case-sensitive comparison with textual convention - reorganized tests to fit into existing tests - added implementation of pduValueAsString - changed behavior to include Float/Double as types in generated snmp.yml - documented these types in FORMAT.md - added implementation of pduToSamples to render these new types as gauge
|
Thanks for helping work through this, I'm still not exactly clear how you'd like it to proceed. This PR does have the generator detect Float/Double based on the textual convention in the MIB. I've changed it now to keep Float/Double as types in that snmp.yml. I've documented these types in FORMAT.md. However, I then also had to add some code to properly render these as a guage when scraped by prometheus. I've also tried to incorporate your other feedback. Hopefully this gets us a little closer. |
generator/tree.go
Outdated
| @@ -95,6 +95,15 @@ func prepareTree(nodes *Node) map[string]*Node { | |||
| } | |||
| }) | |||
|
|
|||
| // Promote Opaque Float/Double textual convention to type | |||
There was a problem hiding this comment.
Comments should end in a full stop.
generator/tree.go
Outdated
| @@ -95,6 +95,15 @@ func prepareTree(nodes *Node) map[string]*Node { | |||
| } | |||
| }) | |||
|
|
|||
| // Promote Opaque Float/Double textual convention to type | |||
| walkNode(nodes, func(n *Node) { | |||
| if n.Type == "OPAQUE" { | |||
There was a problem hiding this comment.
I'd expect that checking the textual convention is enough
generator/tree.go
Outdated
| @@ -113,6 +122,10 @@ func metricType(t string) (string, bool) { | |||
| return "InetAddress", true | |||
| case "PhysAddress48", "DisplayString": | |||
| return t, true | |||
| case "Float": | |||
There was a problem hiding this comment.
You can combine this into the previous case
- comment ends in full stop - simplified conditional logic - consolidated switch cases
|
Pushed commit addressing most recent review comments. |
|
Thanks! |
updated vendored version of gosnmp to latest, which now supports
returning values from opaque float and double
updated generator to recognize opaque float/double and configure
it to work as a gauge
updated collector to extract these values correctly
@brian-brazil