Add additional metrics by mslipper · Pull Request #2500 · tendermint/tendermint

mslipper · 2018-09-27T07:57:58Z

Continues addressing cosmos/cosmos-sdk#2169.

Updated all relevant documentation in docs
Updated all code comments where relevant
Wrote tests
Updated CHANGELOG_PENDING.md

Continues addressing cosmos/cosmos-sdk#2169.

melekes · 2018-09-27T08:09:57Z

mempool/metrics.go

+			Subsystem: MetricsSubsytem,
+			Name:      "tx_size_bytes",
+			Help:      "Transaction sizes in bytes.",
+			Buckets:   stdprometheus.ExponentialBuckets(1, 2, 10),


what's the last bucket - 1024 bytes? some of our users have 100KB transactions. the max size for a transaction right now is 21MB (block size)

Was unaware of what the best buckets would be, so I made a guess based on my knowledge of Bitcoin.

melekes · 2018-09-27T08:10:43Z

mempool/metrics.go

+	// Histogram of transaction sizes, in bytes.
+	TxSizeBytes metrics.Histogram
+	// Number of failed transactions.
+	FailedTxs metrics.Gauge


why gauge if it's only growing?

melekes · 2018-09-27T08:12:29Z

state/metrics.go

+			Namespace: namespace,
+			Subsystem: MetricsSubsystem,
+			Name:      "block_processing_time",
+			Help:      "Time between BeginBlock and EndBlock.",


unit? seconds? ms?

ms. Will add.

codecov-io · 2018-09-27T08:40:56Z

Codecov Report

❗ No coverage uploaded for pull request base (develop@4c4a95c). Click here to learn what that means.
The diff coverage is 41.46%.

@@            Coverage Diff             @@
##             develop    #2500   +/-   ##
==========================================
  Coverage           ?   61.19%           
==========================================
  Files              ?      198           
  Lines              ?    16556           
  Branches           ?        0           
==========================================
  Hits               ?    10131           
  Misses             ?     5565           
  Partials           ?      860

Impacted Files	Coverage Δ
mempool/mempool.go	`74.9% <100%> (ø)`
consensus/reactor.go	`71.61% <100%> (ø)`
p2p/metrics.go	`17.94% <14.28%> (ø)`
consensus/metrics.go	`15.59% <14.28%> (ø)`
mempool/metrics.go	`18.18% <16.66%> (ø)`
state/metrics.go	`25% <25%> (ø)`
state/execution.go	`75.45% <66.66%> (ø)`
node/node.go	`66.05% <84.61%> (ø)`

mempool/metrics.go

melekes

🍰 🌮 🍉

melekes · 2018-10-02T10:08:23Z

We also need to update the docs! https://tendermint.com/docs/tendermint-core/metrics.html#list-of-available-metrics

mslipper · 2018-10-08T04:53:08Z

@melekes docs should be good to go :)

xla

Thanks for the follow-up and introduction of even more metrics, this is ace!

Some style nits to be addressed.

xla · 2018-10-08T11:04:13Z

consensus/reactor.go

 				if numVotes := ps.RecordVote(); numVotes%votesToContributeToBecomeGoodPeer == 0 {
 					conR.Switch.MarkPeerAsGood(peer)
-				}
+	}


This reformatting seems incorrect.

xla · 2018-10-08T11:04:30Z

consensus/reactor.go

 					conR.Switch.MarkPeerAsGood(peer)
-				}
 			}
+}


Incorrect reformatting.

xla · 2018-10-08T11:04:58Z

docs/tendermint-core/metrics.md

+mempool\_tx\_size\_bytes |histogram |on dev| |transaction sizes in bytes
+mempool\_failed\_txs |counter|on dev| |number of failed transactions
+mempool\_recheck\_times |counter|on dev| |number of transactions rechecked in the mempool
+state\_block\_processing\_time|histogram|on dev| |time between BeginBlock and EndBlock in ms


This reformatting makes it harder to read the content. Let's revert.

xla · 2018-10-08T11:09:00Z

state/execution.go

 // It returns the application root hash (result of abci.Commit).
 func ExecCommitBlock(appConnConsensus proxy.AppConnConsensus, block *types.Block,
-	logger log.Logger, lastValSet *types.ValidatorSet, stateDB dbm.DB) ([]byte, error) {
+		logger log.Logger, lastValSet *types.ValidatorSet, stateDB dbm.DB) ([]byte, error) {


If we reformat this than one param per line wtih trailing comma and newline after the last param.

xla · 2018-10-08T11:09:34Z

state/execution.go

 // updateState returns a new State updated according to the header and responses.
 func updateState(state State, blockID types.BlockID, header *types.Header,
-	abciResponses *ABCIResponses) (State, error) {
+		abciResponses *ABCIResponses) (State, error) {


If we reformat this than one param per line wtih trailing comma and newline after the last param.

xla · 2018-10-08T11:09:37Z

state/execution.go

 // Returns a list of transaction results and updates to the validator set
 func execBlockOnProxyApp(logger log.Logger, proxyAppConn proxy.AppConnConsensus,
-	block *types.Block, lastValSet *types.ValidatorSet, stateDB dbm.DB) (*ABCIResponses, error) {
+		block *types.Block, lastValSet *types.ValidatorSet, stateDB dbm.DB) (*ABCIResponses, error) {


If we reformat this than one param per line wtih trailing comma and newline after the last param.

xla · 2018-10-08T11:09:56Z

node/node.go

 	blockExecLogger := logger.With("module", "state")
 	// make block executor for consensus and blockchain reactors to execute blocks
-	blockExec := sm.NewBlockExecutor(stateDB, blockExecLogger, proxyApp.Consensus(), mempool, evidencePool)
+	blockExec := sm.NewBlockExecutor(stateDB, blockExecLogger, proxyApp.Consensus(), mempool, evidencePool, sm.BlockExecutorWithMetrics(smMetrics))


This line should be broken up, it grew quit big.

jackzampolin · 2018-10-10T15:56:37Z

I think these comments are addressed @xla! Looks like this is ready to merge.

xla

👍 🍡

Thanks for addressing the style concerns.

Add additional metrics

2a4ff4e

Continues addressing cosmos/cosmos-sdk#2169.

mslipper requested review from ebuchman, melekes and xla as code owners September 27, 2018 07:57

melekes reviewed Sep 27, 2018

View reviewed changes

mslipper added 2 commits September 27, 2018 01:24

Add nop metrics to fix NPE

c300926

Tweak buckets, code review

df9edf9

melekes reviewed Sep 27, 2018

View reviewed changes

mempool/metrics.go Outdated Show resolved Hide resolved

Update buckets

4fb79db

melekes approved these changes Oct 1, 2018

View reviewed changes

Update docs with new metrics

793f645

mslipper requested a review from zramsay as a code owner October 4, 2018 19:03

xla suggested changes Oct 8, 2018

View reviewed changes

xla self-assigned this Oct 8, 2018

Code review updates

8f8a1ac

xla approved these changes Oct 10, 2018

View reviewed changes

xla merged commit 92343ef into tendermint:develop Oct 10, 2018

cason mentioned this pull request Aug 20, 2024

metrics: remove unused p2p_num_txs metric cometbft/cometbft#3766

Closed

Conversation

mslipper commented Sep 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Sep 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

melekes left a comment

Choose a reason for hiding this comment

Uh oh!

melekes commented Oct 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mslipper commented Oct 8, 2018

Uh oh!

xla left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jackzampolin commented Oct 10, 2018

Uh oh!

xla left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov-io commented Sep 27, 2018 •

edited

Loading

melekes commented Oct 2, 2018 •

edited

Loading