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.

Sign up for GitHub

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

Jump to bottom

Add metrics to track endpointslice staleness #1930

Merged

k8s-ci-robot merged 1 commit into kubernetes:master from sawsa307:neg-eps-staleness-metrics

Feb 16, 2023

Contributor

sawsa307 commented Feb 2, 2023

Added metrics to track the sync staleness of endpointslices, where staleness is defined as how long since it has been last processed.

k8s-ci-robot added the cncf-cla: yes label

k8s-ci-robot requested review from aojea and prameshj

February 2, 2023 20:13

k8s-ci-robot added the needs-ok-to-test label

Contributor

k8s-ci-robot commented Feb 2, 2023

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.

k8s-ci-robot added the size/M label

sawsa307 force-pushed the neg-eps-staleness-metrics branch 2 times, most recently from 67915aa to 187446e Compare

February 2, 2023 20:24

Contributor Author

sawsa307 commented Feb 2, 2023

/assign @swetharepakula

k8s-ci-robot assigned swetharepakula

sawsa307 marked this pull request as draft

February 4, 2023 05:50

k8s-ci-robot added the do-not-merge/work-in-progress label

sawsa307 marked this pull request as ready for review

February 4, 2023 05:50

k8s-ci-robot removed the do-not-merge/work-in-progress label

k8s-ci-robot requested a review from freehan

February 4, 2023 05:50

Member

bowei commented Feb 9, 2023

/ok-to-test

k8s-ci-robot added ok-to-test and removed needs-ok-to-test labels

swetharepakula reviewed

View reviewed changes

pkg/neg/metrics/metrics.go Outdated Show resolved Hide resolved

pkg/neg/syncers/transaction.go Outdated

+              				s.logger.V(3).Info("Endpoint slice syncs", "Namespace/Name", key, "staleness", staleness)
+              				// remove from previous map since this eps is updated
+              				delete(s.epsLastSyncTimestampMap, key)

Member

swetharepakula Feb 13, 2023

For this metric, I don't think we need to track the lastSyncTimestamp per EPS. we have the following cases:

EPS newly created since last syncer sync
EPS existed previously and was processed during the lastSyncer Sync

Since we already know when we last synced the syncer (from the svcNeg CR), you just need to check if the EPS creation timestamp is before or after the lastSyncTimestamp. If the creationTS is after now() then you publish now()-creationTS() for that endpoint's staleness. If creationTS is before now() then you publish now()-syncerLastSync timestamp.

Otherwise you have to add logic that will allow you to recognize when an EPS was deleted as you will never delete those from the map.

sawsa307 force-pushed the neg-eps-staleness-metrics branch 2 times, most recently from 91de2da to b34fe9e Compare

February 13, 2023 23:22

swetharepakula reviewed

View reviewed changes

pkg/neg/syncers/transaction.go Outdated

+              		negCR, err := getNegFromStore(s.svcNegLister, s.Namespace, s.NegSyncerKey.NegName)
+              		if err != nil {
+              			s.logger.Error(err, "unable to retrieve neg from the store", "neg", klog.KRef(s.Namespace, s.NegName))
+              			return err

Member

swetharepakula Feb 15, 2023

since this is only for metrics, do not return an error.

pkg/neg/syncers/transaction.go Outdated

Comment on lines 262 to 263

+              			if lastSyncTimestamp.Before(&epsCreationTimestamp) {
+              				epsStaleness = time.Since(epsCreationTimestamp.Time)
+              			} else {
+              				epsStaleness = time.Since(lastSyncTimestamp.Time)
+              			}

Member

swetharepakula Feb 15, 2023

We can remove the else and you don't need line 257 either.

epsStaleness := time.Since(lastSyncTimestamp.Time)
if lastSyncTimestamp.Before(&epsCreationTimestamp) {
    epsStaleness = time.Since(epsCreationTimestamp.Time)
}

sawsa307 force-pushed the neg-eps-staleness-metrics branch from b34fe9e to 0824e66 Compare

February 15, 2023 21:41

k8s-ci-robot added the needs-rebase label

sawsa307 force-pushed the neg-eps-staleness-metrics branch from 0824e66 to 7e37954 Compare

February 15, 2023 21:42

k8s-ci-robot removed the needs-rebase label

sawsa307 requested review from swetharepakula and removed request for freehan, aojea and prameshj

February 15, 2023 21:43

swetharepakula reviewed

View reviewed changes

pkg/neg/syncers/transaction.go Outdated

+              			endpointslice := slice.(*discovery.EndpointSlice)
+              			endpointSlices[i] = endpointslice
+              			negCR, err := getNegFromStore(s.svcNegLister, s.Namespace, s.NegSyncerKey.NegName)

Member

swetharepakula Feb 15, 2023

move this outside of the for loop. There is only one NEG per syncer.

swetharepakula reviewed

View reviewed changes

pkg/neg/syncers/transaction.go Show resolved Hide resolved

swetharepakula reviewed

View reviewed changes

pkg/neg/syncers/transaction.go Outdated

+              			if err != nil {
+              				s.logger.Error(err, "unable to retrieve neg from the store", "neg", klog.KRef(s.Namespace, s.NegName))
+              			} else {
+              				var epsStaleness time.Duration

Member

swetharepakula Feb 15, 2023

remove this. Initialize when you will use it

sawsa307 force-pushed the neg-eps-staleness-metrics branch from 7e37954 to 3240b9f Compare

February 15, 2023 22:32


          Add metrics to track endpointslice staleness

8669f63

Added metrics to track the sync staleness of endpointslices, where
staleness is defined as how long since it has been last processed.

sawsa307 force-pushed the neg-eps-staleness-metrics branch from 3240b9f to 8669f63 Compare

February 15, 2023 22:34

swetharepakula reviewed

View reviewed changes

Member

swetharepakula left a comment

/lgtm
/approve

k8s-ci-robot added the lgtm label

Contributor

k8s-ci-robot commented Feb 16, 2023

[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

k8s-ci-robot added the approved label

k8s-ci-robot merged commit 98aa2cf into kubernetes:master

sawsa307 deleted the neg-eps-staleness-metrics branch

April 4, 2023 00:30

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved cncf-cla: yes lgtm ok-to-test size/M