FrankChen021 commented on code in PR #19674:
URL: https://github.com/apache/druid/pull/19674#discussion_r3558724492
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/actions/SegmentAllocateAction.java:
##########
@@ -280,14 +285,28 @@ public SegmentIdWithShardSpec perform(
}
}
+ private void emitSuccessMetric(SegmentIdWithShardSpec allocatedId, Task
task, TaskActionToolbox toolbox)
+ {
+ final ServiceMetricEvent.Builder metricBuilder = new
ServiceMetricEvent.Builder();
+ IndexTaskUtils.setTaskDimensions(metricBuilder, task);
+ if (task instanceof SeekableStreamIndexTask<?,?,?>) {
Review Comment:
[P1] Fix the new checkstyle violations
`SeekableStreamIndexTask<?,?,?>` violates the configured `WhitespaceAfter`
check, while changing line 309 to `.toList()` leaves
`java.util.stream.Collectors` unused and violates `UnusedImports`; validation
will fail until both are corrected.
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/actions/SegmentAllocateAction.java:
##########
@@ -241,6 +245,7 @@ public SegmentIdWithShardSpec perform(
identifier = tryAllocateSubsequentSegment(toolbox, task, rowInterval,
usedSegmentsForRow.iterator().next());
}
if (identifier != null) {
+ emitSuccessMetric(identifier, task, toolbox);
Review Comment:
[P1] Emit the metric for batched allocations
`LocalTaskActionClient` selects `performAsync` whenever batching is enabled,
and `TaskLockConfig` enables batching by default. That path completes directly
through `SegmentAllocationQueue`, so this new call in `perform` is bypassed and
the metric is absent under the default configuration.
##########
docs/operations/metrics.md:
##########
@@ -345,6 +345,7 @@ If the JVM does not support CPU time measurement for the
current thread, `ingest
|`segment/added/bytes`|Size in bytes of new segments created.| `dataSource`,
`taskId`, `taskType`, `groupId`, `interval`, `tags`|Varies|
|`segment/moved/bytes`|Size in bytes of segments moved/archived via the Move
Task.| `dataSource`, `taskId`, `taskType`, `groupId`, `interval`, `tags`|Varies|
|`segment/nuked/bytes`|Size in bytes of segments deleted via the Kill Task.|
`dataSource`, `taskId`, `taskType`, `groupId`, `interval`, `tags`|Varies|
+|`segment/allocated/count`|Number of segments successfully allocated by the
Overlord to an append (realtime or batch) task. May be emitted multiple times
for a single allocated segment ID if multiple tasks request an allocation with
the same parameters.|`id`, `dataSource`, `taskId`, `taskType`, `groupId`,
tags`|Always 1|
Review Comment:
[P3] Document the emitted supervisor dimension
The implementation adds `supervisorId` for seekable-stream tasks, but this
dimension is omitted from the metric table. The same row also has an unmatched
backtick around `tags`.
--
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]
