Skip to content
This repository has been archived by the owner on Sep 21, 2023. It is now read-only.

Add custom marshal implementation for protobuf value type #17

Merged
merged 4 commits into from
Feb 15, 2023

Conversation

fearful-symmetry
Copy link
Contributor

Should fix elastic/elastic-agent-shipper#240

This adds a custom MarshalJSON() method to the various *value types used by the gRPC API, so we can properly marshal the event in a way elasticsearch will expect, and that should be equivalent to how how beats wound send events normally without the shipper in between.

I included a benchmark test in here as well, as right now it doesn't compare too favorably to marshalling from a hashmap:

cpu: Intel(R) Xeon(R) CPU E5-2630 0 @ 2.30GHz
BenchmarkCustomMarshal/marshal_custom_protobuf_message.Value_type-12               37108             43470 ns/op            2907 B/op         67 allocs/op
BenchmarkCustomMarshal/marshal_custom_protobuf_message.Value_type-12               44996             27932 ns/op            2907 B/op         67 allocs/op
BenchmarkCustomMarshal/marshal_custom_protobuf_message.Value_type-12               45432             39136 ns/op            2907 B/op         67 allocs/op
BenchmarkCustomMarshal/marshal_custom_protobuf_message.Value_type-12               33184             42999 ns/op            2907 B/op         67 allocs/op
BenchmarkCustomMarshal/marshal_custom_protobuf_message.Value_type-12               25257             42256 ns/op            2907 B/op         67 allocs/op
BenchmarkCustomMarshal/standard_struct_using_stdlib-12                             62853             22282 ns/op            2353 B/op         49 allocs/op
BenchmarkCustomMarshal/standard_struct_using_stdlib-12                             70322             19309 ns/op            2353 B/op         49 allocs/op
BenchmarkCustomMarshal/standard_struct_using_stdlib-12                             60699             17741 ns/op            2353 B/op         49 allocs/op
BenchmarkCustomMarshal/standard_struct_using_stdlib-12                             87601             14225 ns/op            2353 B/op         49 allocs/op
BenchmarkCustomMarshal/standard_struct_using_stdlib-12                             91309             15955 ns/op            2353 B/op         49 allocs/op
PASS

I've taken a few cracks at optimizing it, but the stdlib json Marshal() is so optimized that I haven't been able to make much progress.

@fearful-symmetry fearful-symmetry added the Team:Elastic-Agent Label for the Agent team label Feb 13, 2023
@fearful-symmetry fearful-symmetry requested a review from a team as a code owner February 13, 2023 22:28
@fearful-symmetry fearful-symmetry self-assigned this Feb 13, 2023
@fearful-symmetry fearful-symmetry requested review from belimawr and leehinman and removed request for a team February 13, 2023 22:28
@elasticmachine
Copy link

elasticmachine commented Feb 13, 2023

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2023-02-15T16:31:21.168+0000

  • Duration: 3 min 46 sec

Test stats 🧪

Test Results
Failed 0
Passed 18
Skipped 0
Total 18

🤖 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
Copy link
Member

cmacknz commented Feb 13, 2023

I included a benchmark test in here as well, as right now it doesn't compare too favorably to marshalling from a hashmap:

Is using https://pkg.go.dev/google.golang.org/protobuf/types/known/structpb#Struct.AsMap followed by the stdlib Marshal any better? Is this a deterministic in how it serializes?

@fearful-symmetry
Copy link
Contributor Author

@cmacknz performance is about the same, but it uses considerably more memory:

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
BenchmarkCustomMarshal/marshal_custom_protobuf_message.Value_type-12               30288             40504 ns/op            4802 B/op         74 allocs/op
BenchmarkCustomMarshal/marshal_custom_protobuf_message.Value_type-12               34208             41640 ns/op            4803 B/op         74 allocs/op
BenchmarkCustomMarshal/marshal_custom_protobuf_message.Value_type-12               27276             39862 ns/op            4802 B/op         74 allocs/op
BenchmarkCustomMarshal/marshal_custom_protobuf_message.Value_type-12               34820             41508 ns/op            4803 B/op         74 allocs/op
BenchmarkCustomMarshal/marshal_custom_protobuf_message.Value_type-12               27446             41209 ns/op            4803 B/op         74 allocs/op
BenchmarkCustomMarshal/standard_struct_using_stdlib-12                             62858             21460 ns/op            2353 B/op         49 allocs/op
BenchmarkCustomMarshal/standard_struct_using_stdlib-12                             50868             21334 ns/op            2353 B/op         49 allocs/op
BenchmarkCustomMarshal/standard_struct_using_stdlib-12                             51178             21987 ns/op            2353 B/op         49 allocs/op
BenchmarkCustomMarshal/standard_struct_using_stdlib-12                             50214             21949 ns/op            2353 B/op         49 allocs/op
BenchmarkCustomMarshal/standard_struct_using_stdlib-12                             67094             21645 ns/op            2353 B/op         49 allocs/op

Trying to bypass/reimplement the behavior of the stdlib Marshal() usually seemed to result in worse performance while I was testing, and it makes me wonder if there's just a certain amount of overhead involved in creating a custom MarshalJSON implementation.

@fearful-symmetry
Copy link
Contributor Author

The more I look over pprof and look at potential improvements, the more I think that any performance improvements might come with a stability tradeoff, as we'd essentially be re-implementing a ton of the stdlib json marshal backend, meaning we'd need to implement all the error handling and edge cases that come with it.

@cmacknz
Copy link
Member

cmacknz commented Feb 14, 2023

I learned today that APM Server implemented their own low level JSON serialization package to speed things up. https://github.com/elastic/go-fastjson

We could explore using that (or another json package to help this out). I don't know that we need to do all of the optimizations in this PR though.

@fearful-symmetry
Copy link
Contributor Author

Yah, agreed, it's easy to get carried away with optimizations like this, particularly before we've tested the entire shipper.

@fearful-symmetry
Copy link
Contributor Author

Alright, using the fastjson libraries gives us a slight improvement:

BenchmarkCustomMarshal/marshal_custom_protobuf_message.Value_type-12               60724             30176 ns/op            2689 B/op         32 allocs/op
BenchmarkCustomMarshal/marshal_custom_protobuf_message.Value_type-12               41823             29709 ns/op            2689 B/op         32 allocs/op
BenchmarkCustomMarshal/marshal_custom_protobuf_message.Value_type-12               36901             29857 ns/op            2689 B/op         32 allocs/op
BenchmarkCustomMarshal/marshal_custom_protobuf_message.Value_type-12               37262             29516 ns/op            2689 B/op         32 allocs/op
BenchmarkCustomMarshal/marshal_custom_protobuf_message.Value_type-12               38206             29550 ns/op            2689 B/op         32 allocs/op

@cmacknz cmacknz requested review from faec and removed request for belimawr February 14, 2023 21:59
pkg/helpers/struct_test.go Outdated Show resolved Hide resolved
// MarshalJSON implements the JSON interface for the value type
func (val *Value) MarshalJSON() ([]byte, error) {
// MarshalFastJSON implements the JSON interface for the value type
func (val *Value) MarshalFastJSON(w *fastjson.Writer) error {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't see test coverage for any of these functions, probably because they are in a separate package from the tests in helpers.

Copy link
Member

@cmacknz cmacknz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's get this integrated into the shipper so we can run a proper performance test.

Co-authored-by: Craig MacKenzie <craig.mackenzie@elastic.co>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Team:Elastic-Agent Label for the Agent team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ES output marshalling entire gRPC object
3 participants