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

Add a toggle to disable the HTLC monitor metrics #99

Merged
merged 1 commit into from
Apr 18, 2023

Conversation

reynico
Copy link
Contributor

@reynico reynico commented Apr 16, 2023

Over 76k entries are registered for lnd_htlcs_resolved_htlcs on the Prometheus endpoint in less than a week, leading to huge amounts of data being consumed by the Prometheus server (around 10MB per request).

% cat metrics | sort | uniq -c | sort -nr | head -10
65926 lnd_htlcs_resolved_htlcs
8404 lnd_htlcs_resolution_time_bucket
 764 lnd_htlcs_resolution_time_sum
 764 lnd_htlcs_resolution_time_count
  41 lnd_channels_updates_count
  41 lnd_channels_unsettled_balance
  41 lnd_channels_sent_sat
  41 lnd_channels_received_sat
  41 lnd_channels_pending_htlc_count
  41 lnd_channels_fee_per_kw

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.

Thanks for the flag, makes sense to me.

config.go Outdated
@@ -51,6 +51,9 @@ type config struct {

// DisableGraph disables collection of graph metrics
DisableGraph bool `long:"disablegraph" description:"Do not collect graph metrics"`

// DisableHtlc disables collection of HTLCs metrics
Copy link
Member

Choose a reason for hiding this comment

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

nit: Bad example on the comment above, but we prefer full sentences for comments, including punctuation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted! 497ed32

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I mostly meant the missing full stop 😅 Feel free to correct that for the existing comment above as well.
And can you please squash the fixup commit with the one that added the line? And usually we prefix the commits with the package that is changed (though not super strictly enforced in this repo): https://github.com/lightningnetwork/lnd/blob/master/docs/code_contribution_guidelines.md

collectors/prometheus.go Show resolved Hide resolved
@reynico reynico force-pushed the htlc-disable-metrics-toggle branch 2 times, most recently from d76042d to 875de5e Compare April 17, 2023 13:16
collectors/prometheus.go Show resolved Hide resolved
config.go Outdated Show resolved Hide resolved
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.

Needs another squash of commits, then this is good to go. Thanks!

@reynico
Copy link
Contributor Author

reynico commented Apr 17, 2023

Needs another squash of commits, then this is good to go. Thanks!

Awesome! want me to squash the two collectors commits?

@guggero
Copy link
Member

guggero commented Apr 17, 2023

Yes, I think everything should go into a single commit.

multi: add a toggle to disable the HTLC monitor collector
@reynico reynico force-pushed the htlc-disable-metrics-toggle branch from 3ec0cf2 to a03710f Compare April 17, 2023 14:53
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.

LGTM 🌊

@Roasbeef Roasbeef merged commit 2d4e987 into lightninglabs:master Apr 18, 2023
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.

3 participants