Neg crd metrics #1222

swetharepakula · 2020-08-20T19:24:23Z

Add metrics to track custom named neg usage
Add metrics to track internal neg latencies

k8s-ci-robot · 2020-08-20T19:24:30Z

Hi @swetharepakula. 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 · 2020-08-20T19:28:18Z

/assign @freehan

swetharepakula · 2020-08-20T19:39:12Z

/cc prameshj

freehan · 2020-08-21T17:26:00Z

/ok-to-test

freehan

LGTM overall

I will let pavithra to take a look

prameshj

mostly LGTM. Just a few nits

prameshj · 2020-08-21T18:11:39Z

pkg/neg/controller.go

@@ -333,7 +333,7 @@ func (c *Controller) nodeWorker() {
 func (c *Controller) processNode() {
 	defer func() {
 		now := c.nodeSyncTracker.Track()
-		metrics.LastSyncTimestamp.WithLabelValues().Set(float64(now.UTC().UnixNano()))


What does this change?

I changed the metric from a GaugeVec to a Gauge, so there are no more labels needed.

prameshj · 2020-08-21T18:13:16Z

pkg/neg/metrics/metrics.go

+
+	syncerMetricsLabels = []string{
+		"neg_type",           //type of neg
+		"endpoint_calulator", // type of endpoint calculator used


nit: call this "endpoint_calculator_mode"?

prameshj · 2020-08-21T18:15:40Z

pkg/neg/metrics/metrics.go

+			Namespace: metrics.GLBC_NAMESPACE,
+			Subsystem: negControllerSubsystem,
+			Name:      negOpLatencyKey,
+			Help:      "Latency of a NEG syncer for Endpoint Operation",


"Latency of NEG operation"?

prameshj · 2020-08-21T18:19:56Z

pkg/neg/metrics/metrics.go

+			Namespace: metrics.GLBC_NAMESPACE,
+			Subsystem: negControllerSubsystem,
+			Name:      negOpEndpointsKey,
+			Help:      "Number of Endpoints during an Endpoint Operation",


NEG operation?

prameshj · 2020-08-21T18:34:24Z

pkg/neg/syncers/transaction.go

@@ -293,11 +298,13 @@ func (s *transactionSyncer) syncNetworkEndpoints(addEndpoints, removeEndpoints m
 		return nil
 	}

-	if err := syncFunc(addEndpoints, attachOp); err != nil {
+	err := syncFunc(addEndpoints, attachOp)


nit: is this change needed?

No this change isn't needed, I think it is a remanent of an earlier version of the code.

prameshj · 2020-08-21T18:37:38Z

pkg/neg/metrics/metrics.go

 	})
 }

+func ObserveNegOperation(operation, negType, apiVersion string, err error, numEndpoints int, start time.Time) {


nit: Can we call this "PublishNegOperationMetrics" and similar for the other functions here? That will make it very clear when reading the code this is called at.

I like that suggestion, updating all the publishing functions.

freehan · 2020-08-24T18:33:42Z

/lgtm
/approve

- remove `glbc` namespace to existing ingress metrics - change last sync timestamp metric to gauge

freehan

/lgtm
/approve

k8s-ci-robot · 2020-08-24T21:32:35Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: freehan, 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~~ [freehan]

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

Cherry Pick #1222 [Neg crd metrics]

Add custom named neg as feature for usage metrics

4cc595f

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 20, 2020

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 Aug 20, 2020

k8s-ci-robot requested review from MrHohn and rramkumar1 August 20, 2020 19:24

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 20, 2020

k8s-ci-robot assigned freehan Aug 20, 2020

k8s-ci-robot requested a review from prameshj August 20, 2020 19:39

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 Aug 21, 2020

freehan assigned prameshj Aug 21, 2020

freehan reviewed Aug 21, 2020

View reviewed changes

prameshj reviewed Aug 21, 2020

View reviewed changes

swetharepakula force-pushed the neg-crd-metrics branch from 7842473 to 5a0145e Compare August 21, 2020 19:27

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Aug 24, 2020

swetharepakula force-pushed the neg-crd-metrics branch from 5a0145e to 208a865 Compare August 24, 2020 18:45

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 24, 2020

swetharepakula force-pushed the neg-crd-metrics branch from 208a865 to b73e95a Compare August 24, 2020 18:58

Add internal neg controller metrics

aba3c6e

- remove `glbc` namespace to existing ingress metrics - change last sync timestamp metric to gauge

swetharepakula force-pushed the neg-crd-metrics branch from b73e95a to aba3c6e Compare August 24, 2020 18:59

prameshj approved these changes Aug 24, 2020

View reviewed changes

freehan approved these changes Aug 24, 2020

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 24, 2020

k8s-ci-robot merged commit 18d503b into kubernetes:master Aug 24, 2020

swetharepakula mentioned this pull request Aug 25, 2020

Cherry Pick #1222 [Neg crd metrics] #1229

Merged

k8s-ci-robot added a commit that referenced this pull request Aug 25, 2020

Merge pull request #1229 from swetharepakula/metrics

81d2811

Cherry Pick #1222 [Neg crd metrics]

swetharepakula deleted the neg-crd-metrics branch September 18, 2020 00:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Neg crd metrics #1222

Neg crd metrics #1222

swetharepakula commented Aug 20, 2020 •

edited

Loading

k8s-ci-robot commented Aug 20, 2020

swetharepakula commented Aug 20, 2020

swetharepakula commented Aug 20, 2020

freehan commented Aug 21, 2020

freehan left a comment

prameshj left a comment

prameshj Aug 21, 2020

swetharepakula Aug 21, 2020

prameshj Aug 21, 2020

prameshj Aug 21, 2020

prameshj Aug 21, 2020

prameshj Aug 21, 2020

swetharepakula Aug 21, 2020

prameshj Aug 21, 2020

swetharepakula Aug 21, 2020

freehan commented Aug 24, 2020

freehan left a comment

k8s-ci-robot commented Aug 24, 2020

Neg crd metrics #1222

Neg crd metrics #1222

Conversation

swetharepakula commented Aug 20, 2020 • edited Loading

k8s-ci-robot commented Aug 20, 2020

swetharepakula commented Aug 20, 2020

swetharepakula commented Aug 20, 2020

freehan commented Aug 21, 2020

freehan left a comment

Choose a reason for hiding this comment

prameshj left a 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

Choose a reason for hiding this comment

freehan commented Aug 24, 2020

freehan left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Aug 24, 2020

swetharepakula commented Aug 20, 2020 •

edited

Loading