-
Notifications
You must be signed in to change notification settings - Fork 276
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.
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
add log for txpool tx track #1046
base: develop
Are you sure you want to change the base?
Conversation
@@ -1767,6 +1773,9 @@ func (pool *TxPool) calculateTxsLifecycle(txs types.Transactions, t time.Time) { | |||
for _, tx := range txs { | |||
if tx.Time().Before(t) { | |||
txLifecycle := t.Sub(tx.Time()) | |||
if txLifecycle >= time.Minute*30 { | |||
log.Warn("calculate tx life cycle, cost over 30 minutes", "tx", tx.Hash().String()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
may also print the time of txLifecycle
.
core/tx_pool.go
Outdated
@@ -431,6 +431,7 @@ func (pool *TxPool) loop() { | |||
if time.Since(pool.beats[addr]) > pool.config.Lifetime { | |||
list := pool.queue[addr].Flatten() | |||
for _, tx := range list { | |||
log.Info("evict tx for timeout", "tx", tx.Hash().String()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we want to add these logs (this and the others)? Just for debugging?
Better to add most of these as Debug logs, otherwise there will be too many logs during normal operation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just want display the tx active state in txpool to make us easy to investigate a tx track
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also think it's not a good idea to spam INFO
logs. Then the general logs output when running a normal node is just too overwhelming and one can't make sense out of it anymore. If need be better to move to DEBUG
and run the nodes that should have create the metrics on debug level
Maybe it's worth thinking about the following (I'm not sure about exact capabilities Prometheus+Grafana): instead of logging this and parsing the logs, maybe we can use Prometheus to store different "events" per tx hash and transform them with some query. This way we can visualize whatever we're interested in and filter the "active tx".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prometheus+Grafana
doesn't work for it. Metrics just show a trend of an event, they can't show specified info. for example, txpool_txfifecycle
it just shows tx occupied time at different percentile ratios, but it can't show which tx cost how much, prometheus can't support that.
If you want to dig some tx why cost so much, I think there are two ways: 1) log, 2) trace. trace need a lot of development.
I also think it's not a good idea to spam INFO logs
for this, I think, it's not too bad. if the log really affect the geth's performance, we can downgrade the log level again
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the general logs output when running a normal node is just too overwhelming and one can't make sense out of it anymore
This was not addressed in your reply. Our main concern is not performance but comprehension when we're looking at the logs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When looking at all the logs to see whether the node is normal, this is only useful when starting up. After that, we should filter the specified log to check.
Co-authored-by: colin <102356659+colinlyguo@users.noreply.github.com>
Co-authored-by: colin <102356659+colinlyguo@users.noreply.github.com>
…t/add_log_for_long_sustain_tx
1. Purpose or design rationale of this PR
add log for tx that be consumed in txpool
2. PR title
Your PR title must follow conventional commits (as we are doing squash merge for each PR), so it must start with one of the following types:
3. Deployment tag versioning
Has the version in
params/version.go
been updated?4. Breaking change label
Does this PR have the
breaking-change
label?