Record metrics if config is nil (#315)

* record if nil

* some more comment

* some more comment

* return

controller/stats_reporter_test.go

@@ -21,15 +21,10 @@ import (
 	"testing"
 	"time"
 
-	. "github.com/knative/pkg/logging/testing"
-	"github.com/knative/pkg/metrics"
 	"go.opencensus.io/stats/view"
-	corev1 "k8s.io/api/core/v1"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
 func TestNewStatsReporterErrors(t *testing.T) {
-	initMetricsConfig(t)
 	// These are invalid as defined by the current OpenCensus library.
 	invalidTagValues := []string{
 		"naïve",                  // Includes non-ASCII character.
@@ -46,7 +41,6 @@ func TestNewStatsReporterErrors(t *testing.T) {
 }
 
 func TestReportQueueDepth(t *testing.T) {
-	initMetricsConfig(t)
 	r1 := &reporter{}
 	if err := r1.ReportQueueDepth(10); err == nil {
 		t.Error("Reporter.Report() expected an error for Report call before init. Got success.")
@@ -69,7 +63,6 @@ func TestReportQueueDepth(t *testing.T) {
 }
 
 func TestReportReconcile(t *testing.T) {
-	initMetricsConfig(t)
 	r, _ := NewStatsReporter("testreconciler")
 	wantTags := map[string]string{
 		"reconciler": "testreconciler",
@@ -86,16 +79,6 @@ func TestReportReconcile(t *testing.T) {
 	checkDistributionData(t, "reconcile_latency", wantTags, 25)
 }
 
-func initMetricsConfig(t *testing.T) {
-	cm := &corev1.ConfigMap{
-		ObjectMeta: metav1.ObjectMeta{
-			Name: "config-observability",
-		},
-		Data: map[string]string{},
-	}
-	metrics.UpdateExporterFromConfigMap("testDomain", "testComponent", TestLogger(t))(cm)
-}
-
 func expectSuccess(t *testing.T, f func() error) {
 	t.Helper()
 	if err := f(); err != nil {

metrics/record.go

@@ -20,29 +20,37 @@ import (
 	"context"
 	"path"
 
-	"github.com/knative/pkg/logging"
 	"github.com/knative/pkg/metrics/metricskey"
 	"go.opencensus.io/stats"
 )
 
-// Record decides whether to record one measurement based on current
+// Record decides whether to record one measurement via OpenCensus based on the
-// metrics config. If yes, it records measurements via OpenCensus.
+// following conditions:
+//   1) No package level metrics config. In this case it just proxies to OpenCensus
+//      based on the assumption that users expect the metrics to be recorded when
+//      they call this function. Users must ensure metrics config are set before
+//      using this function to get expected behavior.
+//   2) The backend is not Stackdriver.
+//   3) The backend is Stackdriver and it is allowed to use custom metrics.
+//   4) The backend is Stackdriver and the metric is "knative_revison" built-in metric.
 func Record(ctx context.Context, ms stats.Measurement) {
 	mc := getCurMetricsConfig()
+
+	// Condition 1)
 	if mc == nil {
-		logging.FromContext(ctx).Warn("The current metrics config has not been successfully set yet, not record the metric %v.", ms)
+		stats.Record(ctx, ms)
 		return
 	}
-	// Should record if one of the following conditions satisfies:
+
-	// 1) the backend is not Stackdriver
+	// Condition 2) and 3)
-	// 2) the backend is Stackdriver and it is allowed to use custom metrics
-	// 3) the backend is Stackdriver and the metric is "knative_revison" built-in metric.
 	if !mc.isStackdriverBackend || mc.allowStackdriverCustomMetrics {
 		stats.Record(ctx, ms)
-	} else {
+		return
-		metricType := path.Join(mc.stackdriverMetricTypePrefix, ms.Measure().Name())
+	}
-		if metricskey.KnativeRevisionMetrics.Has(metricType) {
+
-			stats.Record(ctx, ms)
+	// Condition 4)
-		}
+	metricType := path.Join(mc.stackdriverMetricTypePrefix, ms.Measure().Name())
+	if metricskey.KnativeRevisionMetrics.Has(metricType) {
+		stats.Record(ctx, ms)
 	}
 }

metrics/record_test.go

@@ -60,6 +60,9 @@ func TestRecord(t *testing.T) {
 				allowStackdriverCustomMetrics: true,
 			},
 			measurement: measure.M(3),
+		}, {
+			name:        "empty metricsConfig",
+			measurement: measure.M(4),
 		},
 	}
 
@@ -81,9 +84,6 @@ func TestRecord(t *testing.T) {
 				isStackdriverBackend:        true,
 				stackdriverMetricTypePrefix: "knative.dev/unsupported",
 			},
-			measurement: measure.M(4),
-		}, {
-			name:        "empty metricsConfig",
 			measurement: measure.M(5),
 		},
 	}
@@ -91,7 +91,7 @@ func TestRecord(t *testing.T) {
 	for _, test := range shouldNotReportCases {
 		setCurMetricsConfig(test.metricsConfig)
 		Record(ctx, test.measurement)
-		checkLastValueData(t, test.measurement.Measure().Name(), 3) // The value is still the last one of shouldReportCases
+		checkLastValueData(t, test.measurement.Measure().Name(), 4) // The value is still the last one of shouldReportCases
 	}
 }