Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

metrics: split out /Transaction/AssembleBlock metrics #4795

Merged
merged 6 commits into from
Nov 17, 2022
Merged

metrics: split out /Transaction/AssembleBlock metrics #4795

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e69f926
Split out logging on invalid transactions in /Transaction/AssembleBlo…
iansuvak Nov 15, 2022
c17d3f4
add an explicit counter for transactions expired with LastValid set t…
iansuvak Nov 15, 2022
d2e08bc
simplify the type assertion in switch
iansuvak Nov 15, 2022
b6b079f
Address review feedback
iansuvak Nov 16, 2022
57e08ac
add log info back for LeaseInLedgerError
iansuvak Nov 17, 2022
3560a26
address feedback
iansuvak Nov 17, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
add an explicit counter for transactions expired with LastValid set t… 
…o MaxTxnLife
  • Loading branch information
@iansuvak
iansuvak committed Nov 15, 2022
commit c17d3f4c3d621692629e25c932612ba56efe7df8
5 changes: 5 additions & 0 deletions data/pools/transactionPool.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -694,6 +694,7 @@ func (pool *TransactionPool) recomputeBlockEvaluator(committedTxIds map[transact
pool.assemblyMu.Unlock()

next := bookkeeping.MakeBlock(prev)
proto := config.Consensus[next.CurrentProtocol]
iansuvak marked this conversation as resolved.
Show resolved Hide resolved
pool.numPendingWholeBlocks = 0
hint := pendingCount - int(knownCommitted)
if hint < 0 || int(knownCommitted) < 0 {
Expand Down Expand Up @@ -743,6 +744,10 @@ func (pool *TransactionPool) recomputeBlockEvaluator(committedTxIds map[transact
asmStats.CommittedCount++
stats.RemovedInvalidCount++
case transactions.TxnDeadError:
te := err.(transactions.TxnDeadError)
if proto.MaxTxnLife == uint64(te.LastValid-te.FirstValid) {
asmStats.ExpiredMaxLifeCount++
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be misleading, since we don't know if err.LastValid-err.FirstValid is a small enough to make an impact, say less than 10, or if it is proto.MaxTxnLife-1.
I think it will be better to check if it is less than 50, than compare to proto.MaxTxnLife.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that this is odd, I just didn't want to introduce an arbitrary magic number since any number is really arbitrary. Perhaps more useful would be an average transaction lifespan? Could implement it as running average by keeping running sum of lifetimes of expired txns?

Copy link
Contributor

Choose a reason for hiding this comment

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

Running average is too complex, and I don't suggest that. Also, it is not very helpful.

The "magic" number is the one that makes sense for the collected metrics.
I think, we are interested in learning if the txn life is 5 or 900. But we don't care if it is 900 or 990. I think anything more that 20 will fall into a long-term bucket. So, 20 is not a magic number, but helps characterize a certain behavior of transactions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made the change to 20. We can see what stats this comes out to and further tune when removing old stats.

}
asmStats.ExpiredCount++
stats.ExpiredCount++
case *ledgercore.LeaseInLedgerError:
Expand Down
4 changes: 3 additions & 1 deletion logging/telemetryspec/metric.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ type AssembleBlockStats struct {
InvalidCount int // number of transaction groups that are included in a block
MinFeeErrorCount int // number of transactions excluded because the fee is too low
ExpiredCount int // number of transactions removed because of expiration
ExpiredMaxLifeCount int // number of expired transactions with LastValid set to MaxTxnLife
LeaseErrorCount int // number of transactions removed because it has an already used lease
MinFee uint64
MaxFee uint64
Expand Down Expand Up @@ -105,7 +106,8 @@ func (m AssembleBlockStats) String() string {
b.WriteString(fmt.Sprintf("InvalidCount:%d, ", m.InvalidCount))
b.WriteString(fmt.Sprintf("MinFeeErrorCount:%d, ", m.MinFeeErrorCount))
b.WriteString(fmt.Sprintf("ExpiredCount:%d, ", m.ExpiredCount))
b.WriteString(fmt.Sprintf("LeaseErrorCount:%d, ", m.ExpiredCount))
b.WriteString(fmt.Sprintf("ExpiredMaxLifeCount:%d, ", m.ExpiredMaxLifeCount))
b.WriteString(fmt.Sprintf("LeaseErrorCount:%d, ", m.LeaseErrorCount))
b.WriteString(fmt.Sprintf("MinFee:%d, ", m.MinFee))
b.WriteString(fmt.Sprintf("MaxFee:%d, ", m.MaxFee))
b.WriteString(fmt.Sprintf("AverageFee:%d, ", m.AverageFee))
Expand Down