Skip to content
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

Fix MR state metrics when MRs are deleted #688

Merged
merged 1 commit into from
May 9, 2024

Conversation

ezgidemirel
Copy link
Member

@ezgidemirel ezgidemirel commented May 2, 2024

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:

  • Read and followed Crossplane's contribution process.
  • 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 ezgidemirel requested review from a team as code owners May 2, 2024 12:32
Copy link
Member

@mergenci mergenci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @ezgidemirel 🙏 LGTM.

pkg/statemetrics/mr_state_metrics.go Outdated Show resolved Hide resolved
pkg/statemetrics/mr_state_metrics.go Outdated Show resolved Hide resolved
@negz
Copy link
Member

negz commented May 7, 2024

@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?

@ezgidemirel
Copy link
Member Author

  • 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.

@negz negz merged commit b31be77 into crossplane:master May 9, 2024
9 checks passed
@jbw976
Copy link
Member

jbw976 commented May 10, 2024

@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?

@ezgidemirel
Copy link
Member Author

@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.

Copy link

Successfully created backport PR for release-1.16:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants