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 5 commits
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
18 changes: 13 additions & 5 deletions data/pools/transactionPool.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -737,16 +737,24 @@ func (pool *TransactionPool) recomputeBlockEvaluator(committedTxIds map[transact
for _, tx := range txgroup {
pool.statusCache.put(tx, err.Error())
}

switch err.(type) {
// metrics here are duplicated for historic reasons. stats is hardly used and should be removed in favor of asmstats
switch terr := err.(type) {
case *ledgercore.TransactionInLedgerError:
asmStats.CommittedCount++
stats.RemovedInvalidCount++
case transactions.TxnDeadError:
asmStats.InvalidCount++
if int(terr.LastValid-terr.FirstValid) > 20 {
// cutoff value here is picked as a somewhat arbitrary cutoff trying to separate longer lived transactions from very short lived ones
asmStats.ExpiredLongLivedCount++
}
asmStats.ExpiredCount++
stats.ExpiredCount++
case transactions.MinFeeError, *ledgercore.LeaseInLedgerError:
asmStats.InvalidCount++
case *ledgercore.LeaseInLedgerError:
asmStats.LeaseErrorCount++
stats.RemovedInvalidCount++
algorandskiy marked this conversation as resolved.
Show resolved Hide resolved
pool.log.Infof("Cannot re-add pending transaction to pool: %v", err)
case transactions.MinFeeError:
asmStats.MinFeeErrorCount++
stats.RemovedInvalidCount++
pool.log.Infof("Cannot re-add pending transaction to pool: %v", err)
default:
Expand Down
2 changes: 1 addition & 1 deletion ledger/ledgercore/error.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ func MakeLeaseInLedgerError(txid transactions.Txid, lease Txlease) *LeaseInLedge
func (lile *LeaseInLedgerError) Error() string {
// format the lease as address.
addr := basics.Address(lile.lease.Lease)
return fmt.Sprintf("transaction %v using an overlapping lease %s", lile.txid, addr.String())
return fmt.Sprintf("transaction %v using an overlapping lease (%s, %s)", lile.txid, lile.lease.Sender.String(), addr.String())
Copy link
Contributor

Choose a reason for hiding this comment

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

this is very confusing :) maybe rename addr to leaseData

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean? the "lease" in actual string to "(sender, lease)"

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, but addr := basics.Address(lile.lease.Lease) this is not address but the lease value. I'm referencing to addr.String() that prints lease data, not addr as one can think

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, I think

}

// BlockInLedgerError is returned when a block cannot be added because it has already been done
Expand Down
8 changes: 8 additions & 0 deletions logging/telemetryspec/metric.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,10 @@ type AssembleBlockStats struct {
StartCount int
IncludedCount int // number of transactions that are included in a block
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
ExpiredLongLivedCount int // number of expired transactions with non-super short LastValid values
LeaseErrorCount int // number of transactions removed because it has an already used lease
MinFee uint64
MaxFee uint64
AverageFee uint64
Expand Down Expand Up @@ -100,6 +104,10 @@ func (m AssembleBlockStats) String() string {
b.WriteString(fmt.Sprintf("StartCount:%d, ", m.StartCount))
b.WriteString(fmt.Sprintf("IncludedCount:%d, ", m.IncludedCount))
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("ExpiredLongLivedCount:%d, ", m.ExpiredLongLivedCount))
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