[Faucet] Add balance metric #19681
[Faucet] Add balance metric #19681
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
3 Skipped Deployments
|
stefan-mysten
requested review from
amnn,
leecchh,
manolisliolios,
wlmyng and
jordanjennings-mysten
stefan-mysten
force-pushed
the
faucet_add_total_balance_metric
branch
from
4af56ac
to
f82ddbc
Compare
There was a problem hiding this comment.
I don't think this works too well, for a couple of reasons:
- I think you've only covered the non-batched case.
- The request is happening before you execute the transaction.
- You're asking RPC what it thinks the balance is, but that may account for coins that are not in the faucet's queue (imagine someone transfers a giant coin to the faucet after it starts -- it won't know about it, but the balance will report a large number).
- We are now making a non-trivial number of extra requests to RPC.
Here is what I would do instead:
- Don't add any more RPC calls, just track the balances of the coins you are already reading.
- Instrument the paths where coins enter and leave the queue to hit both endpoints.
thoughts about this :
at best if you wanted to avoid that "high cardinality" problem each coin amount you could use its position in nth largest sorted so labels don't become unique:
you would get some flexibility in querying, could make a bar chart showing the distribution of balances across each coin, seems unnecessary to me, since coin merging should make coin distribution flat? but its an idea and could help debug coin merging. |
|
Yes, sorry I should have been clearer, clarifications follow:
That's right, in particular what I want to measure is the total balance in the coin queue, so it's not going to include coins that are committed to transactions in flight, but I think that's okay because we want to know what the size of our "reservoir" is and if it consistently dips below some threshold, we know we need to fill it back up.
yes, this is what I had in mind -- a single gauge in prometheus that tracks the total balance of all the coins in the in-memory queue. |
|
Thanks @amnn and @jordanjennings-mysten, I will revise it later today. |
stefan-mysten
force-pushed
the
faucet_add_total_balance_metric
branch
from
6fc9bc9
to
ef221bc
Compare
|
@amnn @jordanjennings-mysten I went with the following solution
Let me know your thoughts. |
| let metrics = FaucetMetrics::new(prometheus_registry); | ||
| // set initial balance when faucet starts | ||
| metrics.balance.set(balance as i64); |
There was a problem hiding this comment.
Out of interest, is it possible to sum it as an i64 to begin with?
There was a problem hiding this comment.
That does not work unfortunately as balance.value() in the lines above is u64, so I can only sum it to u64.
| @@ -520,6 +523,7 @@ impl SimpleFaucet { | |||
| .sign_and_execute_txn(uuid, recipient, coin_id, tx_data, false) | |||
| .await?; | |||
| self.metrics.total_coin_requests_succeeded.inc(); | |||
| self.metrics.balance.add(-(total_amount as i64)); | |||
There was a problem hiding this comment.
This doesn't match the actual SUI spent by the transaction (e.g. the amount sent, but also the amount spent on the transaction for computation, storage etc), so it will diverge over time.
One way to handle this is to request balance changes when executing the transaction. The balance change for the faucet's address, should match the balance change on the coin, and can be applied to the metric as a delta. I would suggest moving that logic into sign_and_execute_txn so you only need to handle it in one place.
Description
Added a balance metric for alerting when we need to top it up.
Test plan
How did you test the new or updated feature?
Release notes
Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required.
For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates.