Run order events insertion in parallel #2131
Conversation
There was a problem hiding this comment.
I'm a bit torn on this. On the one hand moving stuff in the background makes the critical path faster. On the other hand we are about to merge the optimization that we only store events for market orders so we'd get rid of many inserts here.
One thing that is very interesting to me is that it looks like inserting events here can take from anywhere between 10ms and 1.5s for prod mainnet so even if we have only a few events to insert we might trigger some pathological behavior which we should move off of the critical path.
Edit: Only noticed after digging more through GH that you already wrote a comment somewhere else explaining your findings.
crates/autopilot/src/run_loop.rs
Outdated
| if let Err(err) = store_order_events_f.await { | ||
| tracing::warn!(auction_id=%id, ?err, "failed to store order events") | ||
| } |
There was a problem hiding this comment.
To keep the diff of the PR smaller and the code easier to read I'd suggest simply spawning the task and forgetting about it. This debugging data is not critical and having an error here is not very relevant. Also the future itself already handles it's own error so we'd only handle tokio runtime errors here which we can probably not do anything about anyway.
That way you can keep most of this code unchanged.
crates/autopilot/src/run_loop.rs
Outdated
| let store_order_events_f = tokio::spawn(async move { | ||
| let start = Instant::now(); | ||
| db.store_order_events(&events).await; | ||
| tracing::debug!(elapsed=?start.elapsed(), aution_id=%id, "storing order events took"); |
There was a problem hiding this comment.
One thing to note here is that when you spawn a new task you lose the tracing context of the task you spawned it from. To keep it around you have to do sth like:
tokio::task::spawn(future.instrument(tracing::Span::current()));| tracing::debug!(elapsed=?start.elapsed(), aution_id=%id, "storing order events took"); | |
| tracing::debug!(elapsed=?start.elapsed(), "stored order events"); |
From the comment, I understood that the proposed improvement for Market orders should be reconsidered and it doesn't make any sense to merge the current implementation. |
| async move { | ||
| let start = Instant::now(); | ||
| db.store_order_events(&events).await; | ||
| tracing::debug!(elapsed=?start.elapsed(), events_count=events.len(), "stored order events"); |
There was a problem hiding this comment.
Does tracing span include the auction id?
There was a problem hiding this comment.
I believe, it should:
services/crates/autopilot/src/run_loop.rs
Lines 72 to 74 in 07a966e
| .iter() | ||
| .map(|o| (o.metadata.uid, OrderEventLabel::Ready)) | ||
| .collect_vec(); | ||
| tokio::spawn( |
There was a problem hiding this comment.
nit: comment on why we spawn a new task here
Description
Executes order events db insert operation in parallel in order to propagate the auction among solvers as soon as possible. The same approach could be used in other parts of the application since this operation takes a while.
How to test
Logs observation
Related Issues
Relates to #2095