Add custom marshal implementation for protobuf value type#17
Add custom marshal implementation for protobuf value type#17fearful-symmetry merged 4 commits intoelastic:mainfrom
Conversation
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
🤖 GitHub commentsExpand to view the GitHub comments
To re-run your PR in the CI, just comment with:
|
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? |
|
@cmacknz performance is about the same, but it uses considerably more memory: Trying to bypass/reimplement the behavior of the stdlib |
|
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. |
|
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. |
|
Yah, agreed, it's easy to get carried away with optimizations like this, particularly before we've tested the entire shipper. |
|
Alright, using the fastjson libraries gives us a slight improvement: |
| // 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 { |
There was a problem hiding this comment.
I can't see test coverage for any of these functions, probably because they are in a separate package from the tests in helpers.
cmacknz
left a comment
There was a problem hiding this comment.
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>
Should fix elastic/elastic-agent-shipper#240
This adds a custom
MarshalJSON()method to the various*valuetypes 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:
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.