Refactor NewValue, add tests by fearful-symmetry · Pull Request #16 · elastic/elastic-agent-shipper-client

fearful-symmetry · 2023-01-31T20:26:52Z

Closes elastic/beats#34319

There's a few things going on here:

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
Add tests
update .go-version manually, since it was a tad out of date

elasticmachine · 2023-01-31T20:29:31Z

💚 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 · 2023-02-08T22:38:02Z

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 · 2023-02-09T19:29:26Z

+	}
+}
+
+func TestStructValue(t *testing.T) {


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 :)

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

fearful-symmetry · 2023-02-09T21:09:23Z

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 · 2023-02-10T18:33:09Z

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

refactor NewValue, add tests

75a6a7d

fearful-symmetry added bug Something isn't working Team:Elastic-Agent Label for the Agent team labels Jan 31, 2023

fearful-symmetry requested a review from a team as a code owner January 31, 2023 20:26

fearful-symmetry self-assigned this Jan 31, 2023

fearful-symmetry requested review from belimawr and leehinman and removed request for a team January 31, 2023 20:26

update deps

388e2d5

cmacknz reviewed Jan 31, 2023

View reviewed changes

Comment thread pkg/helpers/struct.go Outdated

fearful-symmetry added 2 commits January 31, 2023 13:35

try to make CI happy

cc713f5

still fighting with CI

fb89a50

cmacknz requested a review from faec February 1, 2023 16:52

cmacknz reviewed Feb 1, 2023

View reviewed changes

Comment thread pkg/helpers/struct.go Outdated

add fallback for structs, benchmark

d5f7661

fearful-symmetry requested a review from cmacknz February 7, 2023 17:32

cmacknz reviewed Feb 8, 2023

View reviewed changes

Comment thread pkg/helpers/struct.go Outdated

Comment thread pkg/helpers/struct.go Outdated

Comment thread pkg/helpers/struct_test.go Outdated

Comment thread pkg/helpers/struct_test.go

optimize, expand tests

1d6aeaa

fearful-symmetry added 2 commits February 8, 2023 14:40

expand comments

dd490ca

update deps

621b0b5

fearful-symmetry requested a review from cmacknz February 9, 2023 18:19

cmacknz reviewed Feb 9, 2023

View reviewed changes

Comment thread pkg/helpers/struct.go Outdated

cmacknz reviewed Feb 9, 2023

View reviewed changes

Comment thread pkg/helpers/struct.go Outdated

cmacknz reviewed Feb 9, 2023

View reviewed changes

Comment thread pkg/helpers/struct.go Outdated

cmacknz reviewed Feb 9, 2023

View reviewed changes

Comment thread pkg/helpers/struct_test.go Outdated

cmacknz reviewed Feb 9, 2023

View reviewed changes

improve performance for mapstr types, expand tests and benchmarks

f762830

fearful-symmetry requested a review from cmacknz February 9, 2023 21:28

cmacknz approved these changes Feb 10, 2023

View reviewed changes

fearful-symmetry merged commit 7610e18 into elastic:main Feb 10, 2023

Conversation

fearful-symmetry commented Jan 31, 2023

Uh oh!

elasticmachine commented Jan 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💚 Build Succeeded

Build stats

Test stats 🧪

🤖 GitHub comments

Uh oh!

Uh oh!

Uh oh!

cmacknz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fearful-symmetry commented Feb 8, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cmacknz Feb 9, 2023

Choose a reason for hiding this comment

Uh oh!

fearful-symmetry Feb 9, 2023

Choose a reason for hiding this comment

Uh oh!

fearful-symmetry commented Feb 9, 2023

Uh oh!

cmacknz left a comment

Choose a reason for hiding this comment

Uh oh!

cmacknz commented Feb 10, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

elasticmachine commented Jan 31, 2023 •

edited

Loading