Re: [pdm-devel] [PATCH proxmox-datacenter-manager v2 00/28] metric collection improvements (concurrency, config, API, CLI)

[Maximiliano Sandoval <m.sandoval@proxmox.com>]

I went through the series and it looks good to me.

Reviewed-by: Maximiliano Sandoval <m.sandoval@proxmox.com>

Lukas Wagner <l.wagner@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
>


_______________________________________________
pdm-devel mailing list
pdm-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pdm-devel