[pdm-devel] [PATCH proxmox-datacenter-manager v2 00/28] metric collection improvements (concurrency, config, API, CLI)
Maximiliano Sandoval
m.sandoval at proxmox.com
Fri Feb 21 14:19:36 CET 2025
I went through the series and it looks good to me.
Reviewed-by: Maximiliano Sandoval <m.sandoval at proxmox.com>
Lukas Wagner <l.wagner at proxmox.com> writes:
> Key points:
> - fetch metrics concurrently
> - configuration for metric collection
> - new config /etc/proxmox-datacenter-manager/metric-collection.json
> - max-concurrency (number of allowed parallel connections)
> - collection-interval
> - randomized offset for collection start
> (min-interval-offset..max-interval-offset)
> - randomized per-connection delay
> (max-connection-delay..max-connection-delay)
> - Add some tests for the core logic in the metric collection system
> - Allow to trigger metric collection via the API
> - Record metric collection statistics in the RRD
> - overall collection time for all remotes
> - per remote response time when fetching metrics
> - Persist metric collection state to disk:
> /var/lib/proxmox-datacenter-manager/metric-collection-state.json
> (timestamps of last collection, errors)
> - Trigger metric collection for any new remotes added via the API
>
> - Add new API endpoints
> POST /metric-collection/trigger with optional 'remote' param
> GET /metric-collection/status
> GET/PUT /config/metric-collection/default
> GET /remotes/<remote>/metric-collection-rrddata
> GET /metric-collection/rrddata
>
> - Add CLI tooling
> proxmox-datacenter-client metric-collection settings show
> proxmox-datacenter-client metric-collection settings update
> proxmox-datacenter-client metric-collection trigger [--remote <remote>]
> proxmox-datacenter-client metric-collection status
>
>
> ## To reviewers / open questions:
> - Please review the defaults I've chosen for the settings, especially
> the ones for the default metric collection interval (10 minutes) as
> well as max-concurrency (10).
> I also kindly ask to double-check the naming of the properties.
> See "pdm-api-types: add CollectionSettings type" for details
>
> - Please review path and params for new API endpoints (anything public
> facing that is hard to change later)
>
> - I've chosen a section-config config now, even though we only have a
> single section for now. This was done for future-proofing reasons,
> maybe we want to add support for different setting 'groups' or
> something, e.g. to have different settings for distinct sets of
> remotes. Does this make sense?
> Or should I just stick to a simple config for now? (At moments like
> these I wish for TOML configs where we could be a bit more flexible...)
>
> collection-settings: default
> max-concurrency 10
> collection-interval 180
> min-interval-offset 0
> max-interval-offset 20
> min-connection-delay 10
> max-connection-delay 100
>
>
> - Should `GET /remotes/<remote>/metric-collection-rrddata` be
> just `rrddata`?
> not sure if we are going to add any other PDM-native per-remote
> metrics and whether we want to return that from the same API call
> as this...
>
> ## Potential future work
> - UI button for triggering metric collection
> - UI for metric collection settings
> - Show RRD graphs for metric collection stats somewhere
>
> ## Random offset/delay examples
> Example with 'max-concurrency' = 3 and 6 remotes.
>
> X ... timer triggered
> [ A ] .... fetching remote 'A'
> **** .... interval-offset (usually a couple of seconds)
> #### .... random worker delay (usually in millisecond range)
>
> /--########[ B ] ### [ C ]--\
> /---####[ A ] ###### [ D ]--------\
> ----X ************* ---/ ---###### [ E ] #########[ F ]--\----
>
> Changes since [v1]:
> - add missing dependency to librust-rand-dev to d/control
> - Fix a couple of minor spelling/punctuation issues (thx maximiliano)
> - Some minor code style improvments, e.g. using unwrap_or_else instead of doing
> a manual match
> - Document return values of 'setup_timer' function
> - Factor out handle_tick/handle_control_message
> - Minor refatoring/code style improvments
> - CLI: Change 'update-settings' to 'settings update'
> - CLI: Change 'show-settings' to 'settings show'
> - change missed tick behavior for tokio::time::Interval to 'skip' instead
> of burst.
>
> The last three commits are new in v2.
>
> [v1]: https://lore.proxmox.com/pdm-devel/20250211120541.163621-1-l.wagner@proxmox.com/T/#t
>
More information about the pdm-devel
mailing list