Add syncer in error state metrics #2156

sawsa307 · 2023-05-30T17:55:04Z

Add syncer in error state metrics

k8s-ci-robot · 2023-05-30T17:55:13Z

Hi @sawsa307. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

sawsa307 · 2023-05-30T17:55:14Z

/assign @swetharepakula

gauravkghildiyal · 2023-05-30T18:38:01Z

pkg/neg/metrics/syncer_metrics.go

+	syncerInErrorState = prometheus.NewGaugeVec(
+		prometheus.GaugeOpts{
+			Subsystem: negControllerSubsystem,
+			Name:      "syncer_in_error_tate",


typo

By the way, if the metric name is still up for discussion, I'd like to suggest we improve the name to plural form (since this is counting something)

"syncers_in_error_state"

Updated. Thanks!

gauravkghildiyal · 2023-05-30T18:45:30Z

pkg/neg/metrics/syncer_metrics.go

@@ -64,6 +67,15 @@ var (
 		},
 		[]string{"state"},
 	)
+
+	syncerInErrorState = prometheus.NewGaugeVec(


Is there some context on why we didn't want to use the other metric which ALSO counts the number of syncers?

syncerState

In syncerState, the state of a syncer is dependent on the result of last sync, so if a syncer is in error-state, its state will be success after degraded mode intervention. It means the syncer has successfully added endpoints to its NEG, which indicates our degraded mode procedures are working as we expected.
For syncerInErrorState, we want to track if a syncer is consistently in error-state, and this gives us an indication of whether there is bug in dependent system(Arcus API, kubelet, EPS controller and etc.)

sawsa307 · 2023-05-30T19:08:53Z

/assign @gauravkghildiyal

swetharepakula · 2023-05-30T22:59:48Z

pkg/neg/metrics/metricscollector/metrics.go

@@ -65,6 +69,16 @@ var (
 		[]string{"state"},
 	)


should we add an error state label key and add it to this metric? It also matches how you are tracking the information.

I agree with Gaurav, it is confusing to have two similar metrics

swetharepakula · 2023-05-31T16:40:04Z

/ok-to-test

sawsa307 · 2023-05-31T17:26:38Z

/retest

sawsa307 · 2023-05-31T18:30:10Z

/retest

swetharepakula · 2023-05-31T22:31:29Z

pkg/neg/metrics/metricscollector/metrics.go

@@ -62,7 +62,7 @@ var (
 			Name:      "syncer_state",
 			Help:      "Current count of syncers in each state",
 		},
-		[]string{"state"},
+		[]string{"state", "in_error_state"},


lets change this to degraded mode

Discussed offline.

swetharepakula · 2023-05-31T22:31:51Z

pkg/neg/metrics/metricscollector/metrics_collector.go

+func PublishSyncerStateMetrics(stateCount syncerStateCount) {
+	for state, count := range stateCount {
+		if state.inErrorState {
+			syncerState.WithLabelValues(string(state.state), "inErrorState").Set(float64(count))


Change this to DegradedModeEnabled and DegradedModeDisabled

Discussed offline. Now we would have inErrorState as label, "true" and "false" as values.

swetharepakula · 2023-05-31T22:41:18Z

pkg/neg/metrics/metricscollector/metrics_collector.go

 	syncerStateMap map[negtypes.NegSyncerKey]negtypes.Reason
+	// syncerErrorStateMap tracks if each syncer is in error-state
+	syncerErrorStateMap map[negtypes.NegSyncerKey]bool


combine these into a single map. And instead of storing just the reason or the bool store it with the stateWithErrorState struct

Updated. Thanks!

swetharepakula · 2023-05-31T22:44:28Z

pkg/neg/metrics/metricscollector/metrics.go

+type stateWithErrorState struct {
+	state        negtypes.Reason
+	inErrorState bool
 }


Rename the struct to syncerState, and possibly change the state field to lastSyncResult?

Updated. Thanks!

sawsa307 · 2023-05-31T23:43:01Z

pkg/neg/metrics/metricscollector/metrics.go

-	// syncerState tracks the count of syncer in different states
-	syncerState = prometheus.NewGaugeVec(
+	// SyncerCountBySyncResult tracks the count of syncer in different states
+	SyncerCountBySyncResult = prometheus.NewGaugeVec(


We already have a SyncerCountByEndpointType metrics. Add suffix for clarification.

Add syncer in error state metrics

swetharepakula

/lgtm
/approve

k8s-ci-robot · 2023-05-31T23:50:48Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sawsa307, swetharepakula

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:

~~OWNERS~~ [swetharepakula]

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

sawsa307 · 2023-06-01T06:13:42Z

/retest

sawsa307 · 2023-06-01T14:48:29Z

/retest

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 30, 2023

k8s-ci-robot requested review from code-elinka and gauravkghildiyal May 30, 2023 17:55

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 30, 2023

k8s-ci-robot assigned swetharepakula May 30, 2023

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 30, 2023

gauravkghildiyal reviewed May 30, 2023

View reviewed changes

sawsa307 requested a review from gauravkghildiyal May 30, 2023 19:08

k8s-ci-robot assigned gauravkghildiyal May 30, 2023

sawsa307 force-pushed the add-in-error-state-metrics branch 2 times, most recently from a63097b to c5562ee Compare May 30, 2023 22:54

swetharepakula reviewed May 30, 2023

View reviewed changes

sawsa307 force-pushed the add-in-error-state-metrics branch from c5562ee to 149530c Compare May 30, 2023 23:53

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 30, 2023

sawsa307 requested a review from swetharepakula May 30, 2023 23:57

sawsa307 force-pushed the add-in-error-state-metrics branch from 149530c to f2d8231 Compare May 31, 2023 00:08

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 31, 2023

swetharepakula reviewed May 31, 2023

View reviewed changes

sawsa307 force-pushed the add-in-error-state-metrics branch from f2d8231 to d21a4f3 Compare May 31, 2023 23:42

sawsa307 commented May 31, 2023

View reviewed changes

Add syncer in error state metrics

dbd55aa

Add syncer in error state metrics

sawsa307 force-pushed the add-in-error-state-metrics branch from d21a4f3 to dbd55aa Compare May 31, 2023 23:49

swetharepakula reviewed May 31, 2023

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 31, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 31, 2023

k8s-ci-robot merged commit 6b516dd into kubernetes:master Jun 1, 2023

sawsa307 deleted the add-in-error-state-metrics branch September 2, 2023 20:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add syncer in error state metrics #2156

Add syncer in error state metrics #2156

sawsa307 commented May 30, 2023

k8s-ci-robot commented May 30, 2023

sawsa307 commented May 30, 2023

gauravkghildiyal May 30, 2023

sawsa307 May 30, 2023

gauravkghildiyal May 30, 2023

sawsa307 May 30, 2023 •

edited

Loading

sawsa307 commented May 30, 2023

swetharepakula May 30, 2023

swetharepakula commented May 31, 2023

sawsa307 commented May 31, 2023

sawsa307 commented May 31, 2023

swetharepakula May 31, 2023

sawsa307 May 31, 2023

swetharepakula May 31, 2023

sawsa307 May 31, 2023

swetharepakula May 31, 2023

sawsa307 May 31, 2023

swetharepakula May 31, 2023

sawsa307 May 31, 2023

sawsa307 May 31, 2023 •

edited

Loading

swetharepakula left a comment

k8s-ci-robot commented May 31, 2023

sawsa307 commented Jun 1, 2023

sawsa307 commented Jun 1, 2023

Add syncer in error state metrics #2156

Add syncer in error state metrics #2156

Conversation

sawsa307 commented May 30, 2023

k8s-ci-robot commented May 30, 2023

sawsa307 commented May 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sawsa307 May 30, 2023 • edited Loading

Choose a reason for hiding this comment

sawsa307 commented May 30, 2023

Choose a reason for hiding this comment

swetharepakula commented May 31, 2023

sawsa307 commented May 31, 2023

sawsa307 commented May 31, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sawsa307 May 31, 2023 • edited Loading

Choose a reason for hiding this comment

swetharepakula left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented May 31, 2023

sawsa307 commented Jun 1, 2023

sawsa307 commented Jun 1, 2023

sawsa307 May 30, 2023 •

edited

Loading

sawsa307 May 31, 2023 •

edited

Loading