kfaraz commented on code in PR #19624:
URL: https://github.com/apache/druid/pull/19624#discussion_r3489950173
##########
processing/src/main/java/org/apache/druid/query/QueryMetrics.java:
##########
@@ -329,7 +329,16 @@ default void filterBundle(FilterBundle.BundleInfo
bundleInfo)
QueryMetrics<QueryType> reportQueryBytes(long byteCount);
/**
- * Registers "segments queried count" metric.
+ * Registers the {@code query/segments/count} metric, the number of segments
touched by the query.
+ *
+ * Emitted once per query. The value's meaning depends on the emitting
process:
+ * <ul>
+ * <li>On the Broker, it is the number of segments in the planned query
distribution (a snapshot-based
+ * count that may double-count segments that moved and were
re-fetched).</li>
+ * <li>On a data node (Historical, Peon/realtime), it is the number of
segments that node actually scanned
+ * for the query.</li>
+ * </ul>
+ * The two are disambiguated by the emitting service/host, not by the metric
name.
Review Comment:
Nit: Do we really want to overload the definition of this metric?
We may even take this opportunity to use a different metric name for the
number of segments targeted by the query plan.
##########
processing/src/main/java/org/apache/druid/query/QueryMetrics.java:
##########
@@ -329,7 +329,16 @@ default void filterBundle(FilterBundle.BundleInfo
bundleInfo)
QueryMetrics<QueryType> reportQueryBytes(long byteCount);
/**
- * Registers "segments queried count" metric.
+ * Registers the {@code query/segments/count} metric, the number of segments
touched by the query.
+ *
+ * Emitted once per query. The value's meaning depends on the emitting
process:
Review Comment:
```suggestion
* Emitted once per query. The meaning of the metric depends on the
emitting process:
```
##########
server/src/main/java/org/apache/druid/segment/realtime/appenderator/SinkQuerySegmentWalker.java:
##########
@@ -544,6 +579,20 @@ public void after(boolean isDone, Throwable thrown)
log.error(e, "Failed to emit metrics for segment[%s]",
segmentAndMetrics.getKey());
}
}
+
+ if (segmentCount != null) {
Review Comment:
```suggestion
// Emit total segmentCount once per query
if (segmentCount != null) {
```
Do we also want to add a validation on `segmentId == null` here to ensure
that we don't accidentally call this more than once?
--
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]
