Refactor NewValue, add tests

[fearful-symmetry]

Closes elastic/beats#34319

There's a few things going on here:

1. Refactor NewValue to make it a little more hardy, and to better handle the []string values that were causing the failures seen in the above issue
2. Add tests
3. update .go-version manually, since it was a tad out of date

[elasticmachine]

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

• Start Time: 2023-02-09T21:08:15.156+0000

• Duration: 3 min 50 sec

Test stats 🧪

Test	Results
Failed	0
Passed	17
Skipped	0
Total	17

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

• /test : Re-trigger the build.

[cmacknz]

Looks good, a couple of minor comments. I am curious about the impact of the use of reflection here since it is generally known to be expensive, and I think we could probably avoid it for common cases for primitive types.

We are going to execute this for every event processed by the shipper so the cost of this conversion will definitely show up in our performance tests.

[fearful-symmetry]

Alright, made a few performance changes, which seems to have helped:

# Before
goos: linux
goarch: amd64
pkg: github.com/elastic/elastic-agent-shipper-client/pkg/helpers
cpu: Intel(R) Xeon(R) CPU E5-2630 0 @ 2.30GHz
BenchmarkCustomUnmarshal-12        54525             26741 ns/op            4984 B/op        126 allocs/op
BenchmarkCustomUnmarshal-12        42554             24781 ns/op            4984 B/op        126 allocs/op
BenchmarkCustomUnmarshal-12        83989             24396 ns/op            4984 B/op        126 allocs/op
BenchmarkCustomUnmarshal-12        42492             26332 ns/op            4984 B/op        126 allocs/op
BenchmarkCustomUnmarshal-12        62872             27214 ns/op            4984 B/op        126 allocs/op
PASS
ok      github.com/elastic/elastic-agent-shipper-client/pkg/helpers     8.567s

# After
goos: linux
goarch: amd64
pkg: github.com/elastic/elastic-agent-shipper-client/pkg/helpers
cpu: Intel(R) Xeon(R) CPU E5-2630 0 @ 2.30GHz
BenchmarkCustomUnmarshal-12        57163             18618 ns/op            4392 B/op        111 allocs/op
BenchmarkCustomUnmarshal-12        57745             19610 ns/op            4392 B/op        111 allocs/op
BenchmarkCustomUnmarshal-12        75910             17909 ns/op            4392 B/op        111 allocs/op
BenchmarkCustomUnmarshal-12        55810             20084 ns/op            4392 B/op        111 allocs/op
BenchmarkCustomUnmarshal-12        56702             19406 ns/op            4392 B/op        111 allocs/op
PASS
ok      github.com/elastic/elastic-agent-shipper-client/pkg/helpers     6.823s

[cmacknz]

We aren't hitting a few cases in this test or the benchmarks looking at the code coverage with:

go test ./pkg/helpers/... -coverprofile=cover.tmp
go tool cover -html cover.tmp

We are missing the map[string]interface{} and reflect.Slice cases which both seem important enough to test. Hitting all the primitive number cases is probably easy to do but also unlikely to matter that much.

I am giving this PR extra scrutiny because any problems here mean the shipper just doesn't work at all, or works unnecessarily slowly :)

[fearful-symmetry]

Oh man, I completely forgot about the -coverprofile feature, haven't used that in years.

[fearful-symmetry]

Alright, going over the coverage profile made me realize that the mapstr.M type that's everywhere in beats was falling back to the reflection unpacking. Fixed, which resulted in another performance boost:

# before
goos: linux
goarch: amd64
pkg: github.com/elastic/elastic-agent-shipper-client/pkg/helpers
cpu: Intel(R) Xeon(R) CPU E5-2630 0 @ 2.30GHz
BenchmarkCustomUnmarshal-12        46443             31416 ns/op            6888 B/op        165 allocs/op
BenchmarkCustomUnmarshal-12        35974             32374 ns/op            6888 B/op        165 allocs/op
BenchmarkCustomUnmarshal-12        37224             29770 ns/op            6888 B/op        165 allocs/op
BenchmarkCustomUnmarshal-12        36456             31567 ns/op            6889 B/op        165 allocs/op
BenchmarkCustomUnmarshal-12        34078             32552 ns/op            6888 B/op        165 allocs/op
PASS
ok      github.com/elastic/elastic-agent-shipper-client/pkg/helpers     7.638s

# after
goos: linux
goarch: amd64
pkg: github.com/elastic/elastic-agent-shipper-client/pkg/helpers
cpu: Intel(R) Xeon(R) CPU E5-2630 0 @ 2.30GHz
BenchmarkCustomUnmarshal-12        59274             20546 ns/op            6041 B/op        124 allocs/op
BenchmarkCustomUnmarshal-12        97509             13711 ns/op            6040 B/op        124 allocs/op
BenchmarkCustomUnmarshal-12        61920             24602 ns/op            6040 B/op        124 allocs/op
BenchmarkCustomUnmarshal-12        52556             23920 ns/op            6040 B/op        124 allocs/op
BenchmarkCustomUnmarshal-12        55135             24024 ns/op            6040 B/op        124 allocs/op
PASS
ok      github.com/elastic/elastic-agent-shipper-client/pkg/helpers     7.664s

[cmacknz]

Thanks for all the iterations!

I suspect we may eventually want a way to know we are falling back on reflection, but in theory it will show up in the profiles in our performance tests.

[cmacknz]

I would tag a release for this fix before updating Beats + the shipper.