Make ServiceMetricEvent immutable, do not allow null dimension values

indexing-hadoop/src/test/java/org/apache/druid/indexer/HadoopTaskTest.java

@@ -58,7 +58,7 @@ public void testBuildClassLoader() throws Exception
       @Override
       public String getType()
       {
-        return null;
+        return "hadoop-test";
       }
 
       @Override

indexing-service/src/test/java/org/apache/druid/indexing/common/task/IndexTaskUtilsTest.java

@@ -38,7 +38,6 @@ public class IndexTaskUtilsTest
   private static final Map<String, Object> METRIC_TAGS = ImmutableMap.of("k1", "v1", "k2", 20);
 
   private static final String GROUP_ID = "groupId123";
-  @Mock
   private Task task;
   @Mock
   private AbstractTask abstractTask;
@@ -48,10 +47,13 @@ public class IndexTaskUtilsTest
   public void setUp()
   {
     metricBuilder = ServiceMetricEvent.builder();
-    Mockito.when(task.getContextValue(DruidMetrics.TAGS)).thenReturn(METRIC_TAGS);
-    Mockito.when(task.getGroupId()).thenReturn(GROUP_ID);
+    task = new NoopTask(GROUP_ID, GROUP_ID, "wiki", 1L, 0L, Map.of(DruidMetrics.TAGS, METRIC_TAGS));
     Mockito.when(abstractTask.getContextValue(DruidMetrics.TAGS)).thenReturn(METRIC_TAGS);
     Mockito.when(abstractTask.getGroupId()).thenReturn(GROUP_ID);
+    Mockito.when(abstractTask.getId()).thenReturn(GROUP_ID);
+    Mockito.when(abstractTask.getType()).thenReturn("test");
+    Mockito.when(abstractTask.getDataSource()).thenReturn("wiki");
+    Mockito.when(abstractTask.getIngestionMode()).thenReturn(AbstractTask.IngestionMode.APPEND);
   }
 
   @Test
@@ -71,7 +73,7 @@ public void testSetTaskDimensionsForAbstractTaskWithContextTagsShouldSetTags()
   @Test
   public void testSetTaskDimensionsWithoutTagsShouldNotSetTags()
   {
-    Mockito.when(task.getContextValue(DruidMetrics.TAGS)).thenReturn(null);
+    task = new NoopTask(null, null, "wiki", 1L, 0L, null);
     IndexTaskUtils.setTaskDimensions(metricBuilder, task);
     Assert.assertNull(metricBuilder.getDimension(DruidMetrics.TAGS));
   }
@@ -91,14 +93,6 @@ public void testSetTaskDimensionsWithGroupIdShouldSetGroupId()
     Assert.assertEquals(GROUP_ID, metricBuilder.getDimension(DruidMetrics.GROUP_ID));
   }
 
-  @Test
-  public void testSetTaskDimensionsWithoutGroupIdShouldNotSetGroupId()
-  {
-    Mockito.when(task.getGroupId()).thenReturn(null);
-    IndexTaskUtils.setTaskDimensions(metricBuilder, task);
-    Assert.assertNull(metricBuilder.getDimension(DruidMetrics.GROUP_ID));
-  }
-
   @Test
   public void testSetTaskDimensionsForAbstractTaskWithGroupIdShouldSetGroupId()
   {

indexing-service/src/test/java/org/apache/druid/indexing/seekablestream/SeekableStreamIndexTaskRunnerAuthTest.java

@@ -381,7 +381,7 @@ public TestSeekableStreamIndexTask(
     @Override
     public String getType()
     {
-      return null;
+      return "test";
     }
 
     @Override

processing/src/main/java/org/apache/druid/java/util/emitter/service/ServiceMetricEvent.java

@@ -22,9 +22,9 @@
 import com.fasterxml.jackson.annotation.JsonValue;
 import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableMap;
-import com.google.common.collect.Maps;
 import org.apache.druid.guice.annotations.PublicApi;
 import org.apache.druid.java.util.common.DateTimes;
+import org.apache.druid.java.util.common.IAE;
 import org.apache.druid.java.util.common.ISE;
 import org.apache.druid.java.util.emitter.core.Event;
 import org.apache.druid.java.util.emitter.core.EventMap;
@@ -35,7 +35,7 @@
 import java.util.TreeMap;
 
 /**
- *
+ * Immutable metric event emitted by a Druid {@link ServiceEmitter}.
  */
 @PublicApi
 public class ServiceMetricEvent implements Event
@@ -47,15 +47,18 @@ public static Builder builder()
 
   private final DateTime createdTime;
   private final ImmutableMap<String, String> serviceDims;
-  private final Map<String, Object> userDims;
+  private final ImmutableMap<String, Object> userDims;
   private final String feed;
   private final String metric;
   private final Number value;
 
+  /**
+   * Creates an immutable metric event.
+   */
   private ServiceMetricEvent(
       DateTime createdTime,
       ImmutableMap<String, String> serviceDims,
-      Map<String, Object> userDims,
+      ImmutableMap<String, Object> userDims,
       String feed,
       String metric,
       Number value
@@ -92,7 +95,7 @@ public String getHost()
 
   public Map<String, Object> getUserDims()
   {
-    return ImmutableMap.copyOf(userDims);
+    return userDims;
   }
 
   public String getMetric()
@@ -116,23 +119,10 @@ public EventMap toMap()
         .putAll(serviceDims)
         .put("metric", metric)
         .put("value", value)
-        .putAll(
-            Maps.filterEntries(
-                userDims,
-                input -> input.getKey() != null
-            )
-        )
+        .putAll(userDims)
         .build();
   }
 
-  /**
-   * Creates an immutable copy of this metric event. This is used only in tests.
-   */
-  public ServiceMetricEvent copy()
-  {
-    return new ServiceMetricEvent(createdTime, serviceDims, Map.copyOf(userDims), feed, metric, value);
-  }
-
   /**
    * Builder for a {@link ServiceMetricEvent}. This builder can be used for
    * building only one event.
@@ -153,20 +143,38 @@ public Builder setFeed(String feed)
 
     public Builder setDimension(String dim, String[] values)
     {
+      if (dim == null) {
+        throw new IAE("Dimension name cannot be null");
+      }
+
       userDims.put(dim, Arrays.asList(values));
       return this;
     }
 
+    /**
+     * Adds a dimension to be emitted with this metric event, only if the given
+     * value is not null.
+     *
+     * @throws IAE if the dimension name is null.
+     */
     public Builder setDimensionIfNotNull(String dim, Object value)
     {
-      if (value != null) {
-        userDims.put(dim, value);
-      }
-      return this;
+      return value == null ? this : setDimension(dim, value);
     }
 
+    /**
+     * Adds a dimension to be emitted with this metric event.
+     *
+     * @throws IAE if the dimension name or the given value is null.
+     */
     public Builder setDimension(String dim, Object value)
     {
+      if (dim == null) {
+        throw new IAE("Dimension name cannot be null");
+      } else if (value == null) {
+        throw new IAE("Value of dimension[%s] cannot be null", dim);
+      }
+
       userDims.put(dim, value);
       return this;
     }
@@ -205,7 +213,7 @@ public ServiceMetricEvent build(ImmutableMap<String, String> serviceDimensions)
       return new ServiceMetricEvent(
           createdTime,
           serviceDimensions,
-          userDims,
+          ImmutableMap.copyOf(userDims),
           feed,
           metric,
           value

processing/src/test/java/org/apache/druid/java/util/emitter/service/ServiceMetricEventTest.java

@@ -22,6 +22,7 @@
 
 import com.google.common.collect.ImmutableMap;
 import org.apache.druid.java.util.common.DateTimes;
+import org.apache.druid.java.util.common.IAE;
 import org.junit.Assert;
 import org.junit.Test;
 
@@ -319,18 +320,16 @@ public void testSetDimensionIfNotNullShouldNotSetNullDimension()
   }
 
   @Test
-  public void test_copy_returnsAnImmutableInstance()
+  public void test_builder_createsImmutableEvents()
   {
     final ServiceMetricEvent.Builder eventBuilder = ServiceMetricEvent
         .builder()
         .setDimension("dim1", "v1")
         .setMetric("m1", 100);
 
     final ServiceMetricEvent event1 = eventBuilder.build("coordinator", "localhost");
-    final ServiceMetricEvent event1Copy = event1.copy();
 
     Assert.assertEquals(Map.of("dim1", "v1"), event1.getUserDims());
-    Assert.assertEquals(Map.of("dim1", "v1"), event1Copy.getUserDims());
 
     final ServiceMetricEvent event2 = eventBuilder
         .setDimension("dim2", "v2")
@@ -339,9 +338,25 @@ public void test_copy_returnsAnImmutableInstance()
 
     // Verify that the original event gets changed dimensions
     Assert.assertEquals(Map.of("dim1", "v1", "dim2", "v2"), event2.getUserDims());
-    Assert.assertEquals(Map.of("dim1", "v1", "dim2", "v2"), event1.getUserDims());
+    Assert.assertEquals(Map.of("dim1", "v1"), event1.getUserDims());
+  }
+
+  @Test
+  public void test_builder_throwsException_ifDimNameOrValueIsNull()
+  {
+    final ServiceMetricEvent.Builder eventBuilder = ServiceMetricEvent.builder();
 
-    // But the event copy still has the original dimensions
-    Assert.assertEquals(Map.of("dim1", "v1"), event1Copy.getUserDims());
+    Assert.assertThrows(
+        IAE.class,
+        () -> eventBuilder.setDimension("dim1", (Object) null)
+    );
+    Assert.assertThrows(
+        IAE.class,
+        () -> eventBuilder.setDimension(null, null)
+    );
+    Assert.assertThrows(
+        IAE.class,
+        () -> eventBuilder.setDimension(null, new String[]{"a"})
+    );
   }
 }

processing/src/test/java/org/apache/druid/java/util/metrics/StubServiceEmitter.java

@@ -59,7 +59,7 @@ public void emit(Event event)
     if (event instanceof ServiceMetricEvent) {
       ServiceMetricEvent metricEvent = (ServiceMetricEvent) event;
       metricEvents.computeIfAbsent(metricEvent.getMetric(), name -> new ConcurrentLinkedDeque<>())
-                  .add(metricEvent.copy());
+                  .add(metricEvent);
     } else if (event instanceof AlertEvent) {
       alertEvents.add((AlertEvent) event);
     }

processing/src/test/java/org/apache/druid/query/DefaultQueryMetricsTest.java

@@ -66,11 +66,11 @@ public void testDefaultQueryMetricsQuery()
         .context(ImmutableMap.of("testKey", "testValue"))
         .build();
     queryMetrics.query(query);
-    queryMetrics.reportQueryTime(0).emit(serviceEmitter);
     // No way to verify this right now since DefaultQueryMetrics implements a no-op for sqlQueryId(String) and queryId(String)
     // This change is done to keep the code coverage tool happy by exercising the implementation
     queryMetrics.sqlQueryId("dummy");
     queryMetrics.queryId("dummy");
+    queryMetrics.reportQueryTime(0).emit(serviceEmitter);
     Map<String, Object> actualEvent = serviceEmitter.getEvents().get(0).toMap();
     Assert.assertEquals(13, actualEvent.size());
     Assert.assertTrue(actualEvent.containsKey("feed"));