Skip to content

Commit

Permalink
chore(sequencer): add instrumentation (#1761)
Browse files Browse the repository at this point in the history
## Summary
Added instrumentation to most async functions which did not have it.

## Background
Adding instrumentation to all async calls will aid in tracing since
spans will be emitted even if no events happen under them.

## Changes
- Added instrumentation to most async functions which did not have it.
- Added `err` argument to the `instrument` macro where applicable.
- Added fields to the `instrument` macro where applicable.

## Testing
Passing all tests.

## Changelogs
No updates required

## Related Issues
closes #1321
  • Loading branch information
ethanoroshiba authored Dec 19, 2024
1 parent 0332384 commit 96ee2ed
Show file tree
Hide file tree
Showing 42 changed files with 299 additions and 122 deletions.
2 changes: 1 addition & 1 deletion crates/astria-sequencer/src/accounts/component.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ pub(crate) struct AccountsComponent;
impl Component for AccountsComponent {
type AppState = GenesisAppState;

#[instrument(name = "AccountsComponent::init_chain", skip_all)]
#[instrument(name = "AccountsComponent::init_chain", skip_all, err)]
async fn init_chain<S>(mut state: S, app_state: &Self::AppState) -> Result<()>
where
S: accounts::StateWriteExt + assets::StateReadExt,
Expand Down
12 changes: 10 additions & 2 deletions crates/astria-sequencer/src/accounts/query.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,14 +28,18 @@ use tendermint::{
},
block::Height,
};
use tracing::instrument;
use tracing::{
instrument,
Level,
};

use crate::{
accounts::StateReadExt as _,
app::StateReadExt as _,
assets::StateReadExt as _,
};

#[instrument(skip_all, fields(%asset), err(level = Level::DEBUG))]
async fn ibc_to_trace<S: StateRead>(
state: S,
asset: &asset::IbcPrefixed,
Expand All @@ -47,7 +51,7 @@ async fn ibc_to_trace<S: StateRead>(
.ok_or_eyre("asset not found when user has balance of it; this is a bug")
}

#[instrument(skip_all, fields(%address))]
#[instrument(skip_all, fields(%address), err(level = Level::DEBUG))]
async fn get_trace_prefixed_account_balances<S: StateRead>(
state: &S,
address: &Address,
Expand All @@ -70,6 +74,7 @@ async fn get_trace_prefixed_account_balances<S: StateRead>(

/// Returns a list of [`AssetBalance`]s for the provided address. `AssetBalance`s are sorted
/// alphabetically by [`asset::Denom`].
#[instrument(skip_all)]
pub(crate) async fn balance_request(
storage: Storage,
request: request::Query,
Expand Down Expand Up @@ -112,6 +117,7 @@ pub(crate) async fn balance_request(
}
}

#[instrument(skip_all)]
pub(crate) async fn nonce_request(
storage: Storage,
request: request::Query,
Expand Down Expand Up @@ -150,6 +156,7 @@ pub(crate) async fn nonce_request(
}
}

#[instrument(skip_all, fields(%height), err(level = Level::DEBUG))]
async fn get_snapshot_and_height(storage: &Storage, height: Height) -> Result<(Snapshot, Height)> {
let snapshot = match height.value() {
0 => storage.latest_snapshot(),
Expand All @@ -173,6 +180,7 @@ async fn get_snapshot_and_height(storage: &Storage, height: Height) -> Result<(S
Ok((snapshot, height))
}

#[instrument(skip_all)]
async fn preprocess_request(
storage: &Storage,
request: &request::Query,
Expand Down
17 changes: 10 additions & 7 deletions crates/astria-sequencer/src/accounts/state_ext.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,10 @@ use cnidarium::{
};
use futures::Stream;
use pin_project_lite::pin_project;
use tracing::instrument;
use tracing::{
instrument,
Level,
};

use super::storage::{
self,
Expand Down Expand Up @@ -141,7 +144,7 @@ pub(crate) trait StateReadExt: StateRead + crate::assets::StateReadExt {
}
}

#[instrument(skip_all, fields(address = %address.display_address(), %asset), err)]
#[instrument(skip_all, fields(address = %address.display_address(), %asset), err(level = Level::WARN))]
async fn get_account_balance<'a, TAddress, TAsset>(
&self,
address: &TAddress,
Expand All @@ -165,7 +168,7 @@ pub(crate) trait StateReadExt: StateRead + crate::assets::StateReadExt {
.wrap_err("invalid balance bytes")
}

#[instrument(skip_all)]
#[instrument(skip_all, err)]
async fn get_account_nonce<T: AddressBytes>(&self, address: &T) -> Result<u32> {
let bytes = self
.get_raw(&keys::nonce(address))
Expand All @@ -186,7 +189,7 @@ impl<T: StateRead + ?Sized> StateReadExt for T {}

#[async_trait]
pub(crate) trait StateWriteExt: StateWrite {
#[instrument(skip_all, fields(address = %address.display_address(), %asset, balance), err)]
#[instrument(skip_all, fields(address = %address.display_address(), %asset, balance), err(level = Level::WARN))]
fn put_account_balance<'a, TAddress, TAsset>(
&mut self,
address: &TAddress,
Expand All @@ -205,7 +208,7 @@ pub(crate) trait StateWriteExt: StateWrite {
Ok(())
}

#[instrument(skip_all)]
#[instrument(skip_all, fields(address = %address.display_address(), nonce), err(level = Level::WARN))]
fn put_account_nonce<T: AddressBytes>(&mut self, address: &T, nonce: u32) -> Result<()> {
let bytes = StoredValue::from(storage::Nonce::from(nonce))
.serialize()
Expand All @@ -214,7 +217,7 @@ pub(crate) trait StateWriteExt: StateWrite {
Ok(())
}

#[instrument(skip_all, fields(address = %address.display_address(), %asset, amount), err)]
#[instrument(skip_all, fields(address = %address.display_address(), %asset, amount), err(level = Level::WARN))]
async fn increase_balance<'a, TAddress, TAsset>(
&mut self,
address: &TAddress,
Expand All @@ -241,7 +244,7 @@ pub(crate) trait StateWriteExt: StateWrite {
Ok(())
}

#[instrument(skip_all, fields(address = %address.display_address(), %asset, amount))]
#[instrument(skip_all, fields(address = %address.display_address(), %asset, amount), err(level = Level::DEBUG))]
async fn decrease_balance<'a, TAddress, TAsset>(
&mut self,
address: &TAddress,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,10 @@ use astria_eyre::eyre::{
};
use async_trait::async_trait;
use cnidarium::StateWrite;
use tracing::{
instrument,
Level,
};

use crate::{
action_handler::{
Expand All @@ -36,6 +40,7 @@ impl ActionHandler for BridgeLock {
Ok(())
}

#[instrument(skip_all, err(level = Level::DEBUG))]
async fn check_and_execute<S: StateWrite>(&self, mut state: S) -> Result<()> {
let from = state
.get_transaction_context()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,10 @@ use astria_eyre::eyre::{
};
use async_trait::async_trait;
use cnidarium::StateWrite;
use tracing::{
instrument,
Level,
};

use crate::{
action_handler::ActionHandler,
Expand All @@ -24,6 +28,7 @@ impl ActionHandler for BridgeSudoChange {
Ok(())
}

#[instrument(skip_all, err(level = Level::DEBUG))]
async fn check_and_execute<S: StateWrite>(&self, mut state: S) -> Result<()> {
let from = state
.get_transaction_context()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,10 @@ use astria_eyre::eyre::{
};
use async_trait::async_trait;
use cnidarium::StateWrite;
use tracing::{
instrument,
Level,
};

use crate::{
action_handler::{
Expand All @@ -28,6 +32,7 @@ use crate::{
#[async_trait]
impl ActionHandler for BridgeUnlock {
// TODO(https://github.com/astriaorg/astria/issues/1430): move checks to the `BridgeUnlock` parsing.
#[instrument(skip_all, err(level = Level::DEBUG))]
async fn check_stateless(&self) -> Result<()> {
ensure!(self.amount > 0, "amount must be greater than zero",);
ensure!(self.memo.len() <= 64, "memo must not be more than 64 bytes");
Expand All @@ -46,6 +51,7 @@ impl ActionHandler for BridgeUnlock {
Ok(())
}

#[instrument(skip_all, err(level = Level::DEBUG))]
async fn check_and_execute<S: StateWrite>(&self, mut state: S) -> Result<()> {
let from = state
.get_transaction_context()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,10 @@ use async_trait::async_trait;
use cnidarium::StateWrite;
use futures::StreamExt as _;
use tokio::pin;
use tracing::{
instrument,
Level,
};

use crate::{
action_handler::ActionHandler,
Expand All @@ -25,6 +29,7 @@ impl ActionHandler for FeeAssetChange {
Ok(())
}

#[instrument(skip_all, err(level = Level::DEBUG))]
async fn check_and_execute<S: StateWrite>(&self, mut state: S) -> eyre::Result<()> {
let from = state
.get_transaction_context()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,10 @@ use astria_eyre::eyre::{
};
use async_trait::async_trait;
use cnidarium::StateWrite;
use tracing::{
instrument,
Level,
};

use crate::{
action_handler::ActionHandler,
Expand All @@ -22,6 +26,7 @@ impl ActionHandler for FeeChange {

/// check that the signer of the transaction is the current sudo address,
/// as only that address can change the fee
#[instrument(skip_all, err(level = Level::DEBUG))]
async fn check_and_execute<S: StateWrite>(&self, mut state: S) -> eyre::Result<()> {
let from = state
.get_transaction_context()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,10 @@ use astria_eyre::eyre::{
};
use async_trait::async_trait;
use cnidarium::StateWrite;
use tracing::{
instrument,
Level,
};

use crate::{
action_handler::ActionHandler,
Expand All @@ -23,6 +27,7 @@ impl ActionHandler for IbcRelayerChange {
Ok(())
}

#[instrument(skip_all, err(level = Level::DEBUG))]
async fn check_and_execute<S: StateWrite>(&self, mut state: S) -> Result<()> {
let from = state
.get_transaction_context()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,10 @@ use astria_eyre::eyre::{
};
use async_trait::async_trait;
use cnidarium::StateWrite;
use tracing::{
instrument,
Level,
};

use crate::{
action_handler::ActionHandler,
Expand All @@ -21,6 +25,7 @@ impl ActionHandler for IbcSudoChange {
Ok(())
}

#[instrument(skip_all, err(level = Level::DEBUG))]
async fn check_and_execute<S: StateWrite>(&self, mut state: S) -> Result<()> {
let from = state
.get_transaction_context()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,10 @@ use penumbra_ibc::component::packet::{
Unchecked,
};
use penumbra_proto::core::component::ibc::v1::FungibleTokenPacketData;
use tracing::{
instrument,
Level,
};

use crate::{
accounts::{
Expand All @@ -60,6 +64,7 @@ use crate::{
#[async_trait]
impl ActionHandler for action::Ics20Withdrawal {
// TODO(https://github.com/astriaorg/astria/issues/1430): move checks to the `Ics20Withdrawal` parsing.
#[instrument(skip_all, err(level = Level::DEBUG))]
async fn check_stateless(&self) -> Result<()> {
ensure!(self.timeout_time() != 0, "timeout time must be non-zero",);
ensure!(self.amount() > 0, "amount must be greater than zero",);
Expand Down Expand Up @@ -95,6 +100,7 @@ impl ActionHandler for action::Ics20Withdrawal {
Ok(())
}

#[instrument(skip_all, err(level = Level::DEBUG))]
async fn check_and_execute<S: StateWrite>(&self, mut state: S) -> Result<()> {
let from = state
.get_transaction_context()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,10 @@ use astria_eyre::eyre::{
};
use async_trait::async_trait;
use cnidarium::StateWrite;
use tracing::{
instrument,
Level,
};

use crate::{
action_handler::ActionHandler,
Expand All @@ -26,6 +30,7 @@ impl ActionHandler for InitBridgeAccount {
Ok(())
}

#[instrument(skip_all, err(level = Level::DEBUG))]
async fn check_and_execute<S: StateWrite>(&self, mut state: S) -> Result<()> {
let from = state
.get_transaction_context()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,16 @@ use astria_eyre::eyre::{
};
use async_trait::async_trait;
use cnidarium::StateWrite;
use tracing::{
instrument,
Level,
};

use crate::action_handler::ActionHandler;

#[async_trait]
impl ActionHandler for RollupDataSubmission {
#[instrument(skip_all, err(level = Level::DEBUG))]
async fn check_stateless(&self) -> Result<()> {
// TODO: do we want to place a maximum on the size of the data?
// https://github.com/astriaorg/astria/issues/222
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,10 @@ use astria_eyre::eyre::{
};
use async_trait::async_trait;
use cnidarium::StateWrite;
use tracing::{
instrument,
Level,
};

use crate::{
action_handler::ActionHandler,
Expand All @@ -25,6 +29,7 @@ impl ActionHandler for SudoAddressChange {

/// check that the signer of the transaction is the current sudo address,
/// as only that address can change the sudo address
#[instrument(skip_all, err(level = Level::DEBUG))]
async fn check_and_execute<S: StateWrite>(&self, mut state: S) -> Result<()> {
let from = state
.get_transaction_context()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,10 @@ use astria_eyre::{
},
};
use cnidarium::StateWrite;
use tracing::{
instrument,
Level,
};

use crate::{
accounts::{
Expand Down Expand Up @@ -74,6 +78,7 @@ impl std::error::Error for InvalidNonce {}

#[async_trait::async_trait]
impl ActionHandler for Transaction {
#[instrument(skip_all, err(level = Level::DEBUG))]
async fn check_stateless(&self) -> Result<()> {
ensure!(!self.actions().is_empty(), "must have at least one action");

Expand Down Expand Up @@ -149,7 +154,7 @@ impl ActionHandler for Transaction {
// FIXME (https://github.com/astriaorg/astria/issues/1584): because most lines come from delegating (and error wrapping) to the
// individual actions. This could be tidied up by implementing `ActionHandler for Action`
// and letting it delegate.
#[expect(clippy::too_many_lines, reason = "should be refactored")]
#[instrument(skip_all, err(level = Level::DEBUG))]
async fn check_and_execute<S: StateWrite>(&self, mut state: S) -> Result<()> {
// Add the current signed transaction into the ephemeral state in case
// downstream actions require access to it.
Expand Down
Loading

0 comments on commit 96ee2ed

Please sign in to comment.