[client-common][router] minor cleanup and some tests for otel (#1369)

some minor cleanups and tests including
* Not initializing VeniceOpenTelemetryMetricsRepository when otel is not configured
* if setMetricEntities is not configured in VeniceMetricsConfig and if exponential bucket histogram is enabled (which is enabled by default), builder will fail fast.
* Disabling OTel exporting to endpoint in routers for integration tests
* Removing the explicit names passed in RouterMetricEntity and use the enum name itself to be less error prone
* some tests around these changes

internal/venice-client-common/src/main/java/com/linkedin/venice/stats/VeniceMetricsConfig.java

@@ -111,6 +111,10 @@ public class VeniceMetricsConfig {
 
   private final String serviceName;
   private final String metricPrefix;
+  /**
+   * List of all the metrics emitted by the service: Currently used to set Exponential Histogram view
+   * for instruments of type {@link com.linkedin.venice.stats.metrics.MetricType#HISTOGRAM}
+   */
   private final Collection<MetricEntity> metricEntities;
   /** reusing tehuti's MetricConfig */
   private final MetricConfig tehutiMetricConfig;
@@ -268,6 +272,11 @@ public Builder extractAndSetOtelConfigs(Map<String, String> configs) {
         setEmitOtelMetrics(Boolean.parseBoolean(configValue));
       }
 
+      if (!emitOtelMetrics) {
+        // Early return if OpenTelemetry metrics are disabled
+        return this;
+      }
+
       if ((configValue = configs.get(OTEL_VENICE_METRICS_PREFIX)) != null) {
         setMetricPrefix(configValue);
       }

internal/venice-client-common/src/main/java/com/linkedin/venice/stats/VeniceMetricsRepository.java

@@ -22,11 +22,14 @@ public class VeniceMetricsRepository extends MetricsRepository implements Closea
   public VeniceMetricsRepository() {
     super();
     this.veniceMetricsConfig = new VeniceMetricsConfig.Builder().build();
-    this.openTelemetryMetricsRepository = new VeniceOpenTelemetryMetricsRepository(veniceMetricsConfig);
+    this.openTelemetryMetricsRepository =
+        (veniceMetricsConfig.emitOtelMetrics() ? new VeniceOpenTelemetryMetricsRepository(veniceMetricsConfig) : null);
   }
 
   public VeniceMetricsRepository(VeniceMetricsConfig veniceMetricsConfig) {
-    this(veniceMetricsConfig, new VeniceOpenTelemetryMetricsRepository(veniceMetricsConfig));
+    this(
+        veniceMetricsConfig,
+        veniceMetricsConfig.emitOtelMetrics() ? new VeniceOpenTelemetryMetricsRepository(veniceMetricsConfig) : null);
   }
 
   public VeniceMetricsRepository(

internal/venice-client-common/src/main/java/com/linkedin/venice/stats/VeniceOpenTelemetryMetricNamingFormat.java

@@ -90,4 +90,8 @@ private static String capitalizeFirstLetter(String word) {
     }
     return Character.toUpperCase(word.charAt(0)) + word.substring(1);
   }
+
+  public static VeniceOpenTelemetryMetricNamingFormat getDefaultFormat() {
+    return SNAKE_CASE;
+  }
 }

internal/venice-client-common/src/main/java/com/linkedin/venice/stats/VeniceOpenTelemetryMetricsRepository.java

@@ -77,13 +77,18 @@ MetricExporter getOtlpHttpMetricExporter(VeniceMetricsConfig metricsConfig) {
    * instruments, so {@link OtlpHttpMetricExporterBuilder#setDefaultAggregationSelector} to enable exponential
    * histogram aggregation is not used here to not convert the histograms of type {@link MetricType#MIN_MAX_COUNT_SUM_AGGREGATIONS}
    * to exponential histograms to be able to follow explict boundaries.
+   *
+   * If the metric entities are empty, it will throw an exception. Failing fast here as
+   * 1. If we configure exponential histogram aggregation for every histogram: it could lead to increased memory usage
+   * 2. If we don't configure exponential histogram aggregation for every histogram: it could lead to observability miss
    */
   private void setExponentialHistogramAggregation(SdkMeterProviderBuilder builder, VeniceMetricsConfig metricsConfig) {
     List<String> metricNames = new ArrayList<>();
 
-    if (metricsConfig.getMetricEntities().isEmpty()) {
-      LOGGER
-          .warn("No metric entities found in config: {} to configure exponential histogram", metricsConfig.toString());
+    Collection<MetricEntity> metricEntities = metricsConfig.getMetricEntities();
+    if (metricEntities == null || metricsConfig.getMetricEntities().isEmpty()) {
+      throw new IllegalArgumentException(
+          "metricEntities cannot be empty if exponential Histogram is enabled, List all the metrics used in this service using setMetricEntities method");
     }
 
     for (MetricEntity metricEntity: metricsConfig.getMetricEntities()) {

internal/venice-client-common/src/main/java/com/linkedin/venice/utils/metrics/MetricsRepositoryUtils.java

@@ -2,9 +2,12 @@
 
 import com.linkedin.venice.stats.VeniceMetricsConfig;
 import com.linkedin.venice.stats.VeniceMetricsRepository;
+import com.linkedin.venice.stats.VeniceOpenTelemetryMetricNamingFormat;
+import com.linkedin.venice.stats.metrics.MetricEntity;
 import io.tehuti.metrics.MetricConfig;
 import io.tehuti.metrics.MetricsRepository;
 import io.tehuti.metrics.stats.AsyncGauge;
+import java.util.Collection;
 import java.util.concurrent.TimeUnit;
 
 
@@ -22,8 +25,46 @@ public static MetricsRepository createSingleThreadedMetricsRepository() {
     return createSingleThreadedMetricsRepository(TimeUnit.MINUTES.toMillis(1), 100);
   }
 
+  public static MetricsRepository createSingleThreadedMetricsRepository(
+      long maxMetricsMeasurementTimeoutMs,
+      long initialMetricsMeasurementTimeoutMs) {
+    return new MetricsRepository(getMetricConfig(maxMetricsMeasurementTimeoutMs, initialMetricsMeasurementTimeoutMs));
+  }
+
   public static MetricsRepository createSingleThreadedVeniceMetricsRepository() {
-    return createSingleThreadedVeniceMetricsRepository(TimeUnit.MINUTES.toMillis(1), 100);
+    return createSingleThreadedVeniceMetricsRepository(
+        TimeUnit.MINUTES.toMillis(1),
+        100,
+        false,
+        VeniceOpenTelemetryMetricNamingFormat.getDefaultFormat(),
+        null);
+  }
+
+  public static MetricsRepository createSingleThreadedVeniceMetricsRepository(
+      boolean isOtelEnabled,
+      VeniceOpenTelemetryMetricNamingFormat otelFormat,
+      Collection<MetricEntity> metricEntities) {
+    return createSingleThreadedVeniceMetricsRepository(
+        TimeUnit.MINUTES.toMillis(1),
+        100,
+        isOtelEnabled,
+        otelFormat,
+        metricEntities);
+  }
+
+  public static MetricsRepository createSingleThreadedVeniceMetricsRepository(
+      long maxMetricsMeasurementTimeoutMs,
+      long initialMetricsMeasurementTimeoutMs,
+      boolean isOtelEnabled,
+      VeniceOpenTelemetryMetricNamingFormat otelFormat,
+      Collection<MetricEntity> metricEntities) {
+
+    return new VeniceMetricsRepository(
+        new VeniceMetricsConfig.Builder().setEmitOtelMetrics(isOtelEnabled)
+            .setMetricEntities(metricEntities)
+            .setMetricNamingFormat(otelFormat)
+            .setTehutiMetricConfig(getMetricConfig(maxMetricsMeasurementTimeoutMs, initialMetricsMeasurementTimeoutMs))
+            .build());
   }
 
   public static MetricConfig getMetricConfig(
@@ -36,19 +77,4 @@ public static MetricConfig getMetricConfig(
             .setMaxMetricsMeasurementTimeoutInMs(maxMetricsMeasurementTimeoutMs)
             .build());
   }
-
-  public static MetricsRepository createSingleThreadedMetricsRepository(
-      long maxMetricsMeasurementTimeoutMs,
-      long initialMetricsMeasurementTimeoutMs) {
-    return new MetricsRepository(getMetricConfig(maxMetricsMeasurementTimeoutMs, initialMetricsMeasurementTimeoutMs));
-  }
-
-  public static MetricsRepository createSingleThreadedVeniceMetricsRepository(
-      long maxMetricsMeasurementTimeoutMs,
-      long initialMetricsMeasurementTimeoutMs) {
-    return new VeniceMetricsRepository(
-        new VeniceMetricsConfig.Builder()
-            .setTehutiMetricConfig(getMetricConfig(maxMetricsMeasurementTimeoutMs, initialMetricsMeasurementTimeoutMs))
-            .build());
-  }
 }

internal/venice-client-common/src/test/java/com/linkedin/venice/stats/VeniceMetricsConfigTest.java

@@ -11,6 +11,7 @@
 import static com.linkedin.venice.stats.VeniceMetricsConfig.OTEL_VENICE_METRICS_EXPORT_TO_ENDPOINT;
 import static com.linkedin.venice.stats.VeniceMetricsConfig.OTEL_VENICE_METRICS_EXPORT_TO_LOG;
 import static com.linkedin.venice.stats.VeniceMetricsConfig.OTEL_VENICE_METRICS_NAMING_FORMAT;
+import static com.linkedin.venice.stats.VeniceOpenTelemetryMetricNamingFormat.SNAKE_CASE;
 import static org.testng.Assert.assertEquals;
 import static org.testng.Assert.assertFalse;
 import static org.testng.Assert.assertNotNull;
@@ -42,7 +43,7 @@ public void testDefaultValuesWithBasicConfig() {
     assertEquals(config.getOtelEndpoint(), null);
     assertTrue(config.getOtelHeaders().isEmpty());
     assertFalse(config.exportOtelMetricsToLog());
-    assertEquals(config.getMetricNamingFormat(), VeniceOpenTelemetryMetricNamingFormat.SNAKE_CASE);
+    assertEquals(config.getMetricNamingFormat(), SNAKE_CASE);
     assertEquals(config.getOtelAggregationTemporalitySelector(), AggregationTemporalitySelector.deltaPreferred());
     assertEquals(config.useOtelExponentialHistogram(), true);
     assertEquals(config.getOtelExponentialHistogramMaxScale(), 3);
@@ -83,9 +84,10 @@ public void testOtelMissingConfigs() {
         .build();
   }
 
-  @Test(expectedExceptions = IllegalArgumentException.class)
+  @Test(expectedExceptions = IllegalArgumentException.class, expectedExceptionsMessageRegExp = "No enum constant com.linkedin.venice.stats.VeniceOpenTelemetryMetricNamingFormat.INVALID_FORMAT")
   public void testOtelConfigWithInvalidMetricFormat() {
     Map<String, String> otelConfigs = new HashMap<>();
+    otelConfigs.put(OTEL_VENICE_METRICS_ENABLED, "true");
     otelConfigs.put(OTEL_VENICE_METRICS_NAMING_FORMAT, "INVALID_FORMAT");
 
     new Builder().extractAndSetOtelConfigs(otelConfigs).build();

internal/venice-client-common/src/test/java/com/linkedin/venice/stats/VeniceMetricsRepositoryTest.java

@@ -1,19 +1,66 @@
 package com.linkedin.venice.stats;
 
 import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertFalse;
 import static org.testng.Assert.assertNotNull;
+import static org.testng.Assert.assertNull;
+import static org.testng.Assert.assertTrue;
 
+import com.linkedin.venice.stats.metrics.MetricEntity;
+import com.linkedin.venice.stats.metrics.MetricType;
+import com.linkedin.venice.stats.metrics.MetricUnit;
 import io.tehuti.metrics.MetricConfig;
+import java.util.ArrayList;
+import java.util.Collection;
 import org.mockito.Mockito;
 import org.testng.annotations.Test;
 
 
 public class VeniceMetricsRepositoryTest {
   @Test
-  public void testDefaultConstructor() throws Exception {
+  public void testDefaultConstructor() {
     VeniceMetricsRepository repository = new VeniceMetricsRepository();
     assertNotNull(repository.getVeniceMetricsConfig(), "VeniceMetricsConfig should not be null.");
-    assertNotNull(repository.getOpenTelemetryMetricsRepository(), "OpenTelemetryMetricsRepository should not be null.");
+    assertFalse(repository.getVeniceMetricsConfig().emitOtelMetrics());
+    assertNull(
+        repository.getOpenTelemetryMetricsRepository(),
+        "OpenTelemetryMetricsRepository should be null if not enabled explicitly");
+    repository.close();
+  }
+
+  @Test
+  public void testConstructorWithMetricConfig() {
+    VeniceMetricsConfig metricsConfig = new VeniceMetricsConfig.Builder().build();
+    VeniceMetricsRepository repository = new VeniceMetricsRepository(metricsConfig);
+    assertFalse(metricsConfig.emitOtelMetrics());
+
+    assertEquals(
+        repository.getVeniceMetricsConfig(),
+        metricsConfig,
+        "VeniceMetricsConfig should match the provided config.");
+    assertNull(
+        repository.getOpenTelemetryMetricsRepository(),
+        "OpenTelemetryMetricsRepository should be null if not enabled explicitly");
+    repository.close();
+  }
+
+  @Test
+  public void testConstructorWithMetricConfigAndOtelEnabled() {
+    Collection<MetricEntity> metricEntities = new ArrayList<>();
+    metricEntities
+        .add(new MetricEntity("test_metric", MetricType.HISTOGRAM, MetricUnit.MILLISECOND, "Test description"));
+    VeniceMetricsConfig metricsConfig =
+        new VeniceMetricsConfig.Builder().setEmitOtelMetrics(true).setMetricEntities(metricEntities).build();
+    VeniceMetricsRepository repository = new VeniceMetricsRepository(metricsConfig);
+
+    assertEquals(
+        repository.getVeniceMetricsConfig(),
+        metricsConfig,
+        "VeniceMetricsConfig should match the provided config.");
+    assertTrue(metricsConfig.emitOtelMetrics());
+    assertNotNull(
+        repository.getOpenTelemetryMetricsRepository(),
+        "OpenTelemetryMetricsRepository should not be null if enabled explicitly");
     repository.close();
   }
 

internal/venice-client-common/src/test/java/com/linkedin/venice/stats/VeniceOpenTelemetryMetricsRepositoryTest.java

@@ -1,18 +1,23 @@
 package com.linkedin.venice.stats;
 
+import static com.linkedin.venice.stats.VeniceOpenTelemetryMetricNamingFormat.SNAKE_CASE;
 import static com.linkedin.venice.stats.VeniceOpenTelemetryMetricNamingFormat.transformMetricName;
 import static com.linkedin.venice.stats.VeniceOpenTelemetryMetricNamingFormat.validateMetricName;
+import static org.mockito.Mockito.when;
 import static org.testng.Assert.assertEquals;
 import static org.testng.Assert.assertNotNull;
 import static org.testng.Assert.assertNull;
 import static org.testng.Assert.assertSame;
+import static org.testng.Assert.fail;
 
+import com.linkedin.venice.exceptions.VeniceException;
 import com.linkedin.venice.stats.metrics.MetricEntity;
 import com.linkedin.venice.stats.metrics.MetricType;
 import com.linkedin.venice.stats.metrics.MetricUnit;
 import io.opentelemetry.api.metrics.DoubleHistogram;
 import io.opentelemetry.api.metrics.LongCounter;
 import io.opentelemetry.sdk.metrics.export.MetricExporter;
+import java.util.ArrayList;
 import org.mockito.Mockito;
 import org.testng.annotations.AfterMethod;
 import org.testng.annotations.BeforeMethod;
@@ -27,13 +32,12 @@ public class VeniceOpenTelemetryMetricsRepositoryTest {
   @BeforeMethod
   public void setUp() {
     mockMetricsConfig = Mockito.mock(VeniceMetricsConfig.class);
-    Mockito.when(mockMetricsConfig.emitOtelMetrics()).thenReturn(true);
-    Mockito.when(mockMetricsConfig.getMetricNamingFormat())
-        .thenReturn(VeniceOpenTelemetryMetricNamingFormat.SNAKE_CASE);
-    Mockito.when(mockMetricsConfig.getMetricPrefix()).thenReturn("test_prefix");
-    Mockito.when(mockMetricsConfig.getServiceName()).thenReturn("test_service");
-    Mockito.when(mockMetricsConfig.exportOtelMetricsToEndpoint()).thenReturn(true);
-    Mockito.when(mockMetricsConfig.getOtelEndpoint()).thenReturn("http://localhost:4318");
+    when(mockMetricsConfig.emitOtelMetrics()).thenReturn(true);
+    when(mockMetricsConfig.getMetricNamingFormat()).thenReturn(SNAKE_CASE);
+    when(mockMetricsConfig.getMetricPrefix()).thenReturn("test_prefix");
+    when(mockMetricsConfig.getServiceName()).thenReturn("test_service");
+    when(mockMetricsConfig.exportOtelMetricsToEndpoint()).thenReturn(true);
+    when(mockMetricsConfig.getOtelEndpoint()).thenReturn("http://localhost:4318");
 
     metricsRepository = new VeniceOpenTelemetryMetricsRepository(mockMetricsConfig);
   }
@@ -52,7 +56,7 @@ public void testConstructorInitialize() {
 
   @Test
   public void testConstructorWithEmitDisabled() {
-    Mockito.when(mockMetricsConfig.emitOtelMetrics()).thenReturn(false);
+    when(mockMetricsConfig.emitOtelMetrics()).thenReturn(false);
     VeniceOpenTelemetryMetricsRepository metricsRepository =
         new VeniceOpenTelemetryMetricsRepository(mockMetricsConfig);
 
@@ -90,8 +94,7 @@ public void testValidateMetricNameWithInvalidName() {
 
   @Test
   public void testTransformMetricName() {
-    Mockito.when(mockMetricsConfig.getMetricNamingFormat())
-        .thenReturn(VeniceOpenTelemetryMetricNamingFormat.SNAKE_CASE);
+    when(mockMetricsConfig.getMetricNamingFormat()).thenReturn(SNAKE_CASE);
     assertEquals(metricsRepository.getFullMetricName("prefix", "metric_name"), "prefix.metric_name");
 
     String transformedName =
@@ -123,4 +126,33 @@ public void testCreateTwoCounters() {
     assertNotNull(counter1);
     assertSame(counter1, counter2, "Should return the same instance for the same counter name.");
   }
+
+  @Test
+  public void testRepositoryCreationWithoutSetMetricEntities() {
+    when(mockMetricsConfig.useOtelExponentialHistogram()).thenReturn(true);
+    when(mockMetricsConfig.getMetricEntities()).thenReturn(null);
+    try {
+      new VeniceOpenTelemetryMetricsRepository(mockMetricsConfig);
+      fail();
+    } catch (VeniceException e) {
+      // Verify that the exception message is correct
+      assertEquals(
+          e.getCause().getMessage(),
+          "metricEntities cannot be empty if exponential Histogram is enabled, List all the metrics used in this service using setMetricEntities method");
+    }
+
+    when(mockMetricsConfig.getMetricEntities()).thenReturn(new ArrayList<>());
+    try {
+      new VeniceOpenTelemetryMetricsRepository(mockMetricsConfig);
+      fail();
+    } catch (VeniceException e) {
+      // Verify that the exception message is correct
+      assertEquals(
+          e.getCause().getMessage(),
+          "metricEntities cannot be empty if exponential Histogram is enabled, List all the metrics used in this service using setMetricEntities method");
+    }
+
+    when(mockMetricsConfig.useOtelExponentialHistogram()).thenReturn(false);
+    new VeniceOpenTelemetryMetricsRepository(mockMetricsConfig);
+  }
 }

internal/venice-client-common/src/test/java/com/linkedin/venice/stats/dimensions/VeniceMetricsDimensionsTest.java

@@ -1,5 +1,6 @@
 package com.linkedin.venice.stats.dimensions;
 
+import static com.linkedin.venice.stats.VeniceOpenTelemetryMetricNamingFormat.SNAKE_CASE;
 import static org.testng.Assert.assertEquals;
 
 import com.linkedin.venice.stats.VeniceOpenTelemetryMetricNamingFormat;
@@ -9,7 +10,7 @@
 public class VeniceMetricsDimensionsTest {
   @Test
   public void testGetDimensionNameInSnakeCase() {
-    VeniceOpenTelemetryMetricNamingFormat format = VeniceOpenTelemetryMetricNamingFormat.SNAKE_CASE;
+    VeniceOpenTelemetryMetricNamingFormat format = SNAKE_CASE;
     for (VeniceMetricsDimensions dimension: VeniceMetricsDimensions.values()) {
       switch (dimension) {
         case VENICE_STORE_NAME:

internal/venice-test-common/src/integrationTest/java/com/linkedin/venice/integration/utils/VeniceRouterWrapper.java

@@ -26,12 +26,9 @@
 import static com.linkedin.venice.stats.VeniceMetricsConfig.OTEL_EXPORTER_OTLP_METRICS_DEFAULT_HISTOGRAM_AGGREGATION;
 import static com.linkedin.venice.stats.VeniceMetricsConfig.OTEL_EXPORTER_OTLP_METRICS_DEFAULT_HISTOGRAM_AGGREGATION_MAX_BUCKETS;
 import static com.linkedin.venice.stats.VeniceMetricsConfig.OTEL_EXPORTER_OTLP_METRICS_DEFAULT_HISTOGRAM_AGGREGATION_MAX_SCALE;
-import static com.linkedin.venice.stats.VeniceMetricsConfig.OTEL_EXPORTER_OTLP_METRICS_ENDPOINT;
 import static com.linkedin.venice.stats.VeniceMetricsConfig.OTEL_EXPORTER_OTLP_METRICS_PROTOCOL;
 import static com.linkedin.venice.stats.VeniceMetricsConfig.OTEL_EXPORTER_OTLP_METRICS_TEMPORALITY_PREFERENCE;
 import static com.linkedin.venice.stats.VeniceMetricsConfig.OTEL_VENICE_METRICS_ENABLED;
-import static com.linkedin.venice.stats.VeniceMetricsConfig.OTEL_VENICE_METRICS_EXPORT_TO_ENDPOINT;
-import static com.linkedin.venice.stats.VeniceMetricsConfig.OTEL_VENICE_METRICS_EXPORT_TO_LOG;
 
 import com.linkedin.venice.client.store.ClientConfig;
 import com.linkedin.venice.helix.HelixBaseRoutingRepository;
@@ -165,10 +162,7 @@ static StatefulServiceProvider<VeniceRouterWrapper> generateService(
           .put(ROUTER_STORAGE_NODE_CLIENT_TYPE, StorageNodeClientType.APACHE_HTTP_ASYNC_CLIENT.name())
           // OpenTelemetry configs
           .put(OTEL_VENICE_METRICS_ENABLED, Boolean.TRUE.toString())
-          .put(OTEL_VENICE_METRICS_EXPORT_TO_LOG, Boolean.TRUE.toString())
-          .put(OTEL_VENICE_METRICS_EXPORT_TO_ENDPOINT, Boolean.TRUE.toString())
           .put(OTEL_EXPORTER_OTLP_METRICS_PROTOCOL, "http/protobuf")
-          .put(OTEL_EXPORTER_OTLP_METRICS_ENDPOINT, "http://localhost:4318/v1/metrics")
           .put(OTEL_EXPORTER_OTLP_METRICS_TEMPORALITY_PREFERENCE, "delta")
           .put(OTEL_EXPORTER_OTLP_METRICS_DEFAULT_HISTOGRAM_AGGREGATION, "base2_exponential_bucket_histogram")
           .put(OTEL_EXPORTER_OTLP_METRICS_DEFAULT_HISTOGRAM_AGGREGATION_MAX_SCALE, 3)