Skip to content

Conversation

@Evalir
Copy link
Member

@Evalir Evalir commented Nov 25, 2025

We're not following our set pattern for metrics (see node-components) and it makes it hard to know what we're really tracking and where.

We're not following our set pattern for metrics (see node-components) and it makes it hard to know what we're really tracking and where.
Copy link
Member Author

Evalir commented Nov 25, 2025

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

counter!("signet.builder.built_blocks").increment(1);
histogram!("signet.builder.built_blocks.tx_count").record(built_block.tx_count() as u32);
metrics::built_blocks().increment(1);
metrics::built_blocks_tx_count().record(built_block.tx_count() as u32);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these lines should both be collapsed into metrcis::record_bulit_block(&block) and the logic to increment, etc should be in the metrics file

Copy link
Member

@prestwich prestwich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is lowkey a NACK from me rn. this work predates our current metrics pattern. the new work doesn't follow our pattern, and adds metrics we don't necessariy want. is there a design doc for this I'm not seeing?

/// Logs a debug message if the channel is closed.
fn track_outbound_tx(&self, envelope: &alloy::consensus::TxEnvelope) {
counter!("signet.builder.flashbots.").increment(1);
metrics::flashbots_submissions().increment(1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto here. we don't want the caller to be responsible for the .increment(1), as it requires knowing the metric semantics

// Don't submit empty blocks
if sim_result.block.is_empty() {
counter!("signet.builder.flashbots.empty_block").increment(1);
metrics::flashbots_empty_blocks().increment(1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

let result =
self.prepare(&sim_result).instrument(span.clone()).await.inspect_err(|error| {
counter!("signet.builder.flashbots.bundle_prep_failures").increment(1);
metrics::flashbots_bundle_prep_failures().increment(1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

.instrument(submit_span.clone())
.await;

metrics::flashbots_submission_duration_ms()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

match response {
Ok(resp) => {
counter!("signet.builder.flashbots.bundles_submitted").increment(1);
metrics::flashbots_bundles_submitted().increment(1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

.inspect(|_| {
metrics::quincey_signature_duration_ms()
.record(start.elapsed().as_millis() as f64);
metrics::quincey_signatures().increment(1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

.inspect_err(|_| {
counter!("signet.builder.quincey_signature_failures").increment(1)
metrics::quincey_signature_duration_ms()
.record(start.elapsed().as_millis() as f64);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

counter!("signet.builder.quincey_signature_failures").increment(1)
metrics::quincey_signature_duration_ms()
.record(start.elapsed().as_millis() as f64);
metrics::quincey_signature_failures().increment(1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

.provider
.get_transaction_count(self.provider.default_signer_address())
.into_future()
.instrument(debug_span!("nonce_fetch"))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what exactly is this span doing? it records no info and has no real parent

.into_future()
.instrument(debug_span!("nonce_fetch"))
.await?;
metrics::nonce_fetch_duration_ms().record(start.elapsed().as_millis() as f64);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is this metric for? why do we want to track this?

// record how long it took to mine the transaction
// potential improvement: use the block timestamp to calculate the time elapsed
histogram!("metrics.tx_mine_time").record(start.elapsed().as_millis() as f64);
metrics::tx_mine_time_ms().record(start.elapsed().as_millis() as f64);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

// log whether the transaction reverted
if receipt.status() {
counter!("metrics.tx_succeeded").increment(1);
metrics::tx_succeeded().increment(1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

debug!("tx succeeded");
} else {
counter!("metrics.tx_reverted").increment(1);
metrics::tx_reverted().increment(1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

Err(PendingTransactionError::TxWatcher(WatchTxError::Timeout)) => {
// log that the transaction timed out
counter!("metrics.tx_not_mined").increment(1);
metrics::tx_not_mined().increment(1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

}
Err(e) => {
counter!("metrics.rpc_error").increment(1);
metrics::rpc_error().increment(1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants