feat: more server metrics #3975

douglaz · 2023-12-19T21:13:46Z

No description provided.

codecov · 2023-12-19T21:23:51Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (05a8497) 56.96% compared to head (6252157) 57.08%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3975      +/-   ##
==========================================
+ Coverage   56.96%   57.08%   +0.11%     
==========================================
  Files         193      193              
  Lines       43228    43326      +98     
==========================================
+ Hits        24624    24731     +107     
+ Misses      18604    18595       -9

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

dpc · 2023-12-20T00:06:19Z

modules/fedimint-ln-server/src/lib.rs

@@ -85,7 +85,19 @@ lazy_static! {
        "contracts::FundedContract::Outgoing"
    ))
    .unwrap();
-    static ref AMOUNTS_BUCKETS_SATS: Vec<f64> = vec![0.0, 0.5, 1.0, 1000.0];
+    static ref AMOUNTS_BUCKETS_SATS: Vec<f64> = vec![
+        0.0,


Isn't 0.0 useless? Only things lower or equal than zero would fall into this bucket, which seems impossible/unlikely and not very useful?

It's one way to track what's zero. Some fees will be zero.

One example where the zero makes it clear what's happening:

mint_redeemed_ecash_sats_bucket{le="0"} 0 mint_redeemed_ecash_sats_bucket{le="0.5"} 678 mint_redeemed_ecash_sats_bucket{le="1"} 722

mint_redeemed_ecash_fees_sats_bucket{le="0"} 756 mint_redeemed_ecash_fees_sats_bucket{le="0.5"} 756 mint_redeemed_ecash_fees_sats_bucket{le="1"} 756

I don't get it. We don't even have fees for ecash/consensus txes, do we? And peg-in/peg-outs must have non-zero fees?

It seems we do have fees for ecash transactions. Lightning fees could also be zero.

It seems we do have fees for ecash transactions

WAT? How? Why I don't know about it.

fedimint/modules/fedimint-mint-server/src/lib.rs

Line 351 in b013367

fee: self.cfg.consensus.fee_consensus.note_spend_abs,

fedimint/modules/fedimint-mint-common/src/config.rs

Line 70 in b013367

/// Fees charged for ecash transactions

Yep, every module could charge fees in theory. In practice we don't do it since it would massively complicate the client (especially for e-cash). It would be easy to get scenarios where coin selection runs into loops where adding/removing e-cash makes adding/removing more e-cash necessary because of fees.

Note: these fees are a separate concept from withdraw or LN fees, they'd be going to guardians (well, technically the federation would just become overcollateralized since there is no way to withdraw them).

It would be easy to get scenarios where coin selection runs into loops where adding/removing e-cash makes adding/removing more e-cash necessary because of fees.

I don't think hitting this would be common. It is exactly like in Bitcoin - not really a problem people complain about. A bit of looping, and tiny overpaying fees should do.

IMO, the bigger issue the whole new aspect of "guardians making money". We have no affordances for it, no means for them to do anything useful with it (withdraw or e.g. contribute to peg-outs, or something), etc.

modules/fedimint-ln-server/src/lib.rs

modules/fedimint-wallet-server/src/lib.rs

elsirion

It just occurred to me that process_input/output is called twice for every input/output: once to verify the tx on submission and another time to process after consensus has been achieved. This would lead to double-counting of most metrics in this PR.

A potential solution for that might be registering on_commit hooks with the database transaction which only run if the fedimint tx is processed successfully.

Do we already have metrics for accepted vs. rejected transactions btw?

modules/fedimint-mint-server/src/lib.rs

dpc · 2023-12-21T19:13:28Z

It just occurred to me that process_input/output is called twice for every input/output: once to verify the tx on submission and another time to process after consensus has been achieved.

Would it make sense to add another method to the trait for "this thing was already accepted". We could make it have a default no-op impl, so it wouldn't even break anything.

We wouldn't even give a handle to the db, so it would be limited to diagnostics. Alternatively the existing process_input could take an extra argument.

@elsirion

elsirion · 2023-12-21T20:41:35Z

Would it make sense to add another method to the trait for "this thing was already accepted"

That would be a lot of API surface only for metrics 😕 this would need to be implemented for inputs+outputs in ServerModule + IServerModule.

elsirion

Looks good to me now, but we should get into the habit of testing our PRs. The double-counting would likely have been caught by a test.

dpc · 2023-12-24T23:04:12Z

modules/fedimint-ln-server/src/lib.rs

+    updated_contract_account: ContractAccount,
+    dbtx: &mut DatabaseTransaction<'_>,
+) {
+    dbtx.on_commit(move || match updated_contract_account.contract {


Oh, I forgot we have this. So good. Very smart. Much 🧠 .

dpc · 2023-12-24T23:05:39Z

he double-counting would likely have been caught by a test.

Bugs in metrics are kind of whateva, :D IMO, and they are a PITA to test because they use global counter.

fedimint-backports · 2023-12-24T23:48:10Z

Backport failed for releases/v0.2, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin releases/v0.2
git worktree add -d .worktree/backport-3975-to-releases/v0.2 origin/releases/v0.2
cd .worktree/backport-3975-to-releases/v0.2
git switch --create backport-3975-to-releases/v0.2
git cherry-pick -x 62521575788147153abdd8248de417de74d5ee7a

douglaz requested review from a team as code owners December 19, 2023 21:13

douglaz added the backport releases/v0.2 label Dec 19, 2023

dpc reviewed Dec 20, 2023

View reviewed changes

modules/fedimint-ln-server/src/lib.rs Outdated Show resolved Hide resolved

dpc reviewed Dec 20, 2023

View reviewed changes

modules/fedimint-wallet-server/src/lib.rs Outdated Show resolved Hide resolved

dpc reviewed Dec 20, 2023

View reviewed changes

modules/fedimint-wallet-server/src/lib.rs Outdated Show resolved Hide resolved

douglaz force-pushed the more_metrics branch 2 times, most recently from 938fdd1 to a7ea1ad Compare December 20, 2023 15:28

dpc previously approved these changes Dec 21, 2023

View reviewed changes

dpc enabled auto-merge December 21, 2023 01:00

elsirion requested changes Dec 21, 2023

View reviewed changes

modules/fedimint-mint-server/src/lib.rs Show resolved Hide resolved

modules/fedimint-mint-server/src/lib.rs Outdated Show resolved Hide resolved

feat: more server metrics

6252157

douglaz dismissed dpc’s stale review via 6252157 December 22, 2023 23:32

douglaz force-pushed the more_metrics branch from a7ea1ad to 6252157 Compare December 22, 2023 23:32

douglaz requested review from dpc and elsirion December 22, 2023 23:34

elsirion approved these changes Dec 23, 2023

View reviewed changes

dpc reviewed Dec 24, 2023

View reviewed changes

dpc approved these changes Dec 24, 2023

View reviewed changes

dpc added this pull request to the merge queue Dec 24, 2023

Merged via the queue into fedimint:master with commit c64aa0e Dec 24, 2023
20 checks passed

github-actions bot mentioned this pull request Dec 24, 2023

Backport PR #3975 #3991

Closed

douglaz deleted the more_metrics branch December 26, 2023 04:00

douglaz restored the more_metrics branch December 26, 2023 04:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: more server metrics #3975

feat: more server metrics #3975

douglaz commented Dec 19, 2023

codecov bot commented Dec 19, 2023 •

edited

dpc Dec 20, 2023 •

edited

douglaz Dec 20, 2023 •

edited

douglaz Dec 20, 2023 •

edited

dpc Dec 20, 2023 •

edited

douglaz Dec 20, 2023 •

edited

dpc Dec 20, 2023

douglaz Dec 21, 2023 •

edited

dpc Dec 21, 2023

elsirion Dec 21, 2023

dpc Dec 21, 2023

elsirion left a comment

dpc commented Dec 21, 2023 •

edited

elsirion commented Dec 21, 2023

elsirion left a comment

dpc Dec 24, 2023

dpc commented Dec 24, 2023

fedimint-backports commented Dec 24, 2023

feat: more server metrics #3975

feat: more server metrics #3975

Conversation

douglaz commented Dec 19, 2023

codecov bot commented Dec 19, 2023 • edited

Codecov Report

dpc Dec 20, 2023 • edited

Choose a reason for hiding this comment

douglaz Dec 20, 2023 • edited

Choose a reason for hiding this comment

douglaz Dec 20, 2023 • edited

Choose a reason for hiding this comment

dpc Dec 20, 2023 • edited

Choose a reason for hiding this comment

douglaz Dec 20, 2023 • edited

Choose a reason for hiding this comment

dpc Dec 20, 2023

Choose a reason for hiding this comment

douglaz Dec 21, 2023 • edited

Choose a reason for hiding this comment

dpc Dec 21, 2023

Choose a reason for hiding this comment

elsirion Dec 21, 2023

Choose a reason for hiding this comment

dpc Dec 21, 2023

Choose a reason for hiding this comment

elsirion left a comment

Choose a reason for hiding this comment

dpc commented Dec 21, 2023 • edited

elsirion commented Dec 21, 2023

elsirion left a comment

Choose a reason for hiding this comment

dpc Dec 24, 2023

Choose a reason for hiding this comment

dpc commented Dec 24, 2023

fedimint-backports commented Dec 24, 2023

codecov bot commented Dec 19, 2023 •

edited

dpc Dec 20, 2023 •

edited

douglaz Dec 20, 2023 •

edited

douglaz Dec 20, 2023 •

edited

dpc Dec 20, 2023 •

edited

douglaz Dec 20, 2023 •

edited

douglaz Dec 21, 2023 •

edited

dpc commented Dec 21, 2023 •

edited