kfaraz commented on code in PR #17488:
URL: https://github.com/apache/druid/pull/17488#discussion_r1849636373
##########
docs/operations/metrics.md:
##########
@@ -391,9 +391,9 @@ These metrics are emitted by the Druid Coordinator in every
run of the correspon
### Service Health
-|Metric|Description|Dimensions|Normal value|
-|------|-----------|----------|------------|
-| `service/heartbeat` | Metric indicating the service is up. This metric is
emitted only when `ServiceStatusMonitor` is enabled. | `leader` on the Overlord
and Coordinator.<br />`workerVersion`, `category`, `status` on the Middle
Manager.<br />`taskId`, `groupId`, `taskType`, `dataSource`, `tags` on the Peon
|1|
+|Metric|Description| Dimensions
|Normal value|
+|------|-----------|-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|------------|
Review Comment:
Please revert these formatting changes.
##########
services/src/main/java/org/apache/druid/cli/CliPeon.java:
##########
@@ -308,6 +308,9 @@ public Supplier<Map<String, Object>>
heartbeatDimensions(Task task)
builder.put(DruidMetrics.DATASOURCE, task.getDataSource());
builder.put(DruidMetrics.TASK_TYPE, task.getType());
builder.put(DruidMetrics.GROUP_ID, task.getGroupId());
+ if (task.getStatus() != null) {
+ builder.put(DruidMetrics.TASK_STATUS, task.getStatus());
Review Comment:
I don't think we should use this dimension as it corresponds to the final
status of a task which only takes values `SUCCESS` or `FAILURE`. We could use
`DruidMetrics.STATUS` or some other dimension instead.
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/task/Task.java:
##########
@@ -313,6 +315,15 @@ default TaskIdentifier getMetadata()
return new TaskIdentifier(this.getId(), this.getGroupId(), this.getType());
}
+ /**
+ * @return The status of the task. Note: this interface method is unstable
at this time.
+ */
+ @Nullable
+ default String getStatus()
Review Comment:
Rather than adding a `getStatus()` method here, I would prefer it if we just
cast to `SeekableStreamTask` in `CliPeon`.
The `Task.run()` method runs the task and returns the final status.
We shouldn't have or need access to the status of the task while the task is
running.
Moreover, the status being returned here is not the same as the task status.
--
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]
