Skip to content

fix: transaction per block metrics#3107

Merged
howardwu merged 6 commits intoProvableHQ:mainnetfrom
miazn:feat/codec-metrics
Feb 26, 2024
Merged

fix: transaction per block metrics#3107
howardwu merged 6 commits intoProvableHQ:mainnetfrom
miazn:feat/codec-metrics

Conversation

@miazn
Copy link
Collaborator

@miazn miazn commented Feb 20, 2024

Motivation

To go along with adding TCP Events received, this PR adds TCP Events sent, as well as adds the node/router plaintext Messages sent and received. edit: removed these since part of the histogram labeling was taking up unnecessary space in prometheus, and the metrics arent really needed
It also changes the transactions per block to be a gauge rather than a counter (which we already registered it as here, since a counter cannot decrease, thus fixing the transaction metrics in Grafana

@miazn
Copy link
Collaborator Author

miazn commented Feb 20, 2024

@joske I believe we will also have to update the TPS stat in the Grafana dash, should it just be the avg() rather than the sum(rate())?

@miazn miazn changed the title Feat/codec metrics fix: transaction per block metric fix Feb 21, 2024
@miazn miazn changed the title fix: transaction per block metric fix fix: transaction per block metrics Feb 21, 2024
@howardwu howardwu requested a review from joske February 23, 2024 02:53

metrics::gauge(metrics::blocks::HEIGHT, next_block.height() as f64);
metrics::counter(metrics::blocks::TRANSACTIONS, next_block.transactions().len() as u64);
metrics::increment_gauge(metrics::blocks::TRANSACTIONS, next_block.transactions().len() 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.

@joske the intention is to calculate the number of transactions per block here. Will increment be the right operation?

Copy link
Contributor

Choose a reason for hiding this comment

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

No this will add next_block.transactions().len() to the existing value of the gauge. I think you should use the metrics::gauge method, which will set the current value of transactions per block.

Copy link
Collaborator Author

@miazn miazn Feb 23, 2024

Choose a reason for hiding this comment

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

Since prometheus is a time series database, even if we set this gauge to be the existing number of transactions, it would not be per block. Since we scrape the endpoint every 15s , I'm pretty confident that we would only get the reading of the current state, i.e. we would be "missing" data from one block if blocktime is under 15 seconds.

I decided to make the metric increment because the metric name is snarkos_blocks_transactions_total and I figured this way we would capture all the data. Now, it is correct that this metric should be a counter, since it is always increasing. However, our snarkVM implementation only exposes an increment_counter function that increases by the constant 1 .

I think that this transaction metric should be a total, to reflect the metric name as well as not miss any block data. The best way to do this would be to change the increment counter function in snarkvm to take a value, rather than make it a constant, but switching the metric to a gauge was the lowest-lift method. Let me know what is preferred here.

@howardwu howardwu merged commit 9dbb8d7 into ProvableHQ:mainnet Feb 26, 2024
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.

4 participants