-
Notifications
You must be signed in to change notification settings - Fork 526
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
Introduce efficient codec for model events #4120
Comments
I spent a bit of time looking into this, with master...axw:model-protobuf being the fruits of my labour. I wrote a generator which parses the model types and generates protobuf definitions, and then used plain old golang/protobuf to generate the code. I was going to use gogo/protobuf but had some probably-PEBKAC issues and ended up abandoned that for this short spike. For generating protobuf correctly we would probably need to check them in and take the old definitions into account, so we don't accidentally remove fields and break compatibility with previous releases. Another option would be to have hand-written protobuf as the source of truth for our model types and generate the model types instead, using standard protoc tooling. That would be more typical and almost certainly less error prone. It might be painful to migrate though. While testing I also realised that the model types we have do not have any encoding/json tags, and so we end up encoding all the empty fields. I added some I compared this with the existing JSON codec, but bear in mind that the benchmarks are far from comprehensive. They currently only encode model.Transactions with the TraceID and ID fields set, and nothing else. With a more comprehensive set of fields, we might observe a different outcome. Nevertheless, here's what I found.
Overall the protobuf implementation is faster for both reading and writing, and encodes as fewer bytes (see the MB/s metrics). The protobuf implementation produces fewer allocations, but allocates more memory overall. I imagine this is due to the intermediate protobuf-generated types; we might be able to optimise here, I haven't looked into it. I think this is worth pursuing, but we should probably do some real-world performance testing before embarking on an epic journey. |
@axw can you update the issue with open pre-conditions? |
@simitt I've added some detail to the description. I think this is unblocked now. |
See also elastic/apm-data#36 |
I think we covered everything on the list. Closing this. |
Introduce an efficient binary codec for model events (at least transactions and spans), for persisting to local storage. This would be used in tail-based sampling, and perhaps in other processing such as #5936.
For the most efficient codec, we should use a statically defined encoding. Barring arguments made for alternatives, let's use protobuf.
In order to have a statically defined encoding we'll need our model to be stable. The events we produce are now substantially ECS-compliant (which is stable), with a few exceptions. For example, we emit
transaction.duration.us
orspan.duration.us
; ideally we would record only the ECSevent.duration
field (#5999).I suggest we split this up into two phases:
Phase 1
Phase 2
For phase 2 we'll need some more changes to remove any remaining logic in the model package, e.g. setting
<event>.duration.us
. This can be done in an ingest pipeline.The text was updated successfully, but these errors were encountered: