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

lndmon: cache closedChannels response #104

Merged
merged 1 commit into from
Jan 31, 2024

Conversation

djkazic
Copy link
Contributor

@djkazic djkazic commented Jan 24, 2024

Currently we call closedChannels once per scrape, this can be fairly expensive for LND's grpcServer. This PR will add:

  • a cache map that we populate with a ticker that fires per 10m
  • a warmup call during startup that sets the initial value of closedChannels
  • modifications to return the cache map contents instead of calling the closedChannels GRPC endpoint

@djkazic djkazic requested a review from Roasbeef January 24, 2024 13:35
Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

We should also cache ListSweeps as well (see other items in that issue too).

collectors/channels_collector.go Outdated Show resolved Hide resolved
collectors/channels_collector.go Outdated Show resolved Hide resolved
collectors/channels_collector.go Outdated Show resolved Hide resolved
collectors/channels_collector.go Outdated Show resolved Hide resolved
collectors/channels_collector.go Outdated Show resolved Hide resolved
collectors/channels_collector.go Show resolved Hide resolved
@djkazic djkazic force-pushed the feature/cached-grpc-responses branch from d52b3b7 to 7c5b272 Compare January 26, 2024 15:52
@djkazic
Copy link
Contributor Author

djkazic commented Jan 26, 2024

Squashed && force pushed @bhandras

@djkazic djkazic requested a review from bhandras January 26, 2024 15:52
collectors/prometheus.go Outdated Show resolved Hide resolved
@djkazic djkazic force-pushed the feature/cached-grpc-responses branch from 7c5b272 to 33fe8a3 Compare January 26, 2024 16:33
@djkazic
Copy link
Contributor Author

djkazic commented Jan 26, 2024

Amended && force pushed @bhandras

@djkazic djkazic requested a review from bhandras January 26, 2024 16:33
@djkazic djkazic force-pushed the feature/cached-grpc-responses branch from 33fe8a3 to 47a7fc5 Compare January 26, 2024 16:44
@djkazic djkazic requested a review from bhandras January 26, 2024 16:45
@djkazic
Copy link
Contributor Author

djkazic commented Jan 26, 2024

Fixed quitChan setting @bhandras

Copy link
Member

@bhandras bhandras left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @djkazic 🎉

collectors/channels_collector.go Outdated Show resolved Hide resolved
collectors/channels_collector.go Outdated Show resolved Hide resolved
collectors/channels_collector.go Outdated Show resolved Hide resolved
@djkazic djkazic force-pushed the feature/cached-grpc-responses branch from 47a7fc5 to 7ec821a Compare January 26, 2024 18:46
@djkazic
Copy link
Contributor Author

djkazic commented Jan 26, 2024

Addressed nits @bhandras

@djkazic djkazic requested a review from guggero January 30, 2024 15:38
@djkazic
Copy link
Contributor Author

djkazic commented Jan 30, 2024

Deploying to testnet today.

Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

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

Nice optimization! Code level nits only.

collectors/channels_collector.go Outdated Show resolved Hide resolved
collectors/channels_collector.go Outdated Show resolved Hide resolved
collectors/channels_collector.go Outdated Show resolved Hide resolved

// Start a ticker to update the cache once per 10m
go func() {
ticker := time.NewTicker(10 * time.Minute)
Copy link
Member

Choose a reason for hiding this comment

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

Should we make this a package-level variable just so "magic numbers" are easier to find and change?

Copy link
Member

Choose a reason for hiding this comment

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

The 10 minute magic number is still here in the latest push.

collectors/channels_collector.go Outdated Show resolved Hide resolved
collectors/prometheus.go Outdated Show resolved Hide resolved
@djkazic djkazic force-pushed the feature/cached-grpc-responses branch from 7ec821a to 48670c8 Compare January 31, 2024 13:24
@djkazic
Copy link
Contributor Author

djkazic commented Jan 31, 2024

Addressed nits @guggero

@djkazic djkazic requested a review from guggero January 31, 2024 13:25
Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

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

Almost there.

collectors/channels_collector.go Outdated Show resolved Hide resolved
collectors/prometheus.go Outdated Show resolved Hide resolved

// Start a ticker to update the cache once per 10m
go func() {
ticker := time.NewTicker(10 * time.Minute)
Copy link
Member

Choose a reason for hiding this comment

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

The 10 minute magic number is still here in the latest push.

@djkazic djkazic force-pushed the feature/cached-grpc-responses branch from 48670c8 to 0a31bae Compare January 31, 2024 14:40
@djkazic
Copy link
Contributor Author

djkazic commented Jan 31, 2024

Did another pass @guggero

@djkazic djkazic requested a review from guggero January 31, 2024 14:40
@djkazic djkazic force-pushed the feature/cached-grpc-responses branch from 0a31bae to c8e69f9 Compare January 31, 2024 14:44
collectors/prometheus.go Outdated Show resolved Hide resolved
@djkazic djkazic force-pushed the feature/cached-grpc-responses branch from c8e69f9 to 662470e Compare January 31, 2024 15:43
@djkazic djkazic requested a review from guggero January 31, 2024 15:43
Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

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

Nice, LGTM 🎉

@Roasbeef Roasbeef merged commit 9d40b32 into master Jan 31, 2024
1 check passed
@Roasbeef
Copy link
Member

May also want to also cache PendingChannels as well, since that can also hit some historical buckets.

@guggero guggero deleted the feature/cached-grpc-responses branch February 1, 2024 07:52
bitodt added a commit to bitodt/lndmon that referenced this pull request Feb 4, 2024
…d-grpc-responses

lndmon: cache closedChannels response
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants