-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
fix(sinks): spike out encoding events before batching #18028
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Luke Steensen <[email protected]>
✅ Deploy Preview for vrl-playground ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for vector-project canceled.
|
Datadog ReportBranch report: ✅ |
Signed-off-by: Luke Steensen <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't do any nitpicking here as this is a spike draft. That run_inner fn seems like it could be nice to break up, perhaps around the concurrent_map.
It looks like this more or less accomplishes the goal.
An interesting alternative would be to precompute only what is needed downstream (e.g. partition keys, keys for tagging metrics, other semantically meaningful fields for request building) and pass that along instead of the full original event.
That does seem like it could be a nice adjustment.
} | ||
payload.extend_from_slice(combined_encoder.batch_suffix()); | ||
|
||
let request_metadata = RequestMetadata::new( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like a fringe benefit of this would be that we don't need the RequestMetadataBuilder
anymore ? I added that last year and it was kind of annoying, like if we have all the information we need in one place there are definite perks to that.
DO NOT MERGE: This is still missing at least a few important features.
The problem
Vector currently does batching by using the in-memory size of events (via the
ByteSizeOf
trait), regardless of the encoding that the sink will actually use to write out those events. This is potentially useful if you care about managing Vector's memory use (though somewhat indirect), but surprisingly inaccurate if you're trying to affect the size of the sink's actual output.For example, a simple config of
demo_logs
withjson
formatting piped into theaws_s3
sink will result in 4MB objects (uncompressed) despite a batch size setting of 10MB. Metrics can have a large swing in the other direction due to struct field names not taking up any space in memory, but being a meaningful part of a JSON payload, for example. In general, the issue is heavily data-dependent, but there is very significant impact in many straightforward use cases.How it works today
The flow of the
aws_s3
and many similar sinks is roughly as follows:PartitionedBatcher
turns the input stream ofEvent
s into a stream of(Key, Vec<Event>)
, using the provided batch settings andByteSizeOf
to calculate the size of each event.RequestBuilder
trait:The encoding stage in step 2.2 is somewhat complex, and we actually serialize each event into a new byte buffer before actually writing it to the final payload. This appears to be done primarily so that we can track the actual number of bytes being written for instrumentation purposes.
This PR
In this spike we experiment with pulling parts of the encoding stage up in the pipeline, before the partitioner, so that we can use the actual correct encoded size to build batches accurately (i.e. relative to what we will output, not the in-memory size). This doesn't fit into the current trait structure (e.g.
RequestBuilder
), so there's some messiness in the implementation that we could clean up if we decide this is a viable path forward.To do this, we take a bit of a shortcut in that we keep both the original event and the encoded version together (the new
EncodedEvent
) and pass them along the pipeline for a while. This is probably not optimal from a memory-use perspective, but limited the amount of rework that needed to be done to prove the concept. An interesting alternative would be to precompute only what is needed downstream (e.g. partition keys, keys for tagging metrics, other semantically meaningful fields for request building) and pass that along instead of the full original event.The main thing missing from the current implementation is compression, due to some very thorny compiler errors that popped up when I tried to add it. These issues were likely due to the relatively complex state of the spike (lots of nested async and chaining), and shouldn't prove to be a problem if we choose to move forward and properly factor the code.
Open questions
IncrementalRequestBuilder
? It seems designed to be able to split requests that are too large, which could be enough to solve for hard API failures like Datadog Logs, but does not seem like it'd help at all with use cases like S3 objects being too small. If we do make a larger change, should it replace this version as well?