Add wallet_key_count metric #76

qustavo · 2021-10-06T09:49:14Z

This PR adds a new wallet metric: wallet_key_count which reports the current number of keys derived per account.
This metric is particularly useful when running a recovery with large gaps.

When recovering a node (see recovery.md) we need to specify a recovery window value:

The recovery window is a metric that the on-chain rescanner will use to determine when all the "used" addresses have been found. If the recovery window is two, lnd will fail to find funds in any addresses generated after the point in which two consecutive addresses were generated but never used. If an lnd on-chain wallet was extensively used, then users may want to increase the default value.

To find an accurate recovery window we could rely on the wallet key count (see lightningnetwork/lnd#5767).

This PR also bumps lndclient to :lnd-13-0 (https://github.com/lightninglabs/lndclient/tree/lnd-13-0) which introduces the ListAccounts call.

carlaKC

Thanks for the PR! Sorry that it took some time to get to review.
Just some versioning comments from me.

Any interest in adding these metrics to our provisioned grafana dashboards so they're more easily available?

collectors/channels_collector.go

carlaKC · 2021-10-25T08:36:56Z

collectors/errors.go

+	code := status.Code(err)
+	return code == codes.Unimplemented


could inline this to status.Code(err) == codes.Unimplemented?

carlaKC · 2021-10-25T08:39:16Z

go.mod

-	github.com/prometheus/client_golang v0.9.3
-	github.com/stretchr/testify v1.5.1
-	golang.org/x/text v0.3.2 // indirect
+	github.com/lightninglabs/lndclient v1.0.1-0.20211001112454-5bb2b557e003


We try to stick to tagged versions of lndclient, could you use v0.13.0-9? That should have all the calls you need.

Sure! I used a branch instead of a tag because this change: lightninglabs/lndclient@5bb2b55, which is needed for the PR has not been tagged yet. As soon as it get tagged I can reference it here

Ah my mistake, thought that was included in the 13-09 tag! Pushed v0.13.0-10 to lndclient now, you can use that :)

awesome, thanks!

carlaKC · 2021-10-25T08:40:47Z

collectors/wallet_collector.go

+	// Nodes prior to version 0.13.0 don't implement ListAccounts and will
+	// return a Unimplemented error.


If we bump to lndclient-13.0 it'll require that lnd is running at least lnd version 13.0 to start up (will fail for older clients).

We use this versioning to ensure that we're "safe" to use all new APIs and don't need to worry about unimplemented errors. Happy for this PR to just bump the required version to 13.0 and then we can assume that this API is supported.

when bumped to 0.13.0 lndmon still works against a lnd-0.12.x though

Ah yeah, we've specifically pinned minimum version to 0.11. If you just delete that CheckVersion field, it'll enforce lndclient's minimum version which aligns with the tag.

I'm not sure I follow, if I delete CheckVersion we will drop versions older than 0.13? Is that something we want for this PR?

Yes, generally when we update our projects to depend on a new API we bump the minimum version. Otherwise it can get awfully complex trying to figure out which endpoints we do/don't have access to. If you want to upgrade your lndmon to the version with this feature, you also need to bump you lnd node to at least 0.13.

Ok, if we drop versions older than 0.13 then I can safely remove the status.Code(err) == codes.Unimplemented check right?

Yeah exactly! Just makes life a lot easier :)

go.mod

qustavo · 2021-10-25T13:14:33Z

Any interest in adding these metrics to our provisioned grafana dashboards so they're more easily available?

how? is that dashboard open/public?

guggero · 2021-10-25T13:15:24Z

Yes, they're here: https://github.com/lightninglabs/lndmon/tree/master/grafana/provisioning/dashboards

qustavo · 2021-10-26T09:40:08Z

Yes, they're here: https://github.com/lightninglabs/lndmon/tree/master/grafana/provisioning/dashboards

Thanks, I added the metric to a new panel

carlaKC

One question about metric label + issue with grafana, nearly there!

carlaKC · 2021-11-02T10:07:21Z

collectors/wallet_collector.go

 	errChan chan<- error) *WalletCollector {

+	// these labels are specific for key_count metrics
+	keyCountLabels := []string{"account_name", "address_type"}


Could internal and external be additional labels as well? Rather than needing to split out into two separate metrics? Since they seem functionally the same except for internal/external differentiation.

Yes, that makes sense, will change it

How would you call the label? I can think of visibility or context or external={true|false} ?

key_type or key_branch?

key_type it is

carlaKC

Metrics look good! Just need a little work on the dashboard.

carlaKC · 2021-11-05T10:16:50Z

grafana/provisioning/dashboards/chain.json

+          "refId": "A"
+        }
+      ],
+      "title": "Panel Title",


Name for the panel rather than "panel title"?

carlaKC · 2021-11-05T10:26:35Z

I would suggest something like:

{
  "type": "graph",
  "title": "Address Types",
  "gridPos": {
    "x": 12,
    "y": 16,
    "w": 12,
    "h": 9
  },
  "datasource": "Prometheus",
  "aliasColors": {},
  "bars": true,
  "dashLength": 10,
  "dashes": false,
  "description": "",
  "fill": 1,
  "fillGradient": 0,
  "hiddenSeries": false,
  "id": 13,
  "legend": {
    "avg": false,
    "current": false,
    "max": false,
    "min": false,
    "show": false,
    "total": false,
    "values": false
  },
  "lines": false,
  "linewidth": 1,
  "nullPointMode": "null",
  "options": {
    "dataLinks": []
  },
  "percentage": false,
  "pointradius": 2,
  "points": false,
  "renderer": "flot",
  "seriesOverrides": [],
  "spaceLength": 10,
  "stack": false,
  "steppedLine": false,
  "targets": [
    {
      "refId": "A",
      "expr": "lnd_wallet_key_count",
      "intervalFactor": 1,
      "format": "time_series",
      "legendFormat": "{{address_type}}"
    }
  ],
  "thresholds": [],
  "timeFrom": null,
  "timeRegions": [],
  "timeShift": null,
  "tooltip": {
    "shared": false,
    "sort": 0,
    "value_type": "individual"
  },
  "xaxis": {
    "buckets": null,
    "mode": "series",
    "name": null,
    "show": true,
    "values": [
      "max"
    ]
  },
  "yaxes": [
    {
      "format": "short",
      "label": null,
      "logBase": 1,
      "max": null,
      "min": null,
      "show": true
    },
    {
      "format": "short",
      "label": null,
      "logBase": 1,
      "max": null,
      "min": null,
      "show": true
    }
  ],
  "yaxis": {
    "align": false,
    "alignLevel": null
  },
  "links": []
}

{
  "type": "graph",
  "title": "Address Types",
  "gridPos": {
    "x": 0,
    "y": 25,
    "w": 12,
    "h": 9
  },
  "datasource": "Prometheus",
  "aliasColors": {},
  "bars": true,
  "dashLength": 10,
  "dashes": false,
  "description": "",
  "fill": 1,
  "fillGradient": 0,
  "hiddenSeries": false,
  "id": 12,
  "legend": {
    "avg": false,
    "current": false,
    "max": false,
    "min": false,
    "show": false,
    "total": false,
    "values": false
  },
  "lines": false,
  "linewidth": 1,
  "nullPointMode": "null",
  "options": {
    "dataLinks": []
  },
  "percentage": false,
  "pointradius": 2,
  "points": false,
  "renderer": "flot",
  "seriesOverrides": [],
  "spaceLength": 10,
  "stack": false,
  "steppedLine": false,
  "targets": [
    {
      "refId": "A",
      "expr": "sum(lnd_wallet_key_count{key_type=\"internal\"})",
      "intervalFactor": 1,
      "format": "time_series",
      "legendFormat": "Internal Key Count"
    },
    {
      "refId": "B",
      "expr": "sum(lnd_wallet_key_count{key_type=\"external\"})",
      "intervalFactor": 1,
      "format": "time_series",
      "legendFormat": "External Key Count"
    }
  ],
  "thresholds": [],
  "timeFrom": null,
  "timeRegions": [],
  "timeShift": null,
  "tooltip": {
    "shared": false,
    "sort": 0,
    "value_type": "individual"
  },
  "xaxis": {
    "buckets": null,
    "mode": "series",
    "name": null,
    "show": true,
    "values": [
      "max"
    ]
  },
  "yaxes": [
    {
      "format": "short",
      "label": null,
      "logBase": 1,
      "max": null,
      "min": null,
      "show": true
    },
    {
      "format": "short",
      "label": null,
      "logBase": 1,
      "max": null,
      "min": null,
      "show": true
    }
  ],
  "yaxis": {
    "align": false,
    "alignLevel": null
  },
  "links": []
}

You'd need to update this json a bit (datasource, namespace, better titles etc), but should get you most of the way there.

qustavo · 2021-11-09T13:09:33Z

You are suggesting two different panels? or both graphs belong to the same panel?
Also, I'm using "datasource": "$datasource" following what it's already in the rest of the dashboards, in your example it is using "datasource": "Prometheus", is that changed on purpose?

carlaKC · 2021-12-13T09:55:04Z

Sorry somehow completely missed this comment!

You are suggesting two different panels?

Was thinking 2x panels?

"datasource": "Prometheus", is that changed on purpose?

Nope, should be "$datasource", those are just my local deafults.

carlaKC

Could you post a screenshot of what your dash looks like with these changes? Mine is kinda unreadable.

Can also cut the dashboard part/ add in a follow up to get the prometheus change in.

qustavo · 2021-12-22T08:05:30Z

@carlaKC I just noticed that and it happened when I started using lnd-0.14.0. Earlier versions only have a default account but 0.14.0 now has more accounts (named act:<N>) . So my initial chart is rendering well, I'll need to investigate and see what's the best way to display this info.

qustavo · 2021-12-22T08:48:00Z

After filtering out the non-default wallets in the chart this is the result:

Also, are they all the collected metrics rendered as charts? do we need to do that for this one?

carlaKC · 2022-01-04T13:11:21Z

@carlaKC I just noticed that and it happened when I started using lnd-0.14.0. Earlier versions only have a default account but 0.14.0 now has more accounts (named act:<N>) . So my initial chart is rendering well, I'll need to investigate and see what's the best way to display this info.

Ah interesting! Difficult to display succinctly.

We could just change the existing chart to a graph and go with something like this:

It's not pretty, but has all the information exposed at least.

Also, are they all the collected metrics rendered as charts? do we need to do that for this one?

Generally metrics are, but it's not a strict requirement. If you want to just drop the grafana change, we can just merge the addition of the metric to prometheus!

qustavo · 2022-01-05T07:53:07Z

I prefer to drop the changes to Grafana since it is not pretty to display, and different users might need different chart details/accounts.
Will update the PR now

carlaKC

🏅

lightninglabs-deploy · 2022-01-12T08:51:23Z

@ellemouton: review reminder

ellemouton

Looks good ⭐ Left a few optional nits and one comment about a comment that looks a bit out of place

collectors/wallet_collector.go

ellemouton · 2022-01-12T12:21:13Z

collectors/wallet_collector.go

 		float64(walletBal.Unconfirmed),
 	)
+
+	accounts, err := u.lnd.WalletKit.ListAccounts(context.Background(), "", 0)


nit: wrap at 80 chars

isn't it already below 80? (vim shows :78)

mine shows 83. Is your tab size set to 8 spaces?

collectors/wallet_collector.go

ellemouton · 2022-01-12T13:38:57Z

thanks for the quick update @qustavo 🚀 almost there, just need to also rebase master as your pr now includes some other commits

Signed-off-by: Gustavo Chain <[email protected]>

ellemouton

LGTM 🚀

qustavo mentioned this pull request Oct 6, 2021

mod: bump lndclient to v0.13.0-9 #75

Closed

qustavo force-pushed the wallet_key_count branch from f721b6a to 55a8065 Compare October 6, 2021 10:13

carlaKC self-requested a review October 20, 2021 14:10

carlaKC reviewed Oct 25, 2021

View reviewed changes

qustavo force-pushed the wallet_key_count branch from 55a8065 to 254445a Compare October 25, 2021 12:59

qustavo requested a review from carlaKC October 25, 2021 13:05

qustavo force-pushed the wallet_key_count branch from c0fd49a to 7b4f4a7 Compare October 26, 2021 13:30

carlaKC reviewed Nov 2, 2021

View reviewed changes

qustavo force-pushed the wallet_key_count branch 2 times, most recently from 4800c9b to 910de60 Compare November 2, 2021 11:16

qustavo requested a review from carlaKC November 2, 2021 11:17

carlaKC reviewed Nov 5, 2021

View reviewed changes

qustavo force-pushed the wallet_key_count branch 2 times, most recently from 595f558 to e0eca07 Compare November 9, 2021 14:49

qustavo requested a review from carlaKC December 8, 2021 09:08

carlaKC reviewed Dec 13, 2021

View reviewed changes

qustavo requested a review from carlaKC December 29, 2021 15:34

qustavo force-pushed the wallet_key_count branch from e0eca07 to 40111bc Compare January 5, 2022 08:33

carlaKC approved these changes Jan 5, 2022

View reviewed changes

carlaKC requested a review from ellemouton January 5, 2022 08:46

ellemouton reviewed Jan 12, 2022

View reviewed changes

qustavo force-pushed the wallet_key_count branch from 40111bc to b2c00ea Compare January 12, 2022 13:26

qustavo added 2 commits January 12, 2022 14:39

mod: bump lndclient to v0.13.0-10

f56e1cf

Signed-off-by: Gustavo Chain <[email protected]>

wallet: add keyCount metric

b81c950

Signed-off-by: Gustavo Chain <[email protected]>

qustavo force-pushed the wallet_key_count branch from b2c00ea to b81c950 Compare January 12, 2022 13:39

qustavo requested a review from ellemouton January 12, 2022 13:40

ellemouton approved these changes Jan 12, 2022

View reviewed changes

carlaKC merged commit a1fa10f into lightninglabs:master Jan 12, 2022

qustavo deleted the wallet_key_count branch January 12, 2022 14:31

		// Nodes prior to version 0.13.0 don't implement ListAccounts and will
		// return a Unimplemented error.

Add wallet_key_count metric #76

Add wallet_key_count metric #76

Uh oh!

Conversation

qustavo commented Oct 6, 2021

Uh oh!

carlaKC left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qustavo Oct 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

qustavo commented Oct 25, 2021

Uh oh!

guggero commented Oct 25, 2021

Uh oh!

qustavo commented Oct 26, 2021

Uh oh!

carlaKC left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlaKC left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlaKC commented Nov 5, 2021

Uh oh!

qustavo commented Nov 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

carlaKC commented Dec 13, 2021

Uh oh!

carlaKC left a comment

Choose a reason for hiding this comment

Uh oh!

qustavo commented Dec 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

qustavo commented Dec 22, 2021

Uh oh!

carlaKC commented Jan 4, 2022

Uh oh!

qustavo Oct 26, 2021 •

edited

Loading

qustavo commented Nov 9, 2021 •

edited

Loading

qustavo commented Dec 22, 2021 •

edited

Loading