PR Feedback + SetUserID Defensiveness

pbsmetrics/prometheus/preload.go

@@ -5,15 +5,18 @@ import (
 )
 
 func preloadLabelValues(m *Metrics) {
-	actionValues := actionsAsString()
-	adapterValues := adaptersAsString()
-	adapterErrorValues := adapterErrorsAsString()
-	bidTypeValues := []string{markupDeliveryAdm, markupDeliveryNurl}
-	boolValues := boolValuesAsString()
-	cacheResultValues := cacheResultsAsString()
-	connectionErrorValues := []string{connectionAcceptError, connectionCloseError}
-	requestStatusValues := requestStatusesAsString()
-	requestTypeValues := requestTypesAsString()
+	var (
+		actionValues          = actionsAsString()
+		adapterValues         = adaptersAsString()
+		adapterErrorValues    = adapterErrorsAsString()
+		bidTypeValues         = []string{markupDeliveryAdm, markupDeliveryNurl}
+		boolValues            = boolValuesAsString()
+		cacheResultValues     = cacheResultsAsString()
+		cookieValues          = cookieTypesAsString()
+		connectionErrorValues = []string{connectionAcceptError, connectionCloseError}
+		requestStatusValues   = requestStatusesAsString()
+		requestTypeValues     = requestTypesAsString()
+	)
 
 	preloadLabelValuesForCounter(m.connectionsError, map[string][]string{
 		connectionErrorLabel: connectionErrorValues,
@@ -75,9 +78,9 @@ func preloadLabelValues(m *Metrics) {
 	})
 
 	preloadLabelValuesForCounter(m.adapterRequests, map[string][]string{
-		adapterLabel:  adapterValues,
-		noCookieLabel: boolValues,
-		hasBidsLabel:  boolValues,
+		adapterLabel: adapterValues,
+		cookieLabel:  cookieValues,
+		hasBidsLabel: boolValues,
 	})
 
 	preloadLabelValuesForHistogram(m.adapterRequestsTimer, map[string][]string{

pbsmetrics/prometheus/prometheus.go

@@ -50,13 +50,13 @@ const (
 	bidTypeLabel         = "bid_type"
 	cacheResultLabel     = "cache_result"
 	connectionErrorLabel = "connection_error"
+	cookieLabel          = "cookie"
 	hasBidsLabel         = "has_bids"
 	isAudioLabel         = "audio"
 	isBannerLabel        = "banner"
 	isNativeLabel        = "native"
 	isVideoLabel         = "video"
 	markupDeliveryLabel  = "delivery"
-	noCookieLabel        = "missing_cookie"
 	privacyBlockedLabel  = "privacy_blocked"
 	requestStatusLabel   = "request_status"
 	requestTypeLabel     = "request_type"
@@ -168,8 +168,8 @@ func NewMetrics(cfg config.PrometheusMetrics) *Metrics {
 
 	metrics.adapterRequests = newCounter(cfg, metrics.Registry,
 		"adapter_requests",
-		"Count of requests labeled by adapter, if has no cookie, and if it resulted in bids.",
-		[]string{adapterLabel, noCookieLabel, hasBidsLabel})
+		"Count of requests labeled by adapter, if has a cookie, and if it resulted in bids.",
+		[]string{adapterLabel, cookieLabel, hasBidsLabel})
 
 	metrics.adapterRequestsTimer = newHistogram(cfg, metrics.Registry,
 		"adapter_request_time_seconds",
@@ -291,9 +291,9 @@ func (m *Metrics) RecordRequestTime(labels pbsmetrics.Labels, length time.Durati
 
 func (m *Metrics) RecordAdapterRequest(labels pbsmetrics.AdapterLabels) {
 	m.adapterRequests.With(prometheus.Labels{
-		adapterLabel:  string(labels.Adapter),
-		noCookieLabel: strconv.FormatBool(labels.CookieFlag == pbsmetrics.CookieFlagNo),
-		hasBidsLabel:  strconv.FormatBool(labels.AdapterBids == pbsmetrics.AdapterBidPresent),
+		adapterLabel: string(labels.Adapter),
+		cookieLabel:  string(labels.CookieFlag),
+		hasBidsLabel: strconv.FormatBool(labels.AdapterBids == pbsmetrics.AdapterBidPresent),
 	}).Inc()
 
 	for err := range labels.AdapterErrors {
@@ -348,10 +348,13 @@ func (m *Metrics) RecordAdapterCookieSync(adapter openrtb_ext.BidderName, privac
 }
 
 func (m *Metrics) RecordUserIDSet(labels pbsmetrics.UserLabels) {
-	m.adapterUserSync.With(prometheus.Labels{
-		adapterLabel: string(labels.Bidder),
-		actionLabel:  string(labels.Action),
-	}).Inc()
+	adapter := string(labels.Bidder)
+	if adapter != "" {
+		m.adapterUserSync.With(prometheus.Labels{
+			adapterLabel: adapter,
+			actionLabel:  string(labels.Action),
+		}).Inc()
+	}
 }
 
 func (m *Metrics) RecordStoredReqCacheResult(cacheResult pbsmetrics.CacheResult, inc int) {

pbsmetrics/prometheus/prometheus_test.go

@@ -61,7 +61,7 @@ func TestMetricCountGatekeeping(t *testing.T) {
 	// Verify Per-Adapter Cardinality
 	// - This assertion provides a warning for newly added adapter metrics. Threre are 40+ adapters which makes the
 	//   cost of new per-adapter metrics rather expensive. Thought should be given when adding new per-adapter metrics.
-	assert.True(t, perAdapterCardinalityCount <= 20, "Per-Adapter Cardinality")
+	assert.True(t, perAdapterCardinalityCount <= 22, "Per-Adapter Cardinality")
 }
 
 func TestConnectionMetrics(t *testing.T) {
@@ -487,65 +487,79 @@ func TestAdapterRequestMetrics(t *testing.T) {
 	}
 
 	testCases := []struct {
-		description           string
-		testCase              func(m *Metrics)
-		expectedCount         float64
-		expectedNoCookieCount float64
-		expectedHasBidsCount  float64
+		description                string
+		testCase                   func(m *Metrics)
+		expectedCount              float64
+		expectedCookieNoCount      float64
+		expectedCookieYesCount     float64
+		expectedCookieUnknownCount float64
+		expectedHasBidsCount       float64
 	}{
 		{
 			description: "No Cookie & No Bids",
 			testCase: func(m *Metrics) {
 				performTest(m, pbsmetrics.CookieFlagNo, pbsmetrics.AdapterBidNone)
 			},
-			expectedCount:         1,
-			expectedNoCookieCount: 1,
-			expectedHasBidsCount:  0,
+			expectedCount:              1,
+			expectedCookieNoCount:      1,
+			expectedCookieYesCount:     0,
+			expectedCookieUnknownCount: 0,
+			expectedHasBidsCount:       0,
 		},
 		{
 			description: "Unknown Cookie & No Bids",
 			testCase: func(m *Metrics) {
 				performTest(m, pbsmetrics.CookieFlagUnknown, pbsmetrics.AdapterBidNone)
 			},
-			expectedCount:         1,
-			expectedNoCookieCount: 0,
-			expectedHasBidsCount:  0,
+			expectedCount:              1,
+			expectedCookieNoCount:      0,
+			expectedCookieYesCount:     0,
+			expectedCookieUnknownCount: 1,
+			expectedHasBidsCount:       0,
 		},
 		{
 			description: "Has Cookie & No Bids",
 			testCase: func(m *Metrics) {
 				performTest(m, pbsmetrics.CookieFlagYes, pbsmetrics.AdapterBidNone)
 			},
-			expectedCount:         1,
-			expectedNoCookieCount: 0,
-			expectedHasBidsCount:  0,
+			expectedCount:              1,
+			expectedCookieNoCount:      0,
+			expectedCookieYesCount:     1,
+			expectedCookieUnknownCount: 0,
+			expectedHasBidsCount:       0,
 		},
 		{
 			description: "No Cookie & Bids Present",
 			testCase: func(m *Metrics) {
 				performTest(m, pbsmetrics.CookieFlagNo, pbsmetrics.AdapterBidPresent)
 			},
-			expectedCount:         1,
-			expectedNoCookieCount: 1,
-			expectedHasBidsCount:  1,
+			expectedCount:              1,
+			expectedCookieNoCount:      1,
+			expectedCookieYesCount:     0,
+			expectedCookieUnknownCount: 0,
+			expectedHasBidsCount:       1,
 		},
 		{
 			description: "Unknown Cookie & Bids Present",
 			testCase: func(m *Metrics) {
 				performTest(m, pbsmetrics.CookieFlagUnknown, pbsmetrics.AdapterBidPresent)
 			},
-			expectedCount:         1,
-			expectedNoCookieCount: 0,
-			expectedHasBidsCount:  1,
+			expectedCount:              1,
+			expectedCookieNoCount:      0,
+			expectedCookieYesCount:     0,
+			expectedCookieUnknownCount: 1,
+			expectedHasBidsCount:       1,
 		},
 		{
 			description: "Has Cookie & Bids Present",
 			testCase: func(m *Metrics) {
 				performTest(m, pbsmetrics.CookieFlagYes, pbsmetrics.AdapterBidPresent)
 			},
-			expectedCount:         1,
-			expectedNoCookieCount: 0,
-			expectedHasBidsCount:  1,
+			expectedCount:              1,
+			expectedCookieNoCount:      0,
+			expectedCookieYesCount:     1,
+			expectedCookieUnknownCount: 0,
+			expectedHasBidsCount:       1,
 		},
 	}
 
@@ -555,7 +569,9 @@ func TestAdapterRequestMetrics(t *testing.T) {
 		test.testCase(m)
 
 		var totalCount float64
-		var totalNoCookieCount float64
+		var totalCookieNoCount float64
+		var totalCookieYesCount float64
+		var totalCookieUnknowmCount float64
 		var totalHasBidsCount float64
 		processMetrics(m.adapterRequests, func(m dto.Metric) {
 			isMetricForAdapter := false
@@ -569,19 +585,28 @@ func TestAdapterRequestMetrics(t *testing.T) {
 				value := m.GetCounter().GetValue()
 				totalCount += value
 				for _, label := range m.GetLabel() {
-					if label.GetValue() == "true" {
-						switch label.GetName() {
-						case noCookieLabel:
-							totalNoCookieCount += value
-						case hasBidsLabel:
-							totalHasBidsCount += value
+
+					if label.GetName() == hasBidsLabel && label.GetValue() == "true" {
+						totalHasBidsCount += value
+					}
+
+					if label.GetName() == cookieLabel {
+						switch label.GetValue() {
+						case string(pbsmetrics.CookieFlagNo):
+							totalCookieNoCount += value
+						case string(pbsmetrics.CookieFlagYes):
+							totalCookieYesCount += value
+						case string(pbsmetrics.CookieFlagUnknown):
+							totalCookieUnknowmCount += value
 						}
 					}
 				}
 			}
 		})
 		assert.Equal(t, test.expectedCount, totalCount, test.description+":total")
-		assert.Equal(t, test.expectedNoCookieCount, totalNoCookieCount, test.description+":noCookie")
+		assert.Equal(t, test.expectedCookieNoCount, totalCookieNoCount, test.description+":cookie=no")
+		assert.Equal(t, test.expectedCookieYesCount, totalCookieYesCount, test.description+":cookie=yes")
+		assert.Equal(t, test.expectedCookieUnknownCount, totalCookieUnknowmCount, test.description+":cookie=unknown")
 		assert.Equal(t, test.expectedHasBidsCount, totalHasBidsCount, test.description+":hasBids")
 	}
 }
@@ -744,6 +769,23 @@ func TestUserIDSetMetric(t *testing.T) {
 		})
 }
 
+func TestUserIDSetMetric_WhenBidderEmpty(t *testing.T) {
+	m := createMetricsForTesting()
+	action := pbsmetrics.RequestActionErr
+
+	m.RecordUserIDSet(pbsmetrics.UserLabels{
+		Bidder: openrtb_ext.BidderName(""),
+		Action: action,
+	})
+
+	expectedTotalCount := float64(0)
+	actualTotalCount := float64(0)
+	processMetrics(m.adapterUserSync, func(m dto.Metric) {
+		actualTotalCount += m.GetCounter().GetValue()
+	})
+	assert.Equal(t, expectedTotalCount, actualTotalCount, "total count")
+}
+
 func TestAdapterPanicMetric(t *testing.T) {
 	m := createMetricsForTesting()
 	adapterName := "anyName"

pbsmetrics/prometheus/type_conversion.go

@@ -41,6 +41,15 @@ func boolValuesAsString() []string {
 	}
 }
 
+func cookieTypesAsString() []string {
+	values := pbsmetrics.CookieTypes()
+	valuesAsString := make([]string, len(values))
+	for i, v := range values {
+		valuesAsString[i] = string(v)
+	}
+	return valuesAsString
+}
+
 func cacheResultsAsString() []string {
 	values := pbsmetrics.CacheResults()
 	valuesAsString := make([]string, len(values))