diff --git a/indexing-hadoop/src/test/java/org/apache/druid/indexer/HadoopTaskTest.java b/indexing-hadoop/src/test/java/org/apache/druid/indexer/HadoopTaskTest.java index 02679387e88d..c51c8fd7e4f8 100644 --- a/indexing-hadoop/src/test/java/org/apache/druid/indexer/HadoopTaskTest.java +++ b/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 diff --git a/indexing-service/src/test/java/org/apache/druid/indexing/common/task/IndexTaskUtilsTest.java b/indexing-service/src/test/java/org/apache/druid/indexing/common/task/IndexTaskUtilsTest.java index 025b3ee09ea9..0e5839c87d80 100644 --- a/indexing-service/src/test/java/org/apache/druid/indexing/common/task/IndexTaskUtilsTest.java +++ b/indexing-service/src/test/java/org/apache/druid/indexing/common/task/IndexTaskUtilsTest.java @@ -38,7 +38,6 @@ public class IndexTaskUtilsTest private static final Map 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() { diff --git a/indexing-service/src/test/java/org/apache/druid/indexing/seekablestream/SeekableStreamIndexTaskRunnerAuthTest.java b/indexing-service/src/test/java/org/apache/druid/indexing/seekablestream/SeekableStreamIndexTaskRunnerAuthTest.java index 92c828c69850..ec71cec89c18 100644 --- a/indexing-service/src/test/java/org/apache/druid/indexing/seekablestream/SeekableStreamIndexTaskRunnerAuthTest.java +++ b/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 diff --git a/processing/src/main/java/org/apache/druid/java/util/emitter/service/ServiceMetricEvent.java b/processing/src/main/java/org/apache/druid/java/util/emitter/service/ServiceMetricEvent.java index f7f3549ecf7d..6644886497df 100644 --- a/processing/src/main/java/org/apache/druid/java/util/emitter/service/ServiceMetricEvent.java +++ b/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 serviceDims; - private final Map userDims; + private final ImmutableMap userDims; private final String feed; private final String metric; private final Number value; + /** + * Creates an immutable metric event. + */ private ServiceMetricEvent( DateTime createdTime, ImmutableMap serviceDims, - Map userDims, + ImmutableMap userDims, String feed, String metric, Number value @@ -92,7 +95,7 @@ public String getHost() public Map 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 serviceDimensions) return new ServiceMetricEvent( createdTime, serviceDimensions, - userDims, + ImmutableMap.copyOf(userDims), feed, metric, value diff --git a/processing/src/test/java/org/apache/druid/java/util/emitter/service/ServiceMetricEventTest.java b/processing/src/test/java/org/apache/druid/java/util/emitter/service/ServiceMetricEventTest.java index cf59f0f3c40e..2b9428708020 100644 --- a/processing/src/test/java/org/apache/druid/java/util/emitter/service/ServiceMetricEventTest.java +++ b/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,7 +320,7 @@ public void testSetDimensionIfNotNullShouldNotSetNullDimension() } @Test - public void test_copy_returnsAnImmutableInstance() + public void test_builder_createsImmutableEvents() { final ServiceMetricEvent.Builder eventBuilder = ServiceMetricEvent .builder() @@ -327,10 +328,8 @@ public void test_copy_returnsAnImmutableInstance() .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"}) + ); } } diff --git a/processing/src/test/java/org/apache/druid/java/util/metrics/StubServiceEmitter.java b/processing/src/test/java/org/apache/druid/java/util/metrics/StubServiceEmitter.java index b330573c3621..fac548afb468 100644 --- a/processing/src/test/java/org/apache/druid/java/util/metrics/StubServiceEmitter.java +++ b/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); } diff --git a/processing/src/test/java/org/apache/druid/query/DefaultQueryMetricsTest.java b/processing/src/test/java/org/apache/druid/query/DefaultQueryMetricsTest.java index f7cd6887e7e0..ee82f206f79c 100644 --- a/processing/src/test/java/org/apache/druid/query/DefaultQueryMetricsTest.java +++ b/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 actualEvent = serviceEmitter.getEvents().get(0).toMap(); Assert.assertEquals(13, actualEvent.size()); Assert.assertTrue(actualEvent.containsKey("feed"));