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.

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

Wallet Metric #768

Merged
merged 7 commits into from
Nov 30, 2023
Merged

Wallet Metric #768

merged 7 commits into from
Nov 30, 2023

Conversation

peterjan
Copy link
Member

@peterjan peterjan commented Nov 28, 2023

This PR adds a WalletMetric so we can easily fetch the evolution of the balance over time. This would prevent the wallet page from having to load all transactions (80mb) to accomplish the same. This would allow @alexfreska to fetch the latest transactions (using limit=100) and create the balance graph using the metrics endpoint. This is consistent with hostd

	WalletMetric struct {
		Timestamp time.Time `json:"timestamp"`

		Address types.Address `json:"address"`

		Confirmed   types.Currency `json:"confirmed"`
		Spendable   types.Currency `json:"spendable"`
		Unconfirmed types.Currency `json:"unconfirmed"`
	}

@peterjan peterjan self-assigned this Nov 28, 2023
@peterjan peterjan marked this pull request as ready for review November 28, 2023 15:02
api/metrcis.go Outdated Show resolved Hide resolved
api/metrcis.go Outdated
WalletMetric struct {
Timestamp time.Time `json:"timestamp"`

Address types.Address `json:"address"`
Copy link
Member

Choose a reason for hiding this comment

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

I think the address isn't very useful considering it never changes and is not something that you can visualise in a meaningful way.

Copy link
Member Author

@peterjan peterjan Nov 29, 2023

Choose a reason for hiding this comment

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

I was on the fence when I wrote it. I figured it couldn't hurt but removed it. It wasn't about ever needing to visualise multiple wallets or handle multiple addresses, it was more to ensure we never visualise anything we know not be related to "our wallet".

stores/migrations_metrics.go Outdated Show resolved Hide resolved
Unconfirmed types.Currency `json:"unconfirmed"`
}

WalletMetricsQueryOpts struct{}
Copy link
Member Author

Choose a reason for hiding this comment

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

kept this for consistency and extensibility

Copy link
Member

@ChrisSchinnerl ChrisSchinnerl left a comment

Choose a reason for hiding this comment

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

lgtm but could still use an integration test to verify that we register metrics

@peterjan peterjan merged commit e1efe33 into master Nov 30, 2023
6 checks passed
@peterjan peterjan deleted the pj/wallet-metric branch November 30, 2023 11:24
@peterjan
Copy link
Member Author

lgtm but could still use an integration test to verify that we register metrics

I actually added it and then removed it, I also tested it manually on my node. I'll add an integration test in a F/U PR.

@peterjan peterjan mentioned this pull request Nov 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants