From 4849e0a191ec00554bed304a6dd2f19cfc3a8060 Mon Sep 17 00:00:00 2001 From: Vindhya Ningegowda Date: Fri, 21 Jun 2024 17:06:55 -0700 Subject: [PATCH] core: Add label values size validation in MetricRecorder (#11306) Enhance MetricRecorder: Validate label values count against registered label keys count for default record APIs --- api/src/main/java/io/grpc/MetricRecorder.java | 59 +++++++++++++++++-- .../io/grpc/internal/MetricRecorderImpl.java | 50 ++++------------ 2 files changed, 63 insertions(+), 46 deletions(-) diff --git a/api/src/main/java/io/grpc/MetricRecorder.java b/api/src/main/java/io/grpc/MetricRecorder.java index f128d14e73..d418dcbf59 100644 --- 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 requiredLabelValues, List optionalLabelValues) {} + List requiredLabelValues, List 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 @@ public interface MetricRecorder { * @param optionalLabelValues A list of additional, optional label values for the metric. */ default void addLongCounter(LongCounterMetricInstrument metricInstrument, long value, - List requiredLabelValues, List optionalLabelValues) {} + List requiredLabelValues, List 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 @@ public interface MetricRecorder { * @param optionalLabelValues A list of additional, optional label values for the metric. */ default void recordDoubleHistogram(DoubleHistogramMetricInstrument metricInstrument, double value, - List requiredLabelValues, List optionalLabelValues) {} + List requiredLabelValues, List 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 @@ public interface MetricRecorder { * @param optionalLabelValues A list of additional, optional label values for the metric. */ default void recordLongHistogram(LongHistogramMetricInstrument metricInstrument, long value, - List requiredLabelValues, List optionalLabelValues) {} + List requiredLabelValues, List 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 @@ public interface MetricRecorder { * @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 requiredLabelValues, List optionalLabelValues); + default void recordLongGauge(LongGaugeMetricInstrument metricInstrument, long value, + List requiredLabelValues, List 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 index b771b5d055..452b1c5df0 100644 --- 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 requiredLabelValues, List 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 @@ final class MetricRecorderImpl implements MetricRecorder { @Override public void addLongCounter(LongCounterMetricInstrument metricInstrument, long value, List requiredLabelValues, List 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 @@ final class MetricRecorderImpl implements MetricRecorder { @Override public void recordDoubleHistogram(DoubleHistogramMetricInstrument metricInstrument, double value, List requiredLabelValues, List 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 @@ final class MetricRecorderImpl implements MetricRecorder { @Override public void recordLongHistogram(LongHistogramMetricInstrument metricInstrument, long value, List requiredLabelValues, List 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 @@ final class MetricRecorderImpl implements MetricRecorder { @Override public void recordLongGauge(LongGaugeMetricInstrument metricInstrument, long value, List requiredLabelValues, List 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); }