[Merged by Bors] - Configurable monitoring endpoint frequency #3530
Conversation
michaelsproul
added
ready-for-review
|
Please add some documentation, I searched the "Lighthouse Book" and all I could find was this and that gives no details as to what metric information nor the frequency. When I say "what metric information" the link would be real helpful in that documentation. But some additional verbiage on "exactly" what subset lighthouse is providing when is needed. Also, it would be nice to clarify when to use '--validator-monitor-auto For example, when using Hopefully, this makes sense :) |
|
@winksaville The The validator monitor is a Lighthouse specific feature that provides info about specified validators. The documentation we have here https://lighthouse-book.sigmaprime.io/validator-monitoring.html is for that feature. Happy to add some docs linking to the beaconcha.in service and clarifying that it is different from the lighthouse validator monitor if that would reduce confusion :)
This was done intentionally as we also have setups where the bn and vc are on separate machines. We could add a flag to the vc to prevent redundant sending of system metrics. |
|
Added some docs that cover all the things I think you wanted @winksaville 🙏 https://github.com/sigp/lighthouse/blob/d3788e2937da0173039c0c065ea46101be62adb2/book/src/advanced_metrics.md#remote-monitoring |
| .long("monitoring-endpoint-period") | ||
| .value_name("SECONDS") | ||
| .help("Defines how many seconds to wait between each message sent to \ | ||
| the monitoring-endpoint. Default: 60s") |
There was a problem hiding this comment.
How about something like:
.help(format("Defines how many seconds to wait between each message sent to the monitoring-endpoint. Default: {}", monitoring_api::DEFAULT_UPDATE_DURATION))
There was a problem hiding this comment.
We can't easily put a dynamic value here. The problem is that clap requires the arguments to live as long as the entire App struct, and a format!() string only lives as long the scope in which it's defined. We could plumb the dynamic values around (unwieldy) or do what I was doing before where we leak the string. Leaking the string is quite a nasty hack with a small runtime downside, so I'd prefer to just hardcode this and risk it going out of date. Sometimes worse is better.
I'm quite disappointed in clap for this and other reasons, so I'd support a PR in future to move us away from it completely. We're likely to overhaul all our CLI parsing when adding config file support (#3079), and can decide whether to keep clap and leak strings, or migrate to something better.
There was a problem hiding this comment.
Add comment to update this, string. maybe something like:
the monitoring-endpoint. Default: 60s") // Update when DEFAULT_UPDATE_DURATION changes
| @@ -16,7 +16,7 @@ use types::*; | |||
| pub use types::ProcessType; | |||
|
|
|||
| /// Duration after which we collect and send metrics to remote endpoint. | |||
| pub const UPDATE_DURATION: u64 = 60; | |||
| pub const DEFAULT_UPDATE_DURATION: u64 = 60; | |||
There was a problem hiding this comment.
Add comment something like:
pub const DEFAULT_UPDATE_DURATION: u64 = 60; // Search for this and update hard-coded strings.
| .long("monitoring-endpoint-period") | ||
| .value_name("SECONDS") | ||
| .help("Defines how many seconds to wait between each message sent to \ | ||
| the monitoring-endpoint. Default: 60s") |
There was a problem hiding this comment.
Add comment to update this, string. maybe something like:
the monitoring-endpoint. Default: 60s") // Update when DEFAULT_UPDATE_DURATION changes
|
bors r+ |
## Issue Addressed Closes #3514 ## Proposed Changes - Change default monitoring endpoint frequency to 120 seconds to fit with 30k requests/month limit. - Allow configuration of the monitoring endpoint frequency using `--monitoring-endpoint-frequency N` where `N` is a value in seconds.
bors
bot
changed the title
## Issue Addressed Closes sigp#3514 ## Proposed Changes - Change default monitoring endpoint frequency to 120 seconds to fit with 30k requests/month limit. - Allow configuration of the monitoring endpoint frequency using `--monitoring-endpoint-frequency N` where `N` is a value in seconds.
Issue Addressed
Closes #3514
Proposed Changes
--monitoring-endpoint-frequency NwhereNis a value in seconds.