Repository: knative/pkg
Pull request #1323 (2020-05-11 21:58:33)

Don't record data points when metric config is not initialized yet
A follow up PR of #1263 to a race condition that recording the data points before the metric config is initialized, at which point whether to record is unknown.

This is a breaking change for unit tests of downstream repos. Changes necessary have been made to knative/serving: https://github.com/knative/serving/pull/7911. Verified that unit tests won't break with this PR.

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

This pull-request has been approved by: *<a href="https://github.com/knative/pkg/pull/1323#" title="Author self-approved">yanweiguo</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:

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

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":[]} -->
yanweiguo: /assign @vagababov @anniefu 
knative-metrics-robot: The following is the coverage report on the affected files.
Say `/test pull-knative-pkg-go-coverage` to re-run this coverage report

File | Old Coverage | New Coverage | Delta
---- |:------------:|:------------:|:-----:
[metrics/config.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_pkg/1323/pull-knative-pkg-go-coverage/1259966293944045569/artifacts/line-cov.html#file0) | 94.4% | 93.1% | -1.4

mdemirhan: /lgtm

Patch:
diff --git a/metrics/config.go b/metrics/config.go
index 71161baa1..a752e8d37 100644
--- a/metrics/config.go
+++ b/metrics/config.go
@@ -21,6 +21,7 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
+	"log"
 	"os"
 	"path"
 	"strconv"
@@ -153,7 +154,20 @@ func NewStackdriverClientConfigFromMap(config map[string]string) *StackdriverCli
 // record applies the `ros` Options to each measurement in `mss` and then records the resulting
 // measurements in the metricsConfig's designated backend.
 func (mc *metricsConfig) record(ctx context.Context, mss []stats.Measurement, ros ...stats.Options) error {
-	if mc == nil || mc.recorder == nil {
+	if mc == nil {
+		log.Println(`The metricsConfig has not been initialized yet.
+
+If this is a Go unit test consuming metric.Record(...) or metric.RecordBatch(...) then
+it should add the following import:
+
+import (
+	_ "knative.dev/pkg/metrics/testing"
+)`)
+		// Don't record data points if the metric config is not initialized yet.
+		// At this point, it's unclear whether should record or not.
+		return nil
+	}
+	if mc.recorder == nil {
 		return stats.RecordWithOptions(ctx, append(ros, stats.WithMeasurements(mss...))...)
 	}
 	return mc.recorder(ctx, mss, ros...)


Test patch:
diff --git a/metrics/record_test.go b/metrics/record_test.go
index 80c4d2f05..7a3568bc8 100644
--- a/metrics/record_test.go
+++ b/metrics/record_test.go
@@ -58,9 +58,6 @@ func TestRecordServing(t *testing.T) {
 			stackdriverMetricTypePrefix: "knative.dev/unsupported",
 		},
 		measurement: measure.M(3),
-	}, {
-		name:        "empty metricsConfig",
-		measurement: measure.M(4),
 	}}
 	testRecord(t, measure, shouldReportCases)
 }
@@ -87,9 +84,6 @@ func TestRecordEventing(t *testing.T) {
 			stackdriverMetricTypePrefix: "knative.dev/unsupported",
 		},
 		measurement: measure.M(3),
-	}, {
-		name:        "empty metricsConfig",
-		measurement: measure.M(4),
 	}}
 	testRecord(t, measure, shouldReportCases)
 }
@@ -149,6 +143,9 @@ func testRecord(t *testing.T, measure *stats.Int64Measure, shouldReportCases []c
 			},
 		},
 		measurement: measure.M(5),
+	}, {
+		name:        "empty metricsConfig",
+		measurement: measure.M(4),
 	}}
 
 	for _, test := range shouldNotReportCases {