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

Record suspension metrics #123

Merged
merged 1 commit into from
Mar 17, 2021
Merged

Conversation

somtochiama
Copy link
Member

@somtochiama somtochiama commented Mar 17, 2021

Part of fluxcd/flux2#590
Signed-off-by: Somtochi Onyekwere somtochionyekwere@gmail.com

Signed-off-by: Somtochi Onyekwere <somtochionyekwere@gmail.com>
@@ -411,3 +413,22 @@ func (r *ImageRepositoryReconciler) recordReadinessMetric(ctx context.Context, r
}, !repo.DeletionTimestamp.IsZero())
}
}

func (r *ImageRepositoryReconciler) recordSuspension(ctx context.Context, imageRepo imagev1alpha1.ImageRepository) {
Copy link
Member

@squaremo squaremo Mar 17, 2021

Choose a reason for hiding this comment

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

This looks like it depends very little on imagev1alpha1.ImageRepository -- can it be factored out into fluxcd/pkg, with a minimal interface as its argument?

Copy link
Member

Choose a reason for hiding this comment

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

The runtime/metrics package deals with object references, what factored out do you have in mind?

Copy link
Member

Choose a reason for hiding this comment

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

This method is near identical to https://github.com/fluxcd/helm-controller/pull/236/files#diff-4a892aba8c8073dd54b0ecdb7493670d3b0486a078d02a83efa0cecc09e09b67R737, https://github.com/fluxcd/notification-controller/pull/164/files#diff-c2f1e669308ef8c3c3b1083764bb88a89f3de34b885d30edeae65d1d53a6136aR115, https://github.com/fluxcd/kustomize-controller/pull/299/files#diff-5d1fa9b5557ebd96e4511008d93cb5cc21084d81f6baac4d1ebbbfd28a69eba0R834, etc. The only way in which it differs is the type of the receiver (from which only the metrics recorder and scheme are actually used, and those are common to all the controllers), and the API type, which is only required to have a .Spec.Suspend field, implement whatever interface reference.GetReference() accepts, and have a deletion timestamp (which is already available via GetDeletionTimestamp() in metav1.ObjectMeta, which is embedded by all the API types).

Copy link
Member

Choose a reason for hiding this comment

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

I wander if we could factored out the readiness recorder as well, the Ready condition is the same for all our custom resources.

Copy link
Member

@squaremo squaremo Mar 17, 2021

Choose a reason for hiding this comment

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

I reckon so -- I made an attempt in fluxcd/pkg#56 but didn't line things up quite right, and was trying to do too much at once. A decent start would be simply to provide funcs RecordSuspension and RecordReadiness; a stretch might be to make these methods of a struct (possibly MetricsRecorder).

Copy link
Member

Choose a reason for hiding this comment

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

wdyt @somtochiama, are you up for this refactor?

Copy link
Member

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks @somtochiama 🌷

@stefanprodan stefanprodan merged commit e164cfc into fluxcd:main Mar 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants