core: Add label values size validation in MetricRecorder (grpc#11306)

Enhance MetricRecorder: Validate label values count against registered label keys count for default record APIs
matthewstevenson88 · Jun 22, 2024 · 4849e0a · 4849e0a
1 parent 1eec145
commit 4849e0a
Show file tree

Hide file tree

Showing 2 changed files with 63 additions and 46 deletions.
diff --git a/api/src/main/java/io/grpc/MetricRecorder.java b/api/src/main/java/io/grpc/MetricRecorder.java
@@ -16,6 +16,8 @@
 
 package io.grpc;
 
+import static com.google.common.base.Preconditions.checkArgument;
+
 import java.util.List;
 
 /**
@@ -33,7 +35,16 @@ public interface MetricRecorder {
    * @param optionalLabelValues A list of additional, optional label values for the metric.
    */
   default void addDoubleCounter(DoubleCounterMetricInstrument metricInstrument, double value,
-      List<String> requiredLabelValues, List<String> optionalLabelValues) {}
+      List<String> requiredLabelValues, List<String> optionalLabelValues) {
+    checkArgument(requiredLabelValues != null
+            && requiredLabelValues.size() == metricInstrument.getRequiredLabelKeys().size(),
+        "Incorrect number of required labels provided. Expected: %s",
+        metricInstrument.getRequiredLabelKeys().size());
+    checkArgument(optionalLabelValues != null
+            && optionalLabelValues.size() == metricInstrument.getOptionalLabelKeys().size(),
+        "Incorrect number of optional labels provided. Expected: %s",
+        metricInstrument.getOptionalLabelKeys().size());
+  }
 
   /**
    * Adds a value for a long valued counter metric instrument.
@@ -44,7 +55,16 @@ default void addDoubleCounter(DoubleCounterMetricInstrument metricInstrument, do
    * @param optionalLabelValues A list of additional, optional label values for the metric.
    */
   default void addLongCounter(LongCounterMetricInstrument metricInstrument, long value,
-      List<String> requiredLabelValues, List<String> optionalLabelValues) {}
+      List<String> requiredLabelValues, List<String> optionalLabelValues) {
+    checkArgument(requiredLabelValues != null
+            && requiredLabelValues.size() == metricInstrument.getRequiredLabelKeys().size(),
+        "Incorrect number of required labels provided. Expected: %s",
+        metricInstrument.getRequiredLabelKeys().size());
+    checkArgument(optionalLabelValues != null
+            && optionalLabelValues.size() == metricInstrument.getOptionalLabelKeys().size(),
+        "Incorrect number of optional labels provided. Expected: %s",
+        metricInstrument.getOptionalLabelKeys().size());
+  }
 
   /**
    * Records a value for a double-precision histogram metric instrument.
@@ -55,7 +75,16 @@ default void addLongCounter(LongCounterMetricInstrument metricInstrument, long v
    * @param optionalLabelValues A list of additional, optional label values for the metric.
    */
   default void recordDoubleHistogram(DoubleHistogramMetricInstrument metricInstrument, double value,
-      List<String> requiredLabelValues, List<String> optionalLabelValues) {}
+      List<String> requiredLabelValues, List<String> optionalLabelValues) {
+    checkArgument(requiredLabelValues != null
+            && requiredLabelValues.size() == metricInstrument.getRequiredLabelKeys().size(),
+        "Incorrect number of required labels provided. Expected: %s",
+        metricInstrument.getRequiredLabelKeys().size());
+    checkArgument(optionalLabelValues != null
+            && optionalLabelValues.size() == metricInstrument.getOptionalLabelKeys().size(),
+        "Incorrect number of optional labels provided. Expected: %s",
+        metricInstrument.getOptionalLabelKeys().size());
+  }
 
   /**
    * Records a value for a long valued histogram metric instrument.
@@ -66,7 +95,16 @@ default void recordDoubleHistogram(DoubleHistogramMetricInstrument metricInstrum
    * @param optionalLabelValues A list of additional, optional label values for the metric.
    */
   default void recordLongHistogram(LongHistogramMetricInstrument metricInstrument, long value,
-      List<String> requiredLabelValues, List<String> optionalLabelValues) {}
+      List<String> requiredLabelValues, List<String> optionalLabelValues) {
+    checkArgument(requiredLabelValues != null
+            && requiredLabelValues.size() == metricInstrument.getRequiredLabelKeys().size(),
+        "Incorrect number of required labels provided. Expected: %s",
+        metricInstrument.getRequiredLabelKeys().size());
+    checkArgument(optionalLabelValues != null
+            && optionalLabelValues.size() == metricInstrument.getOptionalLabelKeys().size(),
+        "Incorrect number of optional labels provided. Expected: %s",
+        metricInstrument.getOptionalLabelKeys().size());
+  }
 
   /**
    * Registers a callback to produce metric values for only the listed instruments. The returned
@@ -95,8 +133,17 @@ interface BatchRecorder {
      * @param requiredLabelValues A list of required label values for the metric.
      * @param optionalLabelValues A list of additional, optional label values for the metric.
      */
-    void recordLongGauge(LongGaugeMetricInstrument metricInstrument, long value,
-        List<String> requiredLabelValues, List<String> optionalLabelValues);
+    default void recordLongGauge(LongGaugeMetricInstrument metricInstrument, long value,
+        List<String> requiredLabelValues, List<String> optionalLabelValues) {
+      checkArgument(requiredLabelValues != null
+              && requiredLabelValues.size() == metricInstrument.getRequiredLabelKeys().size(),
+          "Incorrect number of required labels provided. Expected: %s",
+          metricInstrument.getRequiredLabelKeys().size());
+      checkArgument(optionalLabelValues != null
+              && optionalLabelValues.size() == metricInstrument.getOptionalLabelKeys().size(),
+          "Incorrect number of optional labels provided. Expected: %s",
+          metricInstrument.getOptionalLabelKeys().size());
+    }
   }
 
   /** A handle to a registration, that allows unregistration. */

diff --git a/core/src/main/java/io/grpc/internal/MetricRecorderImpl.java b/core/src/main/java/io/grpc/internal/MetricRecorderImpl.java
@@ -63,14 +63,8 @@ final class MetricRecorderImpl implements MetricRecorder {
   @Override
   public void addDoubleCounter(DoubleCounterMetricInstrument metricInstrument, double value,
       List<String> requiredLabelValues, List<String> optionalLabelValues) {
-    checkArgument(requiredLabelValues != null
-            && requiredLabelValues.size() == metricInstrument.getRequiredLabelKeys().size(),
-        "Incorrect number of required labels provided. Expected: "
-            + metricInstrument.getRequiredLabelKeys().size());
-    checkArgument(optionalLabelValues != null
-            && optionalLabelValues.size() == metricInstrument.getOptionalLabelKeys().size(),
-        "Incorrect number of optional labels provided. Expected: "
-            + metricInstrument.getOptionalLabelKeys().size());
+    MetricRecorder.super.addDoubleCounter(metricInstrument, value, requiredLabelValues,
+        optionalLabelValues);
     for (MetricSink sink : metricSinks) {
       // TODO(dnvindhya): Move updating measures logic from sink to here
       int measuresSize = sink.getMeasuresSize();
@@ -95,14 +89,8 @@ public void addDoubleCounter(DoubleCounterMetricInstrument metricInstrument, dou
   @Override
   public void addLongCounter(LongCounterMetricInstrument metricInstrument, long value,
       List<String> requiredLabelValues, List<String> optionalLabelValues) {
-    checkArgument(requiredLabelValues != null
-            && requiredLabelValues.size() == metricInstrument.getRequiredLabelKeys().size(),
-        "Incorrect number of required labels provided. Expected: "
-            + metricInstrument.getRequiredLabelKeys().size());
-    checkArgument(optionalLabelValues != null
-            && optionalLabelValues.size() == metricInstrument.getOptionalLabelKeys().size(),
-        "Incorrect number of optional labels provided. Expected: "
-            + metricInstrument.getOptionalLabelKeys().size());
+    MetricRecorder.super.addLongCounter(metricInstrument, value, requiredLabelValues,
+        optionalLabelValues);
     for (MetricSink sink : metricSinks) {
       int measuresSize = sink.getMeasuresSize();
       if (measuresSize <= metricInstrument.getIndex()) {
@@ -126,14 +114,8 @@ public void addLongCounter(LongCounterMetricInstrument metricInstrument, long va
   @Override
   public void recordDoubleHistogram(DoubleHistogramMetricInstrument metricInstrument, double value,
       List<String> requiredLabelValues, List<String> optionalLabelValues) {
-    checkArgument(requiredLabelValues != null
-            && requiredLabelValues.size() == metricInstrument.getRequiredLabelKeys().size(),
-        "Incorrect number of required labels provided. Expected: "
-            + metricInstrument.getRequiredLabelKeys().size());
-    checkArgument(optionalLabelValues != null
-            && optionalLabelValues.size() == metricInstrument.getOptionalLabelKeys().size(),
-        "Incorrect number of optional labels provided. Expected: "
-            + metricInstrument.getOptionalLabelKeys().size());
+    MetricRecorder.super.recordDoubleHistogram(metricInstrument, value, requiredLabelValues,
+        optionalLabelValues);
     for (MetricSink sink : metricSinks) {
       int measuresSize = sink.getMeasuresSize();
       if (measuresSize <= metricInstrument.getIndex()) {
@@ -157,14 +139,8 @@ public void recordDoubleHistogram(DoubleHistogramMetricInstrument metricInstrume
   @Override
   public void recordLongHistogram(LongHistogramMetricInstrument metricInstrument, long value,
       List<String> requiredLabelValues, List<String> optionalLabelValues) {
-    checkArgument(requiredLabelValues != null
-            && requiredLabelValues.size() == metricInstrument.getRequiredLabelKeys().size(),
-        "Incorrect number of required labels provided. Expected: "
-            + metricInstrument.getRequiredLabelKeys().size());
-    checkArgument(optionalLabelValues != null
-            && optionalLabelValues.size() == metricInstrument.getOptionalLabelKeys().size(),
-        "Incorrect number of optional labels provided. Expected: "
-            + metricInstrument.getOptionalLabelKeys().size());
+    MetricRecorder.super.recordLongHistogram(metricInstrument, value, requiredLabelValues,
+        optionalLabelValues);
     for (MetricSink sink : metricSinks) {
       int measuresSize = sink.getMeasuresSize();
       if (measuresSize <= metricInstrument.getIndex()) {
@@ -220,16 +196,10 @@ static class BatchRecorderImpl implements BatchRecorder {
     @Override
     public void recordLongGauge(LongGaugeMetricInstrument metricInstrument, long value,
         List<String> requiredLabelValues, List<String> optionalLabelValues) {
+      BatchRecorder.super.recordLongGauge(metricInstrument, value, requiredLabelValues,
+          optionalLabelValues);
       checkArgument(allowedInstruments.get(metricInstrument.getIndex()),
           "Instrument was not listed when registering callback: %s", metricInstrument);
-      checkArgument(requiredLabelValues != null
-              && requiredLabelValues.size() == metricInstrument.getRequiredLabelKeys().size(),
-          "Incorrect number of required labels provided. Expected: %s",
-          metricInstrument.getRequiredLabelKeys().size());
-      checkArgument(optionalLabelValues != null
-              && optionalLabelValues.size() == metricInstrument.getOptionalLabelKeys().size(),
-          "Incorrect number of optional labels provided. Expected: %s",
-          metricInstrument.getOptionalLabelKeys().size());
       // Registering the callback checked that the instruments were be present in sink.
       sink.recordLongGauge(metricInstrument, value, requiredLabelValues, optionalLabelValues);
     }