-
Notifications
You must be signed in to change notification settings - Fork 757
Expose classification metrics from LowNodeUtilization plugin #1740
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
base: master
Are you sure you want to change the base?
Conversation
|
Hi @tiraboschi. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
metrics/metrics.go
Outdated
| LoopDuration, | ||
| StrategyDuration, | ||
| LowNodeUtilizationThresholds, | ||
| LowNodeUtilizationClassification, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will not scale well for external plugins. Maybe a combination of init() and metrics registration within each plugin? Or, have you tried to invoke RegisterMetrics within the plugin instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, I tried adding an optional Metrics interface to complement Handle without breaking backward compatibility
| for key, rt := range thresholds { | ||
| for i, t := range rt { | ||
| for resource, value := range t { | ||
| metrics.LowNodeUtilizationThresholds.WithLabelValues(key, strconv.Itoa(i), resource.String()).Set(float64(value)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There could be a new handler for setting metrics. E.g. l.handle.Metric(metricsName).WithLabelValues or similar that would be a no-op in case metrics are not enabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, I implemented a new metric handler wrapping all the metric setters embedding there the skip metric setting.
1874b7c to
40f17fe
Compare
40f17fe to
02b40c7
Compare
|
|
||
| // RegisterMetrics registers the plugin's metrics | ||
| func (l *LowNodeUtilization) RegisterMetrics() { | ||
| RegisterMetrics(metrics.PluginRegistry) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's probably no way to disallow users to invoke this in the NewXXX functions. Also, the descheduler registers the metrics independent of whether metrics server is enabled or not. I wonder what would be the overhead of just registering the metrics directly. Allowing to skip ShouldExport logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, a single plugin can be initialized in each profile. Thus, a metric entry needs to be labeled with a profile name as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a bit of logic to register metrics by profile name and by plugin name.
A metric named Classification registered by LowNodeUtilization plugin required by CustomProfile profile will result in descheduler_custom_profile_low_node_utilization_classification.
02b40c7 to
0603223
Compare
Enhance the plugin interface introducing an additional Metrics interface to be easily implemented by plugins that intend to register and expose metrics. Expose two new metrics from the LowNodeUtilization plugin: - low_node_utilization_thresholds - low_node_utilization_classification These metrics allow users to properly monitor plugin behavior, which was previously only visible in descheduler logs. Signed-off-by: Simone Tiraboschi <[email protected]>
0603223 to
5b89ac2
Compare
Enhance the plugin interface introducing an additional
Metricsinterface to be easily implemented by pluginsthat intend to register and expose metrics.
Expose two new metrics from the
LowNodeUtilizationplugin:descheduler_low_node_utilization_thresholdsdescheduler_low_node_utilization_classificationThese metrics allow users to properly monitor plugin behavior,
which was previously only visible in descheduler logs.