Repository: knative/pkg
Pull request #1494 (2020-07-15 00:33:05)

Remove key in tags to reduce metrics count
Issue: https://github.com/knative/serving/issues/8609

Signed-off-by: Lance Liu <xuliuxl@cn.ibm.com>

knative-prow-robot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/pkg/pull/1494#" title="Author self-approved">lanceliuu</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **grantr**
You can assign the PR to them by writing `/assign @grantr` in a comment when ready.

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative%2Fpkg).

<details open>
Needs approval from an approver in each of these files:

- **[controller/OWNERS](https://github.com/knative/pkg/blob/master/controller/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["grantr"]} -->
knative-prow-robot: Welcome @lanceliuu! It looks like this is your first PR to knative/pkg 🎉
knative-prow-robot: Hi @lanceliuu. Thanks for your PR.

I'm waiting for a [knative](https://github.com/orgs/knative/people) 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](https://github.com/orgs/knative/people) 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](https://go.k8s.io/bot-commands?repo=knative%2Fpkg).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
</details>

evankanderson: You probably also want to remove `keyTagKey` from line 170 and 175 (which is where we instruct OpenCensus to aggregate divided by `key`), and the definition on line 49.
evankanderson: /ok-to-test
knative-prow-robot: @lanceliuu: The following test **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-knative-pkg-unit-tests | d9d7d70e58029c67ae7ee537987cf3f9ef30f563 | [link](https://prow.knative.dev/view/gcs/knative-prow/pr-logs/pull/knative_pkg/1494/pull-knative-pkg-unit-tests/1283297497195745282) | `/test pull-knative-pkg-unit-tests`

[Full PR test history](https://gubernator.knative.dev/pr/knative_pkg/1494). [Your PR dashboard](https://gubernator.knative.dev/pr/lanceliuu).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
lanceliuu: I removed the key tag accordingly. Thank you @evankanderson.
After apply this patch, the total metrics count for controller drops significantly.
Before:
```
~ wc -l metrics-controller.txt
   68628 metrics-controller.txt
```
After:
```
~ wc -l metrics-controller.txt
     570 metrics-controller.txt
```
Also the size of metrics drops from ~11M to ~66K
@vagababov
lanceliuu: /test pull-knative-pkg-unit-tests
knative-prow-robot: @lanceliuu: The following test **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-knative-pkg-unit-tests | d9d7d70e58029c67ae7ee537987cf3f9ef30f563 | [link](https://prow.knative.dev/view/gcs/knative-prow/pr-logs/pull/knative_pkg/1494/pull-knative-pkg-unit-tests/1283304921709416448) | `/test pull-knative-pkg-unit-tests`

[Full PR test history](https://gubernator.knative.dev/pr/knative_pkg/1494). [Your PR dashboard](https://gubernator.knative.dev/pr/lanceliuu).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
evankanderson: /retest
evankanderson: Wow, the cardinality for `key` must have been really high (which will cause problems with Stackdriver, at least, but many monitoring systems struggle with high-cardinality labels).

/lgtm
/approve
knative-prow-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/pkg/pull/1494#issuecomment-658610539" title="Approved">evankanderson</a>*, *<a href="https://github.com/knative/pkg/pull/1494#" title="Author self-approved">lanceliuu</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative%2Fpkg).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[controller/OWNERS](https://github.com/knative/pkg/blob/master/controller/OWNERS)~~ [evankanderson]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
knative-prow-robot: @lanceliuu: The following test **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-knative-pkg-unit-tests | d9d7d70e58029c67ae7ee537987cf3f9ef30f563 | [link](https://prow.knative.dev/view/gcs/knative-prow/pr-logs/pull/knative_pkg/1494/pull-knative-pkg-unit-tests/1283309576212451329) | `/test pull-knative-pkg-unit-tests`

[Full PR test history](https://gubernator.knative.dev/pr/knative_pkg/1494). [Your PR dashboard](https://gubernator.knative.dev/pr/lanceliuu).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
markusthoemmes: /retest

Patch:
diff --git a/controller/controller.go b/controller/controller.go
index 6024ec253..03f67b6ec 100644
--- a/controller/controller.go
+++ b/controller/controller.go
@@ -452,7 +452,7 @@ func (c *Impl) processNextWorkItem() bool {
 		if err != nil {
 			status = falseString
 		}
-		c.statsReporter.ReportReconcile(time.Since(startTime), keyStr, status)
+		c.statsReporter.ReportReconcile(time.Since(startTime), status)
 	}()
 
 	// Embed the key into the logger and attach that to the context we pass
diff --git a/controller/stats_reporter.go b/controller/stats_reporter.go
index c3445ed3d..6efaeca0f 100644
--- a/controller/stats_reporter.go
+++ b/controller/stats_reporter.go
@@ -46,7 +46,6 @@ var (
 	// - length between 1 and 255 inclusive
 	// - characters are printable US-ASCII
 	reconcilerTagKey = tag.MustNewKey("reconciler")
-	keyTagKey        = tag.MustNewKey("key")
 	successTagKey    = tag.MustNewKey("success")
 )
 
@@ -167,12 +166,12 @@ func init() {
 		Description: "Number of reconcile operations",
 		Measure:     reconcileCountStat,
 		Aggregation: view.Count(),
-		TagKeys:     []tag.Key{reconcilerTagKey, keyTagKey, successTagKey},
+		TagKeys:     []tag.Key{reconcilerTagKey, successTagKey},
 	}, {
 		Description: "Latency of reconcile operations",
 		Measure:     reconcileLatencyStat,
 		Aggregation: reconcileDistribution,
-		TagKeys:     []tag.Key{reconcilerTagKey, keyTagKey, successTagKey},
+		TagKeys:     []tag.Key{reconcilerTagKey, successTagKey},
 	}}
 	views = append(views, wp.DefaultViews()...)
 	views = append(views, rp.DefaultViews()...)
@@ -192,7 +191,7 @@ type StatsReporter interface {
 	ReportQueueDepth(v int64) error
 
 	// ReportReconcile reports the count and latency metrics for a reconcile operation
-	ReportReconcile(duration time.Duration, key, success string) error
+	ReportReconcile(duration time.Duration, success string) error
 }
 
 // Reporter holds cached metric objects to report metrics
@@ -234,11 +233,10 @@ func (r *reporter) ReportQueueDepth(v int64) error {
 }
 
 // ReportReconcile reports the count and latency metrics for a reconcile operation
-func (r *reporter) ReportReconcile(duration time.Duration, key, success string) error {
+func (r *reporter) ReportReconcile(duration time.Duration, success string) error {
 	ctx, err := tag.New(
 		context.Background(),
 		tag.Insert(reconcilerTagKey, r.reconciler),
-		tag.Insert(keyTagKey, key),
 		tag.Insert(successTagKey, success))
 	if err != nil {
 		return err


Test patch:
diff --git a/controller/stats_reporter_test.go b/controller/stats_reporter_test.go
index d82e7fe35..ff56d99a3 100644
--- a/controller/stats_reporter_test.go
+++ b/controller/stats_reporter_test.go
@@ -68,7 +68,6 @@ func TestReportReconcile(t *testing.T) {
 	r, _ := NewStatsReporter("testreconciler")
 	wantTags := map[string]string{
 		"reconciler": "testreconciler",
-		"key":        "test/key",
 		"success":    "true",
 	}
 
@@ -81,11 +80,11 @@ func TestReportReconcile(t *testing.T) {
 		initialReconcileLatency = d[0].Data.(*view.DistributionData).Sum()
 	}
 
-	expectSuccess(t, func() error { return r.ReportReconcile(10*time.Millisecond, "test/key", "true") })
+	expectSuccess(t, func() error { return r.ReportReconcile(10*time.Millisecond, "true") })
 	metricstest.CheckCountData(t, "reconcile_count", wantTags, initialReconcileCount+1)
 	metricstest.CheckDistributionData(t, "reconcile_latency", wantTags, 1, initialReconcileLatency+10, initialReconcileLatency+10)
 
-	expectSuccess(t, func() error { return r.ReportReconcile(15*time.Millisecond, "test/key", "true") })
+	expectSuccess(t, func() error { return r.ReportReconcile(15*time.Millisecond, "true") })
 	metricstest.CheckCountData(t, "reconcile_count", wantTags, initialReconcileCount+2)
 	metricstest.CheckDistributionData(t, "reconcile_latency", wantTags, 2, initialReconcileLatency+10, initialReconcileLatency+15)
 }
diff --git a/controller/testing/fake_stats_reporter.go b/controller/testing/fake_stats_reporter.go
index 506754dc1..528a854b3 100644
--- a/controller/testing/fake_stats_reporter.go
+++ b/controller/testing/fake_stats_reporter.go
@@ -30,8 +30,8 @@ type FakeStatsReporter struct {
 
 // FakeReconcileStatData is used to record the calls to ReportReconcile
 type FakeReconcileStatData struct {
-	Duration     time.Duration
-	Key, Success string
+	Duration time.Duration
+	Success  string
 }
 
 // ReportQueueDepth records the call and returns success.
@@ -43,10 +43,10 @@ func (r *FakeStatsReporter) ReportQueueDepth(v int64) error {
 }
 
 // ReportReconcile records the call and returns success.
-func (r *FakeStatsReporter) ReportReconcile(duration time.Duration, key, success string) error {
+func (r *FakeStatsReporter) ReportReconcile(duration time.Duration, success string) error {
 	r.Lock.Lock()
 	defer r.Lock.Unlock()
-	r.reconcileData = append(r.reconcileData, FakeReconcileStatData{duration, key, success})
+	r.reconcileData = append(r.reconcileData, FakeReconcileStatData{duration, success})
 	return nil
 }
 
diff --git a/controller/testing/fake_stats_reporter_test.go b/controller/testing/fake_stats_reporter_test.go
index dc2f894df..8fde1c7e4 100644
--- a/controller/testing/fake_stats_reporter_test.go
+++ b/controller/testing/fake_stats_reporter_test.go
@@ -38,8 +38,8 @@ func TestReportQueueDepth(t *testing.T) {
 
 func TestReportReconcile(t *testing.T) {
 	r := &FakeStatsReporter{}
-	r.ReportReconcile(time.Duration(123), "testkey", "False")
-	if got, want := r.GetReconcileData(), []FakeReconcileStatData{{time.Duration(123), "testkey", "False"}}; !reflect.DeepEqual(want, got) {
+	r.ReportReconcile(time.Duration(123), "False")
+	if got, want := r.GetReconcileData(), []FakeReconcileStatData{{time.Duration(123), "False"}}; !reflect.DeepEqual(want, got) {
 		t.Errorf("reconcile data len: want: %v, got: %v", want, got)
 	}
 }