Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: introduce code for JSON encoding protobuf types #42

Merged
merged 30 commits into from
Jun 8, 2023

Conversation

kruskall
Copy link
Member

@kruskall kruskall commented May 26, 2023

Introduce code for JSON encoding protobuf types

I tried to keep them close to the original apmevent toModelJSON but it should be fairly contained

Related to #36
Blocked by #41

@kruskall kruskall force-pushed the feat/protobuf-json branch from 0aa44f3 to 691ae24 Compare May 26, 2023 08:52
@kruskall kruskall force-pushed the feat/protobuf-json branch from 8206529 to bcefefd Compare May 29, 2023 08:35
@kruskall kruskall marked this pull request as ready for review May 29, 2023 08:35
@kruskall kruskall requested a review from a team May 29, 2023 08:37
Copy link
Member

@axw axw left a comment

Choose a reason for hiding this comment

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

Looks good! I think the CompressionStrategy conversion is wrong, so that's a blocker, but the other things I mentioned are debatable.

While you're here, would you mind changing the port fields in protobuf from uint32 to uint16?

@kruskall
Copy link
Member Author

kruskall commented Jun 1, 2023

While you're here, would you mind changing the port fields in protobuf from uint32 to uint16?

Could you clarify this ? Protobuf doesn't have a uint16 AFAIK

@axw
Copy link
Member

axw commented Jun 2, 2023

While you're here, would you mind changing the port fields in protobuf from uint32 to uint16?

Could you clarify this ? Protobuf doesn't have a uint16 AFAIK

Ah right, I had forgotten this. They will be varints, and the number of bytes will depend on the value. Nothing to do here then.

@kruskall kruskall requested a review from axw June 5, 2023 02:32
@kruskall
Copy link
Member Author

kruskall commented Jun 6, 2023

I've updated the code, here's the benchmarks:

Classic model (model -> modeljson):

BenchmarkAPMEventToJSON/to-json-20                553550              1953 ns/op            3688 B/op         53 allocs/op
BenchmarkAPMEventToJSON/to-json-20                558913              1875 ns/op            3688 B/op         53 allocs/op
BenchmarkAPMEventToJSON/to-json-20                564428              1898 ns/op            3688 B/op         53 allocs/op
BenchmarkAPMEventToJSON/to-json-20                598153              1932 ns/op            3688 B/op         53 allocs/op
BenchmarkAPMEventToJSON/to-json-20                626432              1963 ns/op            3688 B/op         53 allocs/op
BenchmarkAPMEventToJSON/to-json-20                597972              1942 ns/op            3688 B/op         53 allocs/op
BenchmarkAPMEventToJSON/to-json-20                632714              1967 ns/op            3688 B/op         53 allocs/op
BenchmarkAPMEventToJSON/to-json-20                504571              2004 ns/op            3688 B/op         53 allocs/op
BenchmarkAPMEventToJSON/to-json-20                566557              1898 ns/op            3688 B/op         53 allocs/op
BenchmarkAPMEventToJSON/to-json-20                619671              1960 ns/op            3688 B/op         53 allocs/op

Protobuf (modelpb -> modeljson):

BenchmarkAPMEventToJSON/to-json-20                453487              2596 ns/op            5096 B/op         52 allocs/op
BenchmarkAPMEventToJSON/to-json-20                457846              2675 ns/op            5096 B/op         52 allocs/op
BenchmarkAPMEventToJSON/to-json-20                430870              2643 ns/op            5096 B/op         52 allocs/op
BenchmarkAPMEventToJSON/to-json-20                431421              2605 ns/op            5096 B/op         52 allocs/op
BenchmarkAPMEventToJSON/to-json-20                490472              2658 ns/op            5096 B/op         52 allocs/op
BenchmarkAPMEventToJSON/to-json-20                474543              2533 ns/op            5096 B/op         52 allocs/op
BenchmarkAPMEventToJSON/to-json-20                473373              2783 ns/op            5096 B/op         52 allocs/op
BenchmarkAPMEventToJSON/to-json-20                428856              2656 ns/op            5096 B/op         52 allocs/op
BenchmarkAPMEventToJSON/to-json-20                463778              2621 ns/op            5096 B/op         52 allocs/op
BenchmarkAPMEventToJSON/to-json-20                418586              2568 ns/op            5096 B/op         52 allocs/op

Notes:

@kruskall kruskall force-pushed the feat/protobuf-json branch from b12e536 to f4abe77 Compare June 6, 2023 17:28
@kruskall
Copy link
Member Author

kruskall commented Jun 6, 2023

Pushed a few more fixes I found while integrating this with APM Server (no PR yet). This should be ready for review again.

Copy link
Member

@axw axw left a comment

Choose a reason for hiding this comment

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

Thank you!

Comment on lines +24 to +25
delete(in, k)
in[sanitizeLabelKey(k)] = v
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
delete(in, k)
in[sanitizeLabelKey(k)] = v
if k2 := sanitizeLabelKey(k); k2 != k {
delete(in, k)
in[k2] = v
}

We should expect most keys to not require sanitisation, and optimise for that.

@kruskall kruskall merged commit 64d161e into elastic:main Jun 8, 2023
@kruskall kruskall deleted the feat/protobuf-json branch June 8, 2023 06:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants