Add support to convert snmp hex strings to integers

[wiardvanrij]

Required for all PRs:

• Signed CLA.
• Associated README.md updated.
• Has appropriate unit tests.

Relates to #3716

[srebhan]

Looks good so far. One minor thing:
In the comment above the declaration of function fieldConvert() the possible formats are described. It would be nice to either add the new option there or remove the documentation altogether...

[wiardvanrij]

Thanks, I've removed the comments as I think the code speaks for itself and the readme should be sufficient.

[wiardvanrij]

@srebhan - Just letting you know I've build this myself for a team at our company. They have tested this in production today and it's working as intended. I hope we can have this released soon. Thanks!

[phemmer]

Copying my comment from over on #3716 (comment) for visibility:

"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.

[srebhan]

Well reading the code this is exactly what is done here. The data-to-convert is a string containing a HEX representation... What is your critique concretely? In my interpretation hextoint is a conversion of a string e.g. 0xFF2A to the integer datatype in this case 65322. What is your interpretation?

[phemmer]

Well reading the code this is exactly what is done here. The data-to-convert is a string containing a HEX representation

I'm sorry, but you must be misreading the code. binary.LittleEndian.Uint64 and all the neighboring functions convert raw binary, not hex strings.

[srebhan]

Very true I completely missed that. Sorry! So basically we should rename this to bytetoint right?
@ssoroka and @reimda should we add a PR to rename the option before a release!?!???!?

[phemmer]

Very true I completely missed that. Sorry! So basically we should rename this to bytetoint right?

bytetoint seems reasonable. The name still nags at me a little bit, but I can't think of anything better. I think it would also have made sense as a parameter to the int converter, as that converter can already accept multiple input types and outputs to int. But a separate converter works fine too.

[wiardvanrij]

Very true I completely missed that. Sorry! So basically we should rename this to bytetoint right?
@ssoroka and @reimda should we add a PR to rename the option before a release!?!???!?

This is already released: https://docs.influxdata.com/telegraf/v1.17/about_the_project/release-notes-changelog/

bytetoint seems reasonable. The name still nags at me a little bit, but I can't think of anything better. I think it would also have made sense as a parameter to the int converter, as that converter can already accept multiple input types and outputs to int. But a separate converter works fine too.

That's the great thing about OSS, pull-requests can be made.

Currently, it would be breaking to change it again. My suggestion is to just close the 3-year-old issue that had no priority and call it a day. With perhaps some lessons learned.

[srebhan]

@wiardvanrij yeah the really bad thing is that we now "never-again" can reuse the "hextoint" name as it would change the semantics of an existing option. :-( Sorry for having missed it in the first place.

[phemmer]

My bigger concern is that when someone comes looking for functionality to convert from binary (byte encoding) to native integer, they're not going to see this "hextoint" and realize it does what they want.

While I think it would be good to fix the name, and just leave "hextoint" as an undocumented alias, at the minimum we should fix the documentation.