chore(sequencer): update instrumentation for all consensus & app functions #677
Conversation
There was a problem hiding this comment.
Thank you for doing the work and going through these!
I only have a few nits that you can mostly ignore (maybe except the note on using telemetry::display::hex vs the Display impl for tendermint::Hash).
As a side note (not introduced by this PR so need not be done here): we probably want to think about how best to name our spans. If the instrument macro is provided sans a name field, the name of its function is taken.
I find the names of the explicitly named functions too rusty (like App::deliver_tx_after_execution).
| @@ -259,7 +259,9 @@ impl App { | |||
| /// | |||
| /// Returns the transactions which were successfully decoded and executed | |||
| /// in both their [`SignedTransaction`] and raw bytes form. | |||
| #[instrument(name = "App::execute_block_data", skip(self, txs))] | |||
| #[instrument(name = "App::execute_block_data", skip_all, fields( | |||
| tx_count = txs.len() | |||
There was a problem hiding this comment.
Nit, I prefer to not abbreviate things.
| tx_count = txs.len() | |
| transaction_count = txs.len() |
There was a problem hiding this comment.
I would argue this isn't an abbreviation at least not by us, it's the name of it: Blocks have data which is an array of txs, this is executing those. https://docs.cometbft.com/main/spec/core/data_structures#data
| @@ -353,7 +355,9 @@ impl App { | |||
| /// | |||
| /// Note that the first two "transactions" in the block, which are the proposer-generated | |||
| /// commitments, are ignored. | |||
| #[instrument(name = "App::deliver_tx_after_execution", skip(self))] | |||
| #[instrument(name = "App::deliver_tx_after_execution", skip_all, fields( | |||
| tx_hash = %telemetry::display::hex(tx_hash), | |||
There was a problem hiding this comment.
Nit: same here, I'd prefer to not abbreviate.
| @@ -182,7 +206,9 @@ impl Consensus { | |||
| }) | |||
| } | |||
|
|
|||
| #[instrument(skip(self))] | |||
| #[instrument(skip_all, fields( | |||
| tx_hash = %telemetry::display::hex(&Sha256::digest(&deliver_tx.tx)) | |||
There was a problem hiding this comment.
Nit: see my other comment re abbreviation. It seems like tx is widely used in cometbft, so maybe it makes sense to use it here.
| @@ -182,7 +206,9 @@ impl Consensus { | |||
| }) | |||
| } | |||
|
|
|||
| #[instrument(skip(self))] | |||
| #[instrument(skip_all, fields( | |||
| tx_hash = %telemetry::display::hex(&Sha256::digest(&deliver_tx.tx)) | |||
There was a problem hiding this comment.
we need a tx hash method lol. this is annoying to see. don't need to do it here but just in general
Summary
Updates all consensus API calls to have specifically chosen data in the spans, and omits anything but context of function from children (ie handle has context, the funciton it calls doesn't need it)
Background
Debugging sequencer is hard, spans from handle request on process proposal were unreadable. Similar changes made in sequencer for debugging fix.
Changes