Add ClusterSyncFailingSeconds metric #1809
Conversation
- Add gauge "hive_clustersync_failing_seconds" that logs time since the lastTransitionTime of the clusterSync's Failed condition with the label namespaced_name as $namespace/$name - Make it optional and log it conditionally if the time since it has been failing is more than the threshold mentioned - Skip reporting for Unreachable clusters - Clear the metric when the clusterSync succeeds - Clear the metric when the clusterSync is deleted - Clear the metric when the clusterDeplyment is deleted
openshift-ci
bot
added
the
approved
Codecov Report
@@ Coverage Diff @@
## master #1809 +/- ##
==========================================
+ Coverage 41.72% 41.74% +0.02%
==========================================
Files 353 353
Lines 33033 33079 +46
==========================================
+ Hits 13782 13810 +28
- Misses 18097 18113 +16
- Partials 1154 1156 +2
|
There was a problem hiding this comment.
I have looked at this.
The code to declare and observe the new metric looks good ✓
However, I'm leery of the refactor, especially in such a foundational controller, especially since it does change some logic flow/order... and I don't think we actually need to do it. IIUC it's mainly for the purpose of being able to pass the clustersync around; and we only need to do that so we can scrub the metric for it; and the only information we need to do that is the namespace and name. So let's instead take advantage of the fact that the clustersync namespace and name are identical to that of the CD (ref -- we take advantage of this elsewhere too, so it's not likely to change) and not even bother passing the clustersync around.
| // In case the clusterSync has been deleted at this point, clear the clusterSyncFailing metric | ||
| clustersync.ClearClusterSyncFailingSecondsMetric(clusterSync, cdLog) | ||
| } else { | ||
| cdLog.WithError(err).Log(controllerutils.LogLevel(err), "could not get ClusterSync") |
There was a problem hiding this comment.
By not erroring here, we're passing an empty clustersync around. Hopefully the worst that will happen is we'll clear a metric with an empty label, which should be a no-op, but it's not future-proof.
Since the clustersync name and namespace are the same as clusterdeployment name and namespace, utilize that logic to eliminate unnecessary fetching of clustersync
This makes way more sense - fixed |
| // ShouldLogHistogramDurationMetric decides whether the corresponding duration metric of type histogram should be logged. | ||
| // It first checks if that optional metric has been opted into for logging, and the duration has crossed the threshold set. | ||
| func ShouldLogHistogramDurationMetric(metricToLog *prometheus.HistogramVec, timeToLog float64) bool { | ||
| duration, ok := mapMetricToDurationHistograms[metricToLog] | ||
| return ok && duration.Seconds() <= timeToLog | ||
| } | ||
|
|
||
| // ShouldLogGaugeDurationMetric decides whether the corresponding duration metric of type gauge should be logged. It | ||
| // first checks if that optional metric has been opted into for logging, and the duration has crossed the threshold set. | ||
| func ShouldLogGaugeDurationMetric(metricToLog *prometheus.GaugeVec, timeToLog float64) bool { | ||
| duration, ok := mapMetricToDurationGauges[metricToLog] | ||
| return ok && duration.Seconds() <= timeToLog | ||
| } |
There was a problem hiding this comment.
NB: I think we'll be able to collapse the maps and these funcs using generics once #1803 lands :)
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 2uasimojo, suhanime 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 |
|
@suhanime: all tests passed! Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
xref: https://issues.redhat.com/browse/HIVE-1857
/assign @2uasimojo