[coordinator] Fix inverted interpretation of Prometheus remote write shadow sampling config (#4210)

Author: robskillington

src/query/api/v1/handler/prometheus/remote/write.go

@@ -650,13 +650,15 @@ func (h *PromWriteHandler) buildForwardShadowRequestBody(
 
 		// Use a range of 10k to allow for setting 0.01% having an effect
 		// when shadow percent is set (i.e. with percent=0.0001)
-		if hash(buffer)%10000 <= uint64(shadowOpts.Percent*10000) {
-			// Keep this series, it falls below the volume target of shards.
-			h.metrics.forwardShadowKeep.Inc(1)
+		if hash(buffer)%10000 >= uint64(shadowOpts.Percent*10000) {
+			// Skip forwarding this series, not in shadow volume of shards.
+			// Swap it with the tail and continue.
+			h.metrics.forwardShadowDrop.Inc(1)
 			continue
 		}
 
-		h.metrics.forwardShadowDrop.Inc(1)
+		// Keep this series, it falls below the volume target of shards.
+		h.metrics.forwardShadowKeep.Inc(1)
 
 		// Skip forwarding this series, not in shadow volume of shards.
 		// Swap it with the tail and continue.

src/query/api/v1/handler/prometheus/remote/write_test.go

@@ -451,40 +451,38 @@ func TestPromWriteLiteralIsTooLongError(t *testing.T) {
 	}
 }
 
-func TestPromWriteForwardWithShadowDefaultHash(t *testing.T) {
-	testPromWriteForwardWithShadow(t, testPromWriteForwardWithShadowOptions{
-		numSeries:                    10000,
-		percent:                      0.5,
-		expectedFwded:                5000,
-		expectedFwdedAllowedVariance: 0.05,
-	})
-}
-
-func TestPromWriteForwardWithShadowXXHash(t *testing.T) {
-	testPromWriteForwardWithShadow(t, testPromWriteForwardWithShadowOptions{
-		numSeries:                    10000,
-		percent:                      0.5,
-		hash:                         "xxhash",
-		expectedFwded:                5000,
-		expectedFwdedAllowedVariance: 0.05,
-	})
-}
-
-func TestPromWriteForwardWithShadowMurmur3(t *testing.T) {
-	testPromWriteForwardWithShadow(t, testPromWriteForwardWithShadowOptions{
-		numSeries:                    10000,
-		percent:                      0.5,
-		hash:                         "murmur3",
-		expectedFwded:                5000,
-		expectedFwdedAllowedVariance: 0.05,
-	})
+func TestPromWriteForwardWithShadow(t *testing.T) {
+	for _, tt := range []struct {
+		percent         float64
+		numSeries       int
+		allowedVariance float64
+	}{
+		{0, 10000, 0},
+		{0.25, 10000, 0.05},
+		{0.5, 10000, 0.05},
+		{0.75, 10000, 0.05},
+		{1, 10000, 0},
+	} {
+		for _, h := range []string{"", "murmur3", "xxhash"} {
+			h := h
+			t.Run(fmt.Sprintf("hash='%s', params=%+v", h, tt), func(t *testing.T) {
+				testPromWriteForwardWithShadow(t, testPromWriteForwardWithShadowOptions{
+					hash:                         h,
+					numSeries:                    tt.numSeries,
+					percent:                      tt.percent,
+					expectedFwded:                int(float64(tt.numSeries) * tt.percent),
+					expectedFwdedAllowedVariance: tt.allowedVariance,
+				})
+			})
+		}
+	}
 }
 
 type testPromWriteForwardWithShadowOptions struct {
 	numSeries                    int
 	percent                      float64
 	hash                         string
-	expectedFwded                int64
+	expectedFwded                int
 	expectedFwdedAllowedVariance float64
 }
 
@@ -569,11 +567,17 @@ func testPromWriteForwardWithShadow(
 
 	select {
 	case fwdReq := <-forwardRecvReqCh:
-		assert.InEpsilon(t, testOpts.expectedFwded, len(fwdReq.Timeseries),
-			testOpts.expectedFwdedAllowedVariance,
-			fmt.Sprintf("expected=%v, actual=%v, allowed_variance=%v",
-				testOpts.expectedFwded, len(fwdReq.Timeseries),
-				testOpts.expectedFwdedAllowedVariance))
+		if testOpts.expectedFwdedAllowedVariance > 0 {
+			assert.InEpsilon(t, testOpts.expectedFwded, len(fwdReq.Timeseries),
+				testOpts.expectedFwdedAllowedVariance,
+				fmt.Sprintf("expected=%v, actual=%v, allowed_variance=%v",
+					testOpts.expectedFwded, len(fwdReq.Timeseries),
+					testOpts.expectedFwdedAllowedVariance))
+		} else {
+			assert.Equal(t, testOpts.expectedFwded, len(fwdReq.Timeseries),
+				fmt.Sprintf("expected=%v, actual=%v",
+					testOpts.expectedFwded, len(fwdReq.Timeseries)))
+		}
 	case <-time.After(10 * time.Second):
 		require.FailNow(t, "timeout waiting for fwd request")
 	}