add ARO hosted_cluster_info metric and labels #6180
Conversation
Signed-off-by: Salvatore Dario Minonne <[email protected]>
openshift-ci
bot
added
the
area/hypershift-operator
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: sdminonne 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 |
Test Resultse2e-aws
e2e-aks
|
|
/retest-required |
|
@sdminonne: The following test failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
| @@ -92,6 +92,9 @@ const ( | |||
|
|
|||
| ClusterSizeOverrideMetricName = "hypershift_cluster_size_override_instances" | |||
| clusterSizeOverrideMetricHelp = "Number of HostedClusters with a cluster size override annotation" | |||
|
|
|||
| HostedClusterInfoMetricName = "hosted_cluster_info" | |||
There was a problem hiding this comment.
we might want 2 info metrics
- one
hosted_cluster_infometric for all generic labels (at least hosted cluster name, namespace, _id and maybe platform type). - one specific for Azure platform (
hosted_cluster_azure_info) when platform type = "Azure".
One suggestion I made somewhere else was that we could put everything into "hosted_cluster_info" but it's recommended to have consistent labels per metric. And we might get similar requests in the future for AWS, GCP, ....
There was a problem hiding this comment.
thanks @simonpasquier I'll discuss this with my team and let you know.
| append(hclusterLabels, "Microsoft_subscriptionId", | ||
| "Microsoft_resourceGroupName", | ||
| "Microsoft_resourceType", | ||
| "Microsoft_resourceId", | ||
| "Location"), nil) |
There was a problem hiding this comment.
(nit) the implicit convention is that label names are lower-case and words separated by underscores. If the metric becomes Azure specific, the "Microsoft_" prefix may be dropped.
| append(hclusterLabels, "Microsoft_subscriptionId", | |
| "Microsoft_resourceGroupName", | |
| "Microsoft_resourceType", | |
| "Microsoft_resourceId", | |
| "Location"), nil) | |
| append(hclusterLabels, "subscription_id", | |
| "resource_ group_name", | |
| "resource_type", | |
| "resource_id", | |
| "location"), nil) |
There was a problem hiding this comment.
Note taken, if we'll split the metric we may want to get rid of the prefix
| azInfo := hcluster.Spec.Platform.Azure | ||
| subID := azInfo.SubscriptionID | ||
| resGroup := azInfo.ResourceGroupName | ||
| resourceID := fmt.Sprintf("/subscriptions/%s/resourceGroups/%s/providers/Microsoft.RedHatOpenshift/hcpOpenShiftClusters/%s", |
There was a problem hiding this comment.
Does it make sense to have providers/Microsoft.RedHatOpenshift/hcpOpenShiftClusters? What about HyperShift deployments in Azure that aren't ARO?
There was a problem hiding this comment.
Unfortunately I can't answer. I got this string from @geoberle and I don't have a real background. Need to collect more info. Ty!
What this PR does / why we need it:
This PR adds a new metric
hosted_cluster_info. It adds new labels:Fixes #CNTRLPLANE-902
Checklist