fix: trim whitespace on ethtool input plugin

[powersj]

The VMware vmxnet3 driver has hard-coded whitespace in the statistics descriptions. This is done to help make the ethtool output look "nice". However, this is causing lots of weird output and escaped characters in Telegraf output. This PR will remove any leading and trailing whitespace from these description keys.

Resolves: #9902

[powersj]

@reimda would you want this to be a flag in order to not change things for users who are ignoring this? or feel it is a proper fix?

[reimda]

would you want this to be a flag in order to not change things for users who are ignoring this? or feel it is a proper fix?

Since it's a workaround for a single unusual network device driver I don't think it should need a flag. Metrics for these unusual devices seem pretty much unusable so I expect everyone would want the fix.

Usually tag and field names have underscores between words and not spaces. (see https://github.com/influxdata/telegraf/blob/master/docs/developers/REVIEWS.md#metric-schema and https://docs.influxdata.com/influxdb/v1.8/concepts/schema_and_data_layout/#encouraged-schema-design) Maybe we should replace the spaces between words with underscores for these fields too.

[powersj]

Hmm yes replacing spaces with underscores does seem to be a good idea here. Are you good with that being part of the bug fix as well? or something separate with a flag?

[reimda]

I don't know if other ethernet devices have spaces. If it was just vmxnet3 I'd say we don't need a flag for underscores, but we would need to be confident it wasn't going to affect other devices.

From a quick look at linux kernel source, here's where vmware added the whitespace and didn't use underscore:
https://elixir.bootlin.com/linux/latest/source/drivers/net/vmxnet3/vmxnet3_ethtool.c#L42

Compare with a typical driver:
https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/intel/e1000/e1000_ethtool.c#L26

There are 160 or so drivers that return ethtool info. I spot checked 10 of them and found two with spaces. That's enough to show a significant portion of users would be affected if this plugin started switching spaces to underscores.

We definitely need a flag to switch spaces to underscores. I'm changing my mind on trimming whitespace too and think that should be a flag. Then there's no chance existing users will be affected by the change, but users who want cleaner names have a way to get them.

I did also run into one driver that uses camel case. It would be nice if we handled that too.

Here are the drivers I checked if you're interested. Maybe we should copy some of these into unit tests.

underscores: 7
https://elixir.bootlin.com/linux/latest/source/drivers/net/xen-netfront.c#L2354
https://elixir.bootlin.com/linux/latest/source/drivers/net/hyperv/netvsc_drv.c#L1475
https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c#L42
https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/mellanox/mlxsw/spectrum_ethtool.c#L263
https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/marvell/mv643xx_eth.c#L1447
https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/brocade/bna/bnad_ethtool.c#L1075
https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/altera/altera_tse_ethtool.c#L231

spaces: 1
https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch-ethtool.c#L17

mixed underscore and spaces: 1
https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/qlogic/qlcnic/qlcnic_ethtool.c#L28

CamelCase: 1
https://elixir.bootlin.com/linux/latest/source/drivers/net/dsa/lantiq_gswip.c#L301

[reimda]

I'd prefer not to add multiple bool flags to select which field name normalizations should happen. trim_whitespace = true, replace_spaces = true, translate_uppercase = true. If we do it this way the configuration section for a plugin is many lines and future changes just make it longer.

If we are sure we know about all possible normalizations, we could have one setting normalize_field_names = true that does everything. The problem there is if we miss something, in a future release we would have to add another setting normalize_field_names_more = true which is ugly.

We should probably have a setting "workarounds" or "normalize_field_names" that's an array of strings, then users can choose which ones they want to apply: "trim", "underscore", "uppercase". Then if there's something else in the future, we just add another string. normalize_field_names = ["trim", "underscore"]

[powersj]

Thanks for looking into this! How about something like:

  ## Some drivers declare statistics with extra whitespace, different spacing,
  ## and mix cases. This list, when enabled, can be used to clean the keys.
  ## Here are the current possible normalizations:
  ##  * trim: removes leading and trailing whitespace
  ##  * lower: changes all capitalized letters to lowercase
  ##  * spaces: replaces spaces with underscores
  # normalize_key_names = ["trim", "lower", "spaces"]

[Hipska]

Do you have an example of before/after output? That gives some context to reviewers.

[powersj]

In #9902 I have one example that we saw based on the VMware vmxnet3 driver, here part of it:

dns_ethtool,driver=vmxnet3,host=foobar,interface=enp0s3 \ \ mcast\ pkts\ rx=14i,\ \ \ \ \ tso=0i,\ \ LRO\ byte\ rx=0i,Rx\ Queue#=0i,\ \ TSO\ pkts\ tx=10i,\ \ bcast\ pkts\ tx=3i,\ \ pkts\ linearized=0i,

There are numerous stats with leading whitespace and then whitespace between words in the stats. As @reimda pointed out, there are best practices for naming tags and fields, which involve not having leading/trialing whitespace or whitespace between words. When he looked at other drivers showed some hard-code camel case stat names as well.

If you look at the tests you can see some examples of how this would work. Take the following example

Original stat:        "  RX Sent "
Spaces normalization: "__RX_Sent_"
Lower normalization:  "  rx sent "
Trim normalization:   "RX Sent"
All three:            "rx_sent"

[reimda]

You added lower case which is nice, but we probably should handle camel case too since there are drivers that use it RxBytesReceived rx_bytes_received

[Hipska]

The Init method should check if the given values are valid..

[powersj]

@reimda ready for re-review

[reimda]

Other than the doc nitpick this looks good to me.

[telegraf-tiger]

📦 Looks like new artifacts were built from this PR.

Expand this list to get them here! 🐯

Artifact URLs

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

fixed naming items