Introduce High Level MR metrics #683

ezgidemirel · 2024-04-03T13:26:32Z

Description of your changes

This pull request introduces the following high level metrics in the MR reconciler and cherry-picks the relevant commit in this PR.

crossplane_managed_resource_exists{"gvk"}
crossplane_managed_resource_ready{"gvk"}
crossplane_managed_resource_synced{"gvk"}
crossplane_managed_resource_first_time_to_reconcile_seconds{"gvk"}
crossplane_managed_resource_first_time_to_readiness_seconds{"gvk"}
crossplane_managed_resource_deletion_seconds{"gvk"}
crossplane_managed_resource_drift_seconds{"gvk"}

The crossplane_managed_resource_time_to_first_reconcile_seconds and crossplane_managed_resource_first_time_to_readiness_seconds metrics are pushed only once for each resource, capturing the initial reconciliation time after creation and the first instance the resource becomes ready, respectively. It's important to note that these metrics won't be recorded in the event of a pod restart. One potential solution to this issue is to introduce an additional status subresource, such as "observations," where these values can be stored persistently.

Example provider PR which consumes changes-> crossplane-contrib/provider-kubernetes#224

Fixes #

I have:

Read and followed Crossplane's contribution process.
Run make reviewable test to ensure this PR is ready for review.

Signed-off-by: Dr. Stefan Schimanski <[email protected]>

Signed-off-by: ezgidemirel <[email protected]>

pkg/reconciler/managed/metrics.go

Signed-off-by: ezgidemirel <[email protected]>

sttts · 2024-04-15T09:43:55Z

pkg/statemetrics/xr_state_metrics.go

+func (r *XRStateRecorder) Record(ctx context.Context, gvk schema.GroupVersionKind) {
+	l := &unstructured.UnstructuredList{}
+	l.SetGroupVersionKind(gvk)
+	err := r.client.List(ctx, l)


Don't list from a client. List from a cache, i.e. pass in a cache here. Listing can be expensive, imagine 10000 objects.

pkg/reconciler/managed/metrics.go

sttts · 2024-04-15T09:55:40Z

pkg/reconciler/managed/reconciler.go

 	}

 	for _, ro := range o {
 		ro(r)
 	}

+	// State recorder is started in the background to record MR states.
+	go r.stateRecorder.Run(context.Background(), schema.GroupVersionKind(of))


Never leak go routines from a constructor. This must be started outside with an availablectx.

I made state recorders Runnable and registered to manager to manage go routine's lifecycle with it.

sttts · 2024-04-15T09:58:28Z

pkg/statemetrics/mr_state_metrics.go

+func (r *MRStateRecorder) Record(ctx context.Context, gvk schema.GroupVersionKind) {
+	l := &unstructured.UnstructuredList{}
+	l.SetGroupVersionKind(gvk)
+	err := r.client.List(ctx, l)


also here: pass a cache please to make sure this does not hit the apiserver. This can be a very heavy operation, listing thousands of MRs.

Would it be sufficient to accept client.Reader and add a comment letting the caller know it must be cache-backed?

I don't feel strongly - mostly wondering if there's some way to do this up at the controller-runtime level of abstraction without moving down into client-go things. Would be nice, but not critical.

What do you think about passing the following option to the NewManager function in provider's main.go?

mgr, err := ctrl.NewManager(ratelimiter.LimitRESTConfig(cfg, *maxReconcileRate), ctrl.Options{ Client: client.Options{Cache: &client.CacheOptions{Unstructured: true}}, ... })

This makes shouldByPassCache method to return false in the List call.

I no longer use unstructured list, so we'll use client's cache.

pkg/statemetrics/mr_state_metrics.go

pkg/statemetrics/xr_state_metrics.go

pkg/statemetrics/mr_state_metrics.go

pkg/statemetrics/xr_state_metrics.go

negz · 2024-04-15T19:49:37Z

Thanks @ezgidemirel! Still needs some work per @sttts's comments but I like this direction much better.

Signed-off-by: ezgidemirel <[email protected]>

pkg/reconciler/managed/metrics.go

mergenci

"gvk" seems to be misplaced in these instances.

pkg/reconciler/managed/metrics.go

negz · 2024-04-18T04:12:51Z

@ezgidemirel Your PR description mentions some crossplane_composite_resource_ metrics, but I don't see where those are implemented. Are you planning for them to be part of this PR? I'd expect core Crossplane to emit these metrics.

ezgidemirel · 2024-04-18T04:15:20Z

@ezgidemirel Your PR description mentions some crossplane_composite_resource_ metrics, but I don't see where those are implemented. Are you planning for them to be part of this PR? I'd expect core Crossplane to emit these metrics.

I was planning to add the state metrics with this PR, but then removed with this commit. I'll update the PR description.

Signed-off-by: ezgidemirel <[email protected]>

mergenci · 2024-04-22T10:01:23Z

pkg/statemetrics/mr_state_metrics.go

+
+	mrs := mrList.GetItems()
+	if len(mrs) == 0 {
+		return nil


Issue: This early return seems to prevent setting the metric to zero when all resources are deleted.

I'm not sure what we can do at this stage. If we don't have any MRs left, we cannot send the value with the right gvk. I think it's ok to say that, within a time framed monitoring, we'll display the correct metrics. If there are no MRs with a specific GVK in the last 15 mins, users won't see any metrics for that GVK.

turkenh

Thanks @ezgidemirel 🙏

@negz I see all your comments were resolved and we need to get this in to be able ship this functionality with the upcoming provider releases (i.e. tomorrow), so, I'll merge it now. Feel free add more comments if there are still open points and we can get them resolved with a follow up PR.

@mergenci, similarly, feel free to open a follow up PR regarding your comment on early return.

eduardo-diaz-em · 2024-10-18T09:49:30Z

@ezgidemirel Your PR description mentions some crossplane_composite_resource_ metrics, but I don't see where those are implemented. Are you planning for them to be part of this PR? I'd expect core Crossplane to emit these metrics.

I was planning to add the state metrics with this PR, but then removed with this commit. I'll update the PR description.

Hi @ezgidemirel I am running crossplane on 1.17 and I still can't see the crossplane_managed_resources_* metrics (the rest are working fine). According to the docs they should be available, is this a bug or am I missing something? This is my crossplane configuration from ArgoCD:

apiVersion: argoproj.io/v1alpha1
kind: Application
metadata:
  name: crossplane
  namespace: argocd
spec:
  project: system-apps
  source:
    chart: crossplane
    repoURL: https://charts.crossplane.io/stable
    targetRevision: 1.17.0
    helm:
      values: |
        resourcesRBACManager:
          requests:
            cpu: 250m
            memory: 512Mi
          limits:
            cpu: 250m
            memory: 512Mi
        resourcesCrossplane:
          requests:
            cpu: 250m
            memory: 512Mi
          limits:
            cpu: 250m
            memory: 512Mi
        metrics:
          enabled: true
  destination:
    server: 'https://kubernetes.default.svc'
    namespace: crossplane
  syncPolicy:
    automated:
      prune: false
      selfHeal: false
    syncOptions:
      - ServerSideApply=true
      - CreateNamespace=true

mergenci · 2024-10-21T23:04:21Z

@eduardo-diaz-em, this is a crossplane-runtime PR. crossplane-runtime is used by Crossplane providers. Your provider version and which version of crossplane-runtime it uses is important, rather than your Crossplane version. For instance, provider-upjet-aws uses crossplane-runtime v1.16.0-rc.2.0.

I acknowledge that it would be helpful if the documentation clarified distinction between Crossplane and Crossplane provider metrics.

sttts and others added 2 commits April 2, 2024 17:18

reconciler/managed: add crossplane_resource_drift_seconds metric

b424793

Signed-off-by: Dr. Stefan Schimanski <[email protected]>

Add High Level MR metrics

64074c3

Signed-off-by: ezgidemirel <[email protected]>

ezgidemirel requested review from a team as code owners April 3, 2024 13:26

ezgidemirel requested review from bobh66 and ytsarev April 3, 2024 13:26

negz reviewed Apr 3, 2024

View reviewed changes

change init pattern and move state metrics to a go routine

af65888

Signed-off-by: ezgidemirel <[email protected]>