Add label propagation metrics calculation logic #2077

ruixiansong · 2023-04-14T18:09:57Z

k8s-ci-robot · 2023-04-14T18:10:06Z

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

swetharepakula · 2023-04-14T18:12:08Z

/ok-to-test

ruixiansong · 2023-04-14T18:55:44Z

/retest

swetharepakula · 2023-04-14T18:55:54Z

pkg/neg/metrics/neg_metrics_collector.go

@@ -109,6 +110,16 @@ func (sm *SyncerMetrics) SetSyncerEPMetrics(key negtypes.NegSyncerKey, endpointS
 	sm.syncerEPSStateMap[key] = endpointStat.EndpointSliceStateCount
 }

+func (sm *SyncerMetrics) SetLabelMetrics(key negtypes.NegSyncerKey, labelstatLabelPropagationStats LabelPropagationStats) {


rename to labelPropagationStats

swetharepakula · 2023-04-14T19:13:49Z

pkg/neg/syncers/labels/labels.go

@@ -95,8 +102,20 @@ func truncatePodLabel(key, label string, maxTotalSize int) (string, error) {
 		return label, nil
 	}
 	if len(keyBytes)+minLabelLength > maxTotalSize {
+		metrics.PublishLabelPropagationError(TruncationFailure)


similar to logging, pull this out to the caller

swetharepakula · 2023-04-14T19:14:01Z

pkg/neg/syncers/labels/labels.go

 		return "", fmt.Errorf("%w: `%s:%s` truncation failed because the key exceeded the limit, length: %d, limit %d", ErrLabelTruncationFailed, key, label, len(keyBytes)+minLabelLength, maxTotalSize)
 	}
 	truncatedVal := string(labelBytes[:maxTotalSize-len(keyBytes)])
+	metrics.PublishLabelPropagationError(Truncated)


same comment as above

swetharepakula · 2023-04-14T21:03:40Z

pkg/neg/syncers/transaction.go

 				logger.Error(nil, "expected type *v1.Pod", "pod", key, "type", fmt.Sprintf("%T", obj))
 				continue
 			}
 			labelMap, err := labels.GetPodLabelMap(pod, lpConfig)
 			if err != nil {
 				recorder.Eventf(pod, apiv1.EventTypeWarning, "LabelsExceededLimit", "Label Propagation Error: %v", err)
 			}
+			metrics.PublishAnnotationMetrics(labels.GetPodLabelMapSize(labelMap), len(labelMap))


move this to the caller. we shouldn't have metrics in helper methods unless the helper function is for the metrics.

we should also move events to the caller as well for the same reason

pkg/neg/syncers/utils_test.go

swetharepakula · 2023-04-14T23:22:28Z

pkg/neg/metrics/label_propagation_metrics.go

+	annotationSize      = "annotation_size_per_endpoint"
+	labelErrorNumber    = "label_propagation_error_count"
+	numberOfEndpoints   = "number_of_endpoints"
+	epWithAnnotation    = "with_annoatation"


typo: annotation

swetharepakula · 2023-04-14T23:22:52Z

pkg/neg/metrics/label_propagation_metrics.go

+	labelErrorNumber    = "label_propagation_error_count"
+	numberOfEndpoints   = "number_of_endpoints"
+	epWithAnnotation    = "with_annoatation"
+	epWithoutAnnotation = "without_annotation"


we don't need this one. we can calculate with number of endpoints and without_annotation

swetharepakula · 2023-04-14T23:23:34Z

pkg/neg/metrics/neg_metrics_collector.go

@@ -79,6 +80,10 @@ func (sm *SyncerMetrics) Run(stopCh <-chan struct{}) {

 // export exports syncer metrics.
 func (sm *SyncerMetrics) export() {
+	lpMetrics := sm.computeLabelMetrics()
+	NumberOfEndpoints.WithLabelValues(epWithoutAnnotation).Set(float64(lpMetrics.NumberOfEndpoints - lpMetrics.EndpointsWithAnnotation))


let's remove this one. if we need this, we can calculate when we consume the metrics

swetharepakula · 2023-04-14T23:31:00Z

pkg/neg/metrics/label_propagation_metrics.go

+	annotationSize      = "annotation_size_per_endpoint"
+	labelErrorNumber    = "label_propagation_error_count"
+	numberOfEndpoints   = "number_of_endpoints"
+	epWithAnnotation    = "with_annoatation"


github won't let me comment on parts that aren't part of the diff. For the endpointAnnotationLabels, that should be changed to feature and then the value will be with_annotation

swetharepakula · 2023-04-14T23:32:49Z

pkg/neg/syncers/labels/labels.go

+			if errors.Is(err, ErrLabelTruncated) {
+				metrics.PublishLabelPropagationError(Truncated)
+			} else if errors.Is(err, ErrLabelTruncationFailed) {
+				metrics.PublishLabelPropagationError(TruncationFailure)
+			}


pull this out into its own helper: publishLabelPropagationTruncationMetrics

pkg/neg/syncers/utils_test.go

swetharepakula · 2023-04-15T00:01:42Z

/retest

swetharepakula · 2023-04-17T15:55:47Z

pkg/neg/syncers/transaction_test.go

+	} {
+		out := collectLabelStats(tc.curLabelMap, tc.addLabelMap, tc.targetEndpointMap)
+		if diff := cmp.Diff(out, tc.expect); diff != "" {
+			t.Errorf("For test case %s: got %+v, want %+v, diff %s", tc.desc, out, tc.expect, diff)


nit: remove got %+v, want %+v and add the -want, +got

swetharepakula

/lgtm
/approve

k8s-ci-robot · 2023-04-17T16:10:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: songrx1997, 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

k8s-ci-robot assigned swetharepakula Apr 14, 2023

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 14, 2023

k8s-ci-robot requested review from aojea and code-elinka April 14, 2023 18:10

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 14, 2023

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 Apr 14, 2023

swetharepakula reviewed Apr 14, 2023

View reviewed changes

ruixiansong force-pushed the label-propagation-metrics branch from 770e6d8 to 9b62af3 Compare April 14, 2023 23:14

swetharepakula reviewed Apr 14, 2023

View reviewed changes

ruixiansong force-pushed the label-propagation-metrics branch from 9b62af3 to a915ae2 Compare April 14, 2023 23:54

ruixiansong force-pushed the label-propagation-metrics branch 3 times, most recently from 528e53e to 279fc57 Compare April 17, 2023 15:43

ruixiansong changed the title ~~Add label propagation calculation logic~~ Add label propagation metrics calculation logic Apr 17, 2023

swetharepakula reviewed Apr 17, 2023

View reviewed changes

ruixiansong added 2 commits April 17, 2023 09:00

Add label propagation metrics calculation logic

83a8538

Fix logging format in neg metrics collector

2a56103

ruixiansong force-pushed the label-propagation-metrics branch from 279fc57 to 2a56103 Compare April 17, 2023 16:00

swetharepakula reviewed Apr 17, 2023

View reviewed changes

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

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

k8s-ci-robot merged commit 6080857 into kubernetes:master Apr 17, 2023

ruixiansong deleted the label-propagation-metrics branch May 2, 2023 19:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add label propagation metrics calculation logic #2077

Add label propagation metrics calculation logic #2077

ruixiansong commented Apr 14, 2023

k8s-ci-robot commented Apr 14, 2023

swetharepakula commented Apr 14, 2023

ruixiansong commented Apr 14, 2023

swetharepakula Apr 14, 2023

swetharepakula Apr 14, 2023

swetharepakula Apr 14, 2023

swetharepakula Apr 14, 2023

swetharepakula Apr 14, 2023

swetharepakula Apr 14, 2023

swetharepakula Apr 14, 2023

swetharepakula Apr 14, 2023

swetharepakula Apr 14, 2023

swetharepakula Apr 14, 2023

swetharepakula commented Apr 15, 2023

swetharepakula Apr 17, 2023

swetharepakula left a comment

k8s-ci-robot commented Apr 17, 2023

Add label propagation metrics calculation logic #2077

Add label propagation metrics calculation logic #2077

Conversation

ruixiansong commented Apr 14, 2023

k8s-ci-robot commented Apr 14, 2023

swetharepakula commented Apr 14, 2023

ruixiansong commented Apr 14, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swetharepakula commented Apr 15, 2023

Choose a reason for hiding this comment

swetharepakula left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Apr 17, 2023