crossplane-runtime icon indicating copy to clipboard operation
crossplane-runtime copied to clipboard

Fix MR state metrics when MRs are deleted

Open ezgidemirel opened this issue 1 year ago • 1 comments

Description of your changes

In case of deleting all MRs from a GVK, we should record 0 for crossplane_managed_resource_exists, crossplane_managed_resource_ready and crossplane_managed_resource_synced metrics.

Fixes #

I have:

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

How has this code been tested

Tested with provider-kubernetes

Ezgis-MacBook-Pro:phase-2 ezgidemirel$ curl localhost:8080/metrics |grep managed_resource_exists
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 30165    0 30165    0     0  25.3M      0 --:--:-- --:--:-- --:--:-- 28.7M
# HELP crossplane_managed_resource_exists The number of managed resources that exist
# TYPE crossplane_managed_resource_exists gauge
crossplane_managed_resource_exists{gvk="kubernetes.crossplane.io/v1alpha2, Kind=Object"} 0

ezgidemirel avatar May 02 '24 12:05 ezgidemirel

@ezgidemirel Two other small nits I noticed that aren't related to this PR specifically. It's fine to address them separately:

  • MRStateRecorder takes a client.Client, but only reads. It could take a smaller interface - client.Reader.
  • MRStateRecorder is constructed with a managed.ResourceList, which it then passes as an argument to its own Record method every tick. Does Record need to take this as an argument? Would you ever expect Record to be called with a different managed.ResourceList from the one stored in the MRStateRecorder?

negz avatar May 07 '24 20:05 negz

  • MRStateRecorder takes a client.Client, but only reads. It could take a smaller interface - client.Reader.

To access the Schema I need client.Client now.

  • MRStateRecorder is constructed with a managed.ResourceList, which it then passes as an argument to its own Record method every tick. Does Record need to take this as an argument? Would you ever expect Record to be called with a different managed.ResourceList from the one stored in the MRStateRecorder?

Removed the parameter and accessing the struct field in the Record method.

ezgidemirel avatar May 09 '24 15:05 ezgidemirel

@ezgidemirel we already cut the release-1.16 branch, do you want to backport this PR so that it is included in the v1.16 release?

jbw976 avatar May 10 '24 09:05 jbw976

@ezgidemirel we already cut the release-1.16 branch, do you want to backport this PR so that it is included in the v1.16 release?

@jbw976 yes, we can backport it.

ezgidemirel avatar May 10 '24 09:05 ezgidemirel

Successfully created backport PR for release-1.16:

  • #691

github-actions[bot] avatar May 10 '24 09:05 github-actions[bot]