-
Notifications
You must be signed in to change notification settings - Fork 7
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
Analytics crate + downtime metrics between trades for a given time period #1021
base: main
Are you sure you want to change the base?
Conversation
use rain_orderbook_subgraph_client::{OrderbookSubgraphClient, OrderbookSubgraphClientError}; | ||
|
||
#[async_trait] | ||
pub trait OrderbookSubgraphClientTrait { |
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.
I'm creating this for two reasons:
- To make the code more testable
- To not pollute the
OrderbookSubgraphClient
with analytics-related methods
} | ||
|
||
fn create_mock_trade(timestamp: u64) -> Trade { | ||
Trade { |
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.
Note: IMO this should had been provided as a mock interface from the subgraph crate.
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.
makes sense - can you move it over there?
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.
I can, but this particular block is very ugly and opinionated for this set of unit tests. Also out of scope of this PR i'd say.
What I's thinking was some sort of a testing crate that creates fake trades & orders for a given scenario.
let orders = self.orders_list_all().await?; | ||
|
||
let mut all_trades = Vec::new(); | ||
for order in orders { |
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.
I don't like the performance output of this, but to fix that, would had been out of the scope of the PR.
all_trades.extend(trades); | ||
} | ||
|
||
all_trades.sort_by(|a, b| { |
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.
I'm sorting the trades, as it's not guaranteed that the orders will give me chronologically sorted trades.
change in the original API. Added example:
using the let (avg, min, max, count, total) = analytics.calculate_downtime_between_trades(period, threshold_secs).await;
println!("Average downtime: {:.2} seconds", avg);
println!("Minimum downtime: {:.2} seconds", min);
println!("Maximum downtime: {:.2} seconds", max);
println!("Number of occurrences: {}", count);
println!("Total downtime: {:.2} seconds", total); |
Motivation
We wanted to understand the "downtime" between trades as a critical KPI for evaluating the performance and responsiveness.
Currently we're lacking a crate focusing on the analytics part for the future metrics we want to expand on, limiting visibility into creating KPIs.
Solution
This proposed change introduces the downtime metric (min, max & avg) for the time intervals between consecutive trades, irrespective of the orders (gathers all).
Example:
and at the cli:
The current implementation relies in gathering all orders and then sorting the trades in order to calculate the needed time deltas. A potential enhancement is to go directly on chain and get the transfers, as we rely on subgraph crate for now, which gives us the orders.
Checks
By submitting this for review, I'm confirming I've done the following: