Skip to content
This repository has been archived by the owner on Apr 25, 2023. It is now read-only.

feat: add custom kubefed metrics #1196

Merged
merged 4 commits into from
Mar 26, 2020
Merged

feat: add custom kubefed metrics #1196

merged 4 commits into from
Mar 26, 2020

Conversation

hectorj2f
Copy link
Contributor

@hectorj2f hectorj2f commented Feb 25, 2020 •
edited
Loading

What this PR does / why we need it:

It adds kubefed custom metrics to the list of metrics exposed and serve by #1193. It is a follow-up PR that allows to have a better view of the duration of some of the complex kubefed operations.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

related to: #1194
Special notes for your reviewer:

Screen Shot 2020-03-15 at 4 00 44 PM

I created a home made grafana dashboard to testthese metrics:
Screen Shot 2020-03-15 at 4 39 43 PM

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 25, 2020
@hectorj2f hectorj2f mentioned this pull request Feb 25, 2020
Closed
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 25, 2020
@hectorj2f hectorj2f force-pushed the hectorj2f/add_custom_metrics branch 2 times, most recently from b68b143 to 89084be Compare February 25, 2020 17:41
jimmidyson
jimmidyson reviewed Feb 25, 2020
View reviewed changes
pkg/metrics/metrics.go Outdated Show resolved Hide resolved
pkg/metrics/metrics.go Outdated Show resolved Hide resolved
@hectorj2f hectorj2f mentioned this pull request Feb 25, 2020
Closed
6 tasks
@hectorj2f hectorj2f force-pushed the hectorj2f/add_custom_metrics branch from 89084be to 1f79230 Compare March 10, 2020 13:49
@hectorj2f
Copy link
Contributor Author

@jimmidyson could you review again this PR ? I used a grafana dashboard to validate the metrics on a running cluster to which I joined two clusters

@hectorj2f
Copy link
Contributor Author

/assign @jimmidyson

sreis
sreis reviewed Mar 23, 2020
View reviewed changes
Copy link
Contributor

@sreis sreis left a comment

Choose a reason for hiding this comment

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

If we add these metric as is can we change them to also track the errors in some of the operations like join and unjoin in follow up PR? Or would that imply major changes?

docs/keps/20200302-kubefed-metrics.md Outdated Show resolved Hide resolved
pkg/controller/kubefedcluster/controller.go Outdated
@@ -257,6 +260,9 @@ func (cc *ClusterController) updateIndividualClusterStatus(cluster *fedv1b1.Kube
if err := cc.client.UpdateStatus(context.TODO(), cluster); err != nil {
klog.Warningf("Failed to update the status of cluster %q: %v", cluster.Name, err)
}

metrics.ClusterHealthStatusDurationFromStart(clusterHealthStatusStart)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why didn't you use a defer like you did here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It wasn't intentional, I started using them in a way. I now adapted to run as a defer call

pkg/controller/util/federated_informer.go Outdated Show resolved Hide resolved
docs/keps/20200302-kubefed-metrics.md Outdated Show resolved Hide resolved
pkg/metrics/metrics.go Outdated Show resolved Hide resolved
@hectorj2f hectorj2f force-pushed the hectorj2f/add_custom_metrics branch 2 times, most recently from 35a230d to de0b73a Compare March 23, 2020 17:28
@hectorj2f hectorj2f force-pushed the hectorj2f/add_custom_metrics branch from de0b73a to e17725d Compare March 24, 2020 16:11
@pmorie
Copy link
Contributor

pmorie commented Mar 25, 2020

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hectorj2f, pmorie

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 25, 2020
@hectorj2f
Copy link
Contributor Author

Yes, @sreis we could easily adapt the code to add metrics to register the different type of errors.

@pmorie
Copy link
Contributor

pmorie commented Mar 26, 2020

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 26, 2020
@k8s-ci-robot k8s-ci-robot merged commit b11635e into kubernetes-retired:master Mar 26, 2020
@hectorj2f hectorj2f deleted the hectorj2f/add_custom_metrics branch March 26, 2020 23:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@sreis sreis sreis left review comments

@font font Awaiting requested review from font

@gyliu513 gyliu513 Awaiting requested review from gyliu513

@jimmidyson jimmidyson Awaiting requested review from jimmidyson

Assignees

@pmorie pmorie

@jimmidyson jimmidyson

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@hectorj2f @pmorie @k8s-ci-robot @jimmidyson @sreis