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

Jump to bottom

Export wallet address for Prometheus metrics #1206

Merged
merged 6 commits into from
Jun 8, 2023
Merged

Export wallet address for Prometheus metrics #1206

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a7991a4
export relayer address for pro
boojamya Jun 5, 2023
b4f0a7b
address in updateFeesSpent
boojamya Jun 5, 2023
4b0e937
make error messages consistent
boojamya Jun 5, 2023
ea51433
log error rather than return
boojamya Jun 5, 2023
702e084
handle 0 balance
boojamya Jun 5, 2023
11b1733
Merge branch 'main' into dan/export-address
boojamya Jun 7, 2023
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
10 changes: 8 additions & 2 deletions relayer/chains/cosmos/cosmos_chain_processor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -523,14 +523,20 @@ func (ccp *CosmosChainProcessor) CurrentRelayerBalance(ctx context.Context) {
zap.Error(err),
)
}

address, err := ccp.chainProvider.Address()
if err != nil {
ccp.log.Error(
"Failed to get relayer bech32 wallet addresss",
zap.Error(err),
)
}
// Print the relevant gas prices
for _, gasDenom := range *ccp.parsedGasPrices {
for _, balance := range relayerWalletBalance {
if balance.Denom == gasDenom.Denom {
// Convert to a big float to get a float64 for metrics
f, _ := big.NewFloat(0.0).SetInt(balance.Amount.BigInt()).Float64()
ccp.metrics.SetWalletBalance(ccp.chainProvider.ChainId(), ccp.chainProvider.Key(), balance.Denom, f)
ccp.metrics.SetWalletBalance(ccp.chainProvider.ChainId(), ccp.chainProvider.Key(), address, balance.Denom, f)
Copy link

Choose a reason for hiding this comment

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

I wonder how we are using the same address in this loop. Is it supposed to be the same address for all iterations?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I'm not sure I full understand what you're saying.
It should however be noted that this metric will NOT be logged if somehow the wallet balance is exactly 0.

Maybe we should consider removing/altering this if statement...

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 adjusted this logic. See note below.

Copy link

Choose a reason for hiding this comment

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

I'm unfamiliar with how the relayer works, so I was wondering how we use the same address for different denoms.

}
}
}
Expand Down
11 changes: 7 additions & 4 deletions relayer/chains/cosmos/tx.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,8 +213,11 @@ func (cc *CosmosProvider) broadcastTx(
cc.LogFailedTx(rlyResp, err, msgs)
return err
}

cc.UpdateFeesSpent(cc.ChainId(), cc.Key(), fees)
address, err := cc.Address()
if err != nil {
return fmt.Errorf("Failed to get relayer bech32 wallet addresss, %w", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

style nit: Go convention is to use lower case for error messages. Also the comma is uncommon. I'm surprised a linter isn't catching this.

So

return fmt.Errorf("failed to get relayer bech32 wallet addresss: %w", err)

Copy link
Contributor

Choose a reason for hiding this comment

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

I also typically exclude "failed to" and other words because the error message, as it travels up the stack, often looks like:

failed to do X: error doing Y: failed to get Z

So I'd opt for this message:

return fmt.Errorf("get relayer bech32 wallet addresss: %w", err)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted! Yea, this repo is fairly inconsistent with cap : /

Since we switched from returning to logging the error, I kept "failed..."

}
cc.UpdateFeesSpent(cc.ChainId(), cc.Key(), address, fees)
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we sure it's safe to return here if we can't get the address?

This changes the control flow. If we error here, then line 225 go cc.waitForTx(asyncCtx, res.Hash, msgs, asyncTimeout, asyncCallback) never gets called.

This differs from the previous control flow that will always reach line 225.

We could log an error and continue as normal; only recording the metric if we get an address.

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 think the previous control flow does return an error prior to 225? line: 199 and 214.
Maybe I'm overlooking something.

Regardless, I like your idea of simply logging the the error rather than returning.

Copy link
Contributor

@DavidNix DavidNix Jun 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Line 199 and 214 do return an error and exit appropriately.

However, we're introducing a new error path if cc.Address() fails. cc.waitForTx will never be called if cc.Address() fails.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. I see. I mis-interpreted your first comment above.
Thanks for the explanations @DavidNix


// TODO: maybe we need to check if the node has tx indexing enabled?
// if not, we need to find a new way to block until inclusion in a block
Expand Down Expand Up @@ -1257,7 +1260,7 @@ func (cc *CosmosProvider) NewClientState(
}, nil
}

func (cc *CosmosProvider) UpdateFeesSpent(chain, key string, fees sdk.Coins) {
func (cc *CosmosProvider) UpdateFeesSpent(chain, key, address string, fees sdk.Coins) {
// Don't set the metrics in testing
if cc.metrics == nil {
return
Expand All @@ -1270,7 +1273,7 @@ func (cc *CosmosProvider) UpdateFeesSpent(chain, key string, fees sdk.Coins) {
for _, fee := range cc.TotalFees {
// Convert to a big float to get a float64 for metrics
f, _ := big.NewFloat(0.0).SetInt(fee.Amount.BigInt()).Float64()
cc.metrics.SetFeesSpent(chain, key, fee.GetDenom(), f)
cc.metrics.SetFeesSpent(chain, key, address, fee.GetDenom(), f)
}
}

Expand Down
10 changes: 5 additions & 5 deletions relayer/processor/metrics.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,18 +26,18 @@ func (m *PrometheusMetrics) SetLatestHeight(chain string, height int64) {
m.LatestHeightGauge.WithLabelValues(chain).Set(float64(height))
}

func (m *PrometheusMetrics) SetWalletBalance(chain, key, denom string, balance float64) {
m.WalletBalance.WithLabelValues(chain, key, denom).Set(balance)
func (m *PrometheusMetrics) SetWalletBalance(chain, key, address, denom string, balance float64) {
m.WalletBalance.WithLabelValues(chain, key, address, denom).Set(balance)
}

func (m *PrometheusMetrics) SetFeesSpent(chain, key, denom string, amount float64) {
m.FeesSpent.WithLabelValues(chain, key, denom).Set(amount)
func (m *PrometheusMetrics) SetFeesSpent(chain, key, address, denom string, amount float64) {
m.FeesSpent.WithLabelValues(chain, key, address, denom).Set(amount)
}

func NewPrometheusMetrics() *PrometheusMetrics {
packetLabels := []string{"path", "chain", "channel", "port", "type"}
heightLabels := []string{"chain"}
walletLabels := []string{"chain", "key", "denom"}
walletLabels := []string{"chain", "key", "address", "denom"}
registry := prometheus.NewRegistry()
registerer := promauto.With(registry)
return &PrometheusMetrics{
Expand Down