-
Notifications
You must be signed in to change notification settings - Fork 182
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: add tps metric to contracts table #2468
Changes from 13 commits
e5bd050
07ecb40
6a6fec9
716631a
e35d72a
244c277
d938bab
de3f985
b12107d
9eb9b2a
1d6ae93
caf38b2
938eabf
9f57837
4be7b4d
10c561d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -14,8 +14,8 @@ use tracing::{debug, error}; | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
use crate::simple_broker::SimpleBroker; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
use crate::types::{ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Entity as EntityUpdated, Event as EventEmitted, EventMessage as EventMessageUpdated, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Model as ModelRegistered, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Contract as ContractUpdated, Entity as EntityUpdated, Event as EventEmitted, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
EventMessage as EventMessageUpdated, Model as ModelRegistered, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
pub(crate) const LOG_TARGET: &str = "torii_core::executor"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -31,6 +31,7 @@ pub enum Argument { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
#[derive(Debug, Clone)] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
pub enum BrokerMessage { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
SetHead(ContractUpdated), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
ModelRegistered(ModelRegistered), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
EntityUpdated(EntityUpdated), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
EventMessageUpdated(EventMessageUpdated), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -45,8 +46,17 @@ pub struct DeleteEntityQuery { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
pub ty: Ty, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
#[derive(Debug, Clone)] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
pub struct SetHeadQuery { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
pub head: u64, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
pub last_block_timestamp: u64, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
pub txns_count: u64, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
pub contract_address: Felt, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
#[derive(Debug, Clone)] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
pub enum QueryType { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
SetHead(SetHeadQuery), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
SetEntity(Ty), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
DeleteEntity(DeleteEntityQuery), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
EventMessage(Ty), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -178,6 +188,35 @@ impl<'c> Executor<'c> { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
let tx = &mut self.transaction; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
match query_type { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
QueryType::SetHead(set_head) => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
let previous_block_timestamp: u64 = sqlx::query_scalar::<_, i64>( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
"SELECT last_block_timestamp FROM contracts WHERE id = ?", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
.bind(format!("{:#x}", set_head.contract_address)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
.fetch_one(&mut **tx) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
.await? | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
.try_into() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
.map_err(|_| anyhow::anyhow!("Last block timestamp doesn't fit in u64"))?; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
let tps: u64 = if set_head.last_block_timestamp - previous_block_timestamp != 0 { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
set_head.txns_count / (set_head.last_block_timestamp - previous_block_timestamp) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} else { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
set_head.txns_count | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+191
to
+205
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ohayo! Potential underflow and division by zero in TPS calculation Sensei, in the calculation of Apply this diff to prevent underflow: +let time_diff = set_head.last_block_timestamp.checked_sub(previous_block_timestamp).unwrap_or(0);
+let tps: u64 = if time_diff != 0 {
+ set_head.txns_count / time_diff
+} else {
+ set_head.txns_count
+}; 📝 Committable suggestion
Suggested change
Comment on lines
+201
to
+205
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Ohayo! Let's make that TPS calculation more robust, sensei! The current TPS calculation might not handle edge cases well. Consider using a more precise calculation method that avoids potential issues with integer division. Here's a suggestion: let time_diff = set_head.last_block_timestamp.saturating_sub(previous_block_timestamp);
let tps = if time_diff > 0 {
(set_head.txns_count as f64 / time_diff as f64).round() as u64
} else {
0 // or another appropriate default value
}; This approach uses floating-point division for more precise results and handles the case where |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
query.execute(&mut **tx).await.with_context(|| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
format!("Failed to execute query: {:?}, args: {:?}", statement, arguments) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
})?; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
let row = sqlx::query("UPDATE contracts SET tps = ? WHERE id = ? RETURNING *") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
.bind(tps as i64) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
.bind(format!("{:#x}", set_head.contract_address)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
.fetch_one(&mut **tx) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
.await?; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
let contract = ContractUpdated::from_row(&row)?; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
self.publish_queue.push(BrokerMessage::SetHead(contract)); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
QueryType::SetEntity(entity) => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
let row = query.fetch_one(&mut **tx).await.with_context(|| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
format!("Failed to execute query: {:?}, args: {:?}", statement, arguments) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -289,6 +328,7 @@ impl<'c> Executor<'c> { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
fn send_broker_message(message: BrokerMessage) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
match message { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
BrokerMessage::SetHead(update) => SimpleBroker::publish(update), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
BrokerMessage::ModelRegistered(model) => SimpleBroker::publish(model), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
BrokerMessage::EntityUpdated(entity) => SimpleBroker::publish(entity), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
BrokerMessage::EventMessageUpdated(event) => SimpleBroker::publish(event), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,7 +15,7 @@ use starknet_crypto::poseidon_hash_many; | |
use tokio::sync::mpsc::UnboundedSender; | ||
|
||
use crate::cache::{Model, ModelCache}; | ||
use crate::executor::{Argument, DeleteEntityQuery, QueryMessage, QueryType}; | ||
use crate::executor::{Argument, DeleteEntityQuery, QueryMessage, QueryType, SetHeadQuery}; | ||
use crate::utils::utc_dt_string_from_timestamp; | ||
|
||
type IsEventMessage = bool; | ||
|
@@ -86,17 +86,32 @@ impl Sql { | |
)) | ||
} | ||
|
||
pub fn set_head(&mut self, head: u64) -> Result<()> { | ||
let head = Argument::Int( | ||
pub async fn set_head( | ||
&mut self, | ||
head: u64, | ||
last_block_timestamp: u64, | ||
world_txns_count: u64, | ||
contract_address: Felt, | ||
) -> Result<()> { | ||
let head_arg = Argument::Int( | ||
head.try_into().map_err(|_| anyhow!("Head value {} doesn't fit in i64", head))?, | ||
); | ||
let last_block_timestamp_arg = | ||
Argument::Int(last_block_timestamp.try_into().map_err(|_| { | ||
anyhow!("Last block timestamp value {} doesn't fit in i64", last_block_timestamp) | ||
})?); | ||
let id = Argument::FieldElement(self.world_address); | ||
self.executor | ||
.send(QueryMessage::other( | ||
"UPDATE contracts SET head = ? WHERE id = ?".to_string(), | ||
vec![head, id], | ||
)) | ||
.map_err(|e| anyhow!("Failed to send set_head message: {}", e))?; | ||
|
||
self.executor.send(QueryMessage::new( | ||
"UPDATE contracts SET head = ?, last_block_timestamp = ? WHERE id = ?".to_string(), | ||
vec![head_arg, last_block_timestamp_arg, id], | ||
Comment on lines
+106
to
+107
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ohayo, sensei! Should Currently, the SQL statement only updates |
||
QueryType::SetHead(SetHeadQuery { | ||
head, | ||
last_block_timestamp, | ||
txns_count: world_txns_count, | ||
contract_address, | ||
}), | ||
))?; | ||
|
||
Ok(()) | ||
} | ||
|
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.
Ohayo! Watch out for that sneaky i64 to u64 conversion, sensei!
The conversion from
i64
tou64
forprevious_block_timestamp
could fail if the value is negative. Consider using a safe conversion method or handling potential errors. Here's a suggestion:This approach will provide a more informative error message if the conversion fails.