-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Export wallet address for Prometheus metrics #1206
Changes from 3 commits
a7991a4
b4f0a7b
4b0e937
ea51433
702e084
11b1733
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
So I'd opt for this message: return fmt.Errorf("get relayer bech32 wallet addresss: %w", err) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. Regardless, I like your idea of simply logging the the error rather than returning. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. I see. I mis-interpreted your first comment above. |
||
|
||
// 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 | ||
|
@@ -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 | ||
|
@@ -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) | ||
} | ||
} | ||
|
||
|
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 wonder how we are using the same address in this loop. Is it supposed to be the same address for all iterations?
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.
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...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 adjusted this logic. See note below.
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'm unfamiliar with how the relayer works, so I was wondering how we use the same address for different denoms.