fixed percentiles not being able to be ints

[MyaLongmire]

Required for all PRs:

• Updated associated README.md.
• Wrote appropriate unit tests.

resolves #9441

added a Number struct and an unmarshal that will allow for ints or floats

[sspaink]

Good work, overall it looks good but can you add another unit test that reads from a toml config with a int instead of a float to verify this change works? Something like this:

func TestConfig(t *testing.T) {
        // Load a sample toml config
	c := config.NewConfig()
	require.NoError(t, c.LoadConfig("./testdata/statsd_int.toml"))
        // Create a statsd input plugin struct and load it with expected values
	s := inputs.Inputs["statsd"]().(*Statsd)
	s.Percentiles = 90.0
	require.Equal(t, s, c.Inputs[0].Input)
}

[srebhan]

Hey @MyaLongmire, thanks for looking into this. IMO the problem here is an issue in the TOML parser as 90 is a perfect float64 value and should be parsed as such by the parser. So TBH I rather prefer to fix this function instead of circumventing the problem by doing the parsing manually.
What do you think?

[MyaLongmire]

Good work, overall it looks good but can you add another unit test that reads from a toml config with a int instead of a float to verify this change works? Something like this:

func TestConfig(t *testing.T) {
        // Load a sample toml config
	c := config.NewConfig()
	require.NoError(t, c.LoadConfig("./testdata/statsd_int.toml"))
        // Create a statsd input plugin struct and load it with expected values
	s := inputs.Inputs["statsd"]().(*Statsd)
	s.Percentiles = 90.0
	require.Equal(t, s, c.Inputs[0].Input)
}

I added a test that ensures ints can be added in the config, although I changed your idea so I wouldn't have to add a test data folder

[MyaLongmire]

Hey @MyaLongmire, thanks for looking into this. IMO the problem here is an issue in the TOML parser as 90 is a perfect float64 value and should be parsed as such by the parser. So TBH I rather prefer to fix this function instead of circumventing the problem by doing the parsing manually.
What do you think?

How would we know that 90 is supposed to be a float while parsing it? I feel like putting it in config copies how it used to be in internal.

[srebhan]

How would we know that 90 is supposed to be a float while parsing it?
The TOML parser has the struct it unmarshalls to. If you look at the function I linked, fv reflect.Value is the reference to the struct field the parser should unmarshal to. In the big switch fv.Kind() { statement below, the parser converts the known TOML Integer value to the known field type. It's only that it does not take "converting an int to a float" into account. We just would need to add

	case reflect.Float32, reflect.Float64:
		f := float64(i)
		if fv.OverflowFloat(f) {
			return &errorOutOfRange{fv.Kind(), f}
		}
		fv.SetFloat(f)

I feel like putting it in config copies how it used to be in internal.

Right, and you can do this as it restores the functionality. However, it is a workaround and the problem occurs for any float64 field exposed as a configuration option. So you would need to convert all of those fields to config.Number... :-S

[reimda]

@srebhan @MyaLongmire I don't think we want to make this change apply to all float64 fields in the config. TOML's design intentionally has a separate float and int type so making a toml int field initialize a telegraf float setting feels wrong. I think this change should be a very isolated workaround only for the statsd plugin. Statsd is special only for backward compatibility.

I'd prefer not to put Number in config/types.go because I don't want other plugins to do this in the future. What do you think about moving Number from config/ into plugins/inputs/statsd/ and putting in a comment saying it is for backward compatibility and asking people not to do the same thing elsewhere?

[srebhan]

@reimda: Fine with me, I just wanted to avoid spreading of Number again. :-)

In general I think using a TOML int as float is a valid use-case. Maybe the parser could have a strict_typing setting, but using 90 as a float is perfectly valid. Using 90.0 as int is not. :-)

test written

[telegraf-tiger]

Looks like new artifacts were built from this PR. Get them here!

Artifact URLs

• darwin_amd64.tar.gz

• windows_amd64.zip

• windows_i386.zip

• aarch64.rpm

• linux_arm64.tar.gz

• arm64.deb

• linux_mipsel.tar.gz

• mipsel.deb

• ppc64le.rpm

• linux_ppc64le.tar.gz

• ppc64el.deb

• x86_64.rpm

• freebsd_amd64.tar.gz

• linux_amd64.tar.gz

• amd64.deb

• i386.rpm

• freebsd_i386.tar.gz

• linux_i386.tar.gz

• i386.deb

• armv6hl.rpm

• freebsd_armv7.tar.gz

• linux_armhf.tar.gz

• armhf.deb

• static_linux_amd64.tar.gz

• linux_mips.tar.gz

• mips.deb

• s390x.rpm

• linux_s390x.tar.gz

• s390x.deb

• armel.rpm

• linux_armel.tar.gz

• armel.deb

[srebhan]

Perfect. Thanks @MyaLongmire for fixing this one!