kfaraz commented on code in PR #18876:
URL: https://github.com/apache/druid/pull/18876#discussion_r2652290966
##########
processing/src/main/java/org/apache/druid/java/util/metrics/TaskHolder.java:
##########
@@ -17,13 +17,14 @@
* under the License.
*/
-package org.apache.druid.server.metrics;
+package org.apache.druid.java.util.metrics;
import javax.annotation.Nullable;
+import java.util.Map;
/**
* Provides identifying information for a task. Implementations return {@code
null}
- * when used in server processes that are not {@code CliPeon}.
+ * when used in server processes that are not {@code CliPeon}. Note that t
Review Comment:
Incomplete javadoc?
##########
services/src/main/java/org/apache/druid/cli/PeonTaskHolder.java:
##########
@@ -59,4 +62,34 @@ public String getTaskId()
{
return taskProvider.get().getId();
}
+
+ @Override
+ public String getTaskType()
+ {
+ return taskProvider.get().getType();
+ }
+
+ @Override
+ public String getGroupId()
+ {
+ return taskProvider.get().getGroupId();
+ }
+
+ /**
+ * @return a map of all task-specific dimensions applicable to this peon.
+ * The task ID ({@link TaskHolder#getTaskId()}) is added to both {@link
DruidMetrics#TASK_ID}
+ * {@link DruidMetrics#ID} dimensions to the map for backward compatibility.
{@link DruidMetrics#ID} is
+ * deprecated because it's ambiguous and can be removed in a future
release.</p>
+ */
+ @Override
+ public Map<String, String> getMetricDimensions()
+ {
+ return Map.of(
+ DruidMetrics.DATASOURCE, getDataSource(),
Review Comment:
Nit: Each getter invokes the `taskProvider` separately. Maybe just invoke it
once and assign the result to a `Task task` variable.
##########
processing/src/main/java/org/apache/druid/java/util/emitter/service/ServiceEmitter.java:
##########
@@ -25,39 +25,56 @@
import org.apache.druid.java.util.common.lifecycle.LifecycleStop;
import org.apache.druid.java.util.emitter.core.Emitter;
import org.apache.druid.java.util.emitter.core.Event;
+import org.apache.druid.java.util.metrics.NoopTaskHolder;
+import org.apache.druid.java.util.metrics.TaskHolder;
import java.io.IOException;
public class ServiceEmitter implements Emitter
{
- private final ImmutableMap<String, String> serviceDimensions;
- private final Emitter emitter;
+ protected final Emitter emitter;
Review Comment:
Why not private?
##########
server/src/test/java/org/apache/druid/server/metrics/GroupByStatsMonitorTest.java:
##########
@@ -98,30 +103,31 @@ public void testMonitorWithServiceDimensions()
{
final String dataSource = "fooDs";
final String taskId = "taskId1";
+ final String groupId = "test_groupid";
+ final String taskType = "test_tasktype";
final GroupByStatsMonitor monitor = new GroupByStatsMonitor(
groupByStatsProvider,
- mergeBufferPool,
- new TestTaskHolder(dataSource, taskId)
+ mergeBufferPool
);
- final StubServiceEmitter emitter = new StubServiceEmitter("service",
"host");
+ final StubServiceEmitter emitter = new StubServiceEmitter("service",
"host", new TestTaskHolder(dataSource, taskId, taskType, groupId));
+ emitter.start();
monitor.doMonitor(emitter);
emitter.flush();
// Trigger metric emission
monitor.doMonitor(emitter);
- final Map<String, Object> dimFilters = Map.of(
- "taskId", List.of(taskId), "dataSource", List.of(dataSource), "id",
List.of(taskId)
+ final Map<String, Object> dimFilters = Map.of(DruidMetrics.DATASOURCE,
dataSource, DruidMetrics.TASK_ID, taskId,
Review Comment:
Please put each key-value pair in a separate line.
##########
processing/src/main/java/org/apache/druid/java/util/metrics/Monitors.java:
##########
@@ -20,43 +20,39 @@
package org.apache.druid.java.util.metrics;
import java.util.List;
-import java.util.Map;
public class Monitors
{
/**
* Creates a JVM monitor, configured with the given dimensions, that gathers
all currently available JVM-wide
* monitors. Emitted events have default feed {@link
FeedDefiningMonitor#DEFAULT_METRICS_FEED}
- * See: {@link Monitors#createCompoundJvmMonitor(Map, String)}
- *
- * @param dimensions common dimensions to configure the JVM monitor with
+ * See: {@link Monitors#createCompoundJvmMonitor(String)}
*
* @return a universally useful JVM-wide monitor
*/
- public static Monitor createCompoundJvmMonitor(Map<String, String[]>
dimensions)
+ public static Monitor createCompoundJvmMonitor()
Review Comment:
Is this class/method used anywhere? I couldn't find any usage.
##########
processing/src/test/java/org/apache/druid/java/util/metrics/StubServiceEmitter.java:
##########
@@ -41,18 +44,32 @@
*/
public class StubServiceEmitter extends ServiceEmitter implements
MetricsVerifier
{
+ public static final String TYPE = "stub";
+
private final Deque<Event> events = new ConcurrentLinkedDeque<>();
private final Deque<AlertEvent> alertEvents = new ConcurrentLinkedDeque<>();
private final ConcurrentHashMap<String, Deque<ServiceMetricEvent>>
metricEvents = new ConcurrentHashMap<>();
public StubServiceEmitter()
{
- super("testing", "localhost", null);
+ this("testing", "localhost");
}
+ /**
+ * Initialize a stub service emitter and auto-{@link #start()} it for test
convenience.
+ */
public StubServiceEmitter(String service, String host)
{
- super(service, host, null);
+ this(service, host, new NoopTaskHolder());
+ super.start();
+ }
+
+ /**
+ * Initialize a stub service emitter. Tests must explicitly call {@link
#start()}.
+ */
+ public StubServiceEmitter(String service, String host, TaskHolder taskHolder)
Review Comment:
We should update the `LatchableEmitter` constructor to accept a `TaskHolder`
too.
Otherwise, the task dimensions will not show up in embedded tests.
You could also add a short test method in any of the existing tests to
verify the new dimensions.
##########
processing/src/main/java/org/apache/druid/java/util/metrics/TaskHolder.java:
##########
@@ -38,4 +39,21 @@ public interface TaskHolder
*/
@Nullable
String getTaskId();
+
+ /**
+ * @return the taskId, or {@code null} if called from a server that is not
{@code CliPeon}.
+ */
+ @Nullable
+ String getTaskType();
+
+ /**
+ * @return the taskId, or {@code null} if called from a server that is not
{@code CliPeon}.
Review Comment:
```suggestion
* @return the group ID of this task, or {@code null} if called from a
server that is not {@code CliPeon}.
```
##########
server/src/test/java/org/apache/druid/server/metrics/GroupByStatsMonitorTest.java:
##########
@@ -174,4 +180,17 @@ public void testMonitoringMergeBuffer_pendingRequests()
// do nothing
}
}
+
+
+ private void verifyTaskServiceDimensions(StubServiceEmitter emitter, String
metricName, Map<String, Object> dimFilters, Number expectedValue)
Review Comment:
```suggestion
private void verifyMetricValue(StubServiceEmitter emitter, String
metricName, Map<String, Object> dimFilters, Number expectedValue)
```
##########
processing/src/test/java/org/apache/druid/java/util/metrics/StubServiceEmitter.java:
##########
@@ -173,5 +191,11 @@ public void flush()
@Override
public void close()
{
+ try {
+ emitter.close();
Review Comment:
Isn't the `emitter` always a `NoopEmitter`, why do we need to invoke `start`
and `close` on it?
##########
processing/src/test/java/org/apache/druid/java/util/metrics/StubServiceEmitterModule.java:
##########
@@ -0,0 +1,43 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.java.util.metrics;
+
+import com.google.inject.Binder;
+import com.google.inject.Module;
+import com.google.inject.Provides;
+import com.google.inject.name.Named;
+import org.apache.druid.guice.ManageLifecycle;
+import org.apache.druid.java.util.emitter.core.Emitter;
+
+public class StubServiceEmitterModule implements Module
Review Comment:
Instead of adding this, I wonder if we shouldn't just move
`LatchableEmitterModule` to `processing/src/test/`.
##########
server/src/test/java/org/apache/druid/server/emitter/EmitterModuleTest.java:
##########
@@ -87,6 +108,78 @@ public void testInvalidEmitterType()
makeInjectorWithProperties(props).getInstance(Emitter.class);
}
+ @Test
+ public void testEmitterForTaskContainsAllTaskDimensions()
+ {
+ Properties props = new Properties();
+ props.setProperty("druid.emitter", "stub");
+ EmitterModule emitterModule = new EmitterModule();
+ emitterModule.setProps(props);
+
+ ImmutableSet<NodeRole> nodeRoles = ImmutableSet.of();
+
+ TestTaskHolder testTaskHolder = new TestTaskHolder("d", "e", "a", "w");
Review Comment:
Maybe use strings that are easier to follow, e.g.
`datasource = wiki`, `taskID = id1`, `taskType = type1`, `groupId = group1`,
etc.
##########
processing/src/main/java/org/apache/druid/java/util/metrics/TaskHolder.java:
##########
@@ -38,4 +39,21 @@ public interface TaskHolder
*/
@Nullable
String getTaskId();
+
+ /**
+ * @return the taskId, or {@code null} if called from a server that is not
{@code CliPeon}.
Review Comment:
```suggestion
* @return the type name of this task, or {@code null} if called from a
server that is not {@code CliPeon}.
```
##########
processing/src/test/java/org/apache/druid/java/util/metrics/StubServiceEmitter.java:
##########
@@ -41,18 +44,32 @@
*/
public class StubServiceEmitter extends ServiceEmitter implements
MetricsVerifier
{
+ public static final String TYPE = "stub";
+
private final Deque<Event> events = new ConcurrentLinkedDeque<>();
private final Deque<AlertEvent> alertEvents = new ConcurrentLinkedDeque<>();
private final ConcurrentHashMap<String, Deque<ServiceMetricEvent>>
metricEvents = new ConcurrentHashMap<>();
public StubServiceEmitter()
{
- super("testing", "localhost", null);
+ this("testing", "localhost");
}
+ /**
+ * Initialize a stub service emitter and auto-{@link #start()} it for test
convenience.
Review Comment:
Instead of doing the auto-start in the constructor, add a static utility
method that may be used something like
```
ServiceEmitter emitter = StubServiceEmitter.createStarted();
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
