You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "jtuglu1 (via GitHub)" <gi...@apache.org> on 2026/06/24 01:48:14 UTC
[PR] feat: add query/segment/count metric to data nodes (druid)
jtuglu1 opened a new pull request, #19624:
URL: https://github.com/apache/druid/pull/19624
<!-- Replace XXXX with the id of the issue fixed in this PR. Remove this section if there is no corresponding issue. Don't reference the issue in the title of this pull-request. -->
<!-- If you are a committer, follow the PR action item checklist for committers:
https://github.com/apache/druid/blob/master/dev/committer-instructions.md#pr-and-issue-action-item-checklist-for-committers. -->
### Description
Allows for quick queries to understand how many segments a given query issued scans for against a given data node. Better than counting the unique query/segment/time or similar, especially if you are sampling.
<!--
In each section, please describe design decisions made, including:
- Choice of algorithms
- Behavioral aspects. What configuration values are acceptable? How are corner cases and error conditions handled, such as when there are insufficient resources?
- Class organization and design (how the logic is split between classes, inheritance, composition, design patterns)
- Method organization and design (how the logic is split between methods, parameters and return types)
- Naming (class, method, API, configuration, HTTP endpoint, names of emitted metrics)
-->
<!-- It's good to describe an alternative design (or mention an alternative name) for every design (or naming) decision point and compare the alternatives with the designs that you've implemented (or the names you've chosen) to highlight the advantages of the chosen designs and names. -->
<!-- If there was a discussion of the design of the feature implemented in this PR elsewhere (e. g. a "Proposal" issue, any other issue, or a thread in the development mailing list), link to that discussion from this PR description and explain what have changed in your final design compared to your original proposal or the consensus version in the end of the discussion. If something hasn't changed since the original discussion, you can omit a detailed discussion of those aspects of the design here, perhaps apart from brief mentioning for the sake of readability of this PR description. -->
<!-- Some of the aspects mentioned above may be omitted for simple and small changes. -->
#### Release note
<!-- Give your best effort to summarize your changes in a couple of sentences aimed toward Druid users.
If your change doesn't have end user impact, you can skip this section.
For tips about how to write a good release note, see [Release notes](https://github.com/apache/druid/blob/master/CONTRIBUTING.md#release-notes).
-->
<hr>
##### Key changed/added classes in this PR
* `MyFoo`
* `OurBar`
* `TheirBaz`
<hr>
<!-- Check the items by putting "x" in the brackets for the done things. Not all of these items apply to every PR. Remove the items which are not done or not relevant to the PR. None of the items from the checklist below are strictly necessary, but it would be very helpful if you at least self-review the PR. -->
This PR has:
- [ ] been self-reviewed.
- [ ] using the [concurrency checklist](https://github.com/apache/druid/blob/master/dev/code-review/concurrency.md) (Remove this item if the PR doesn't have any relation to concurrency.)
- [ ] added documentation for new or modified features or behaviors.
- [ ] a release note entry in the PR description.
- [ ] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
- [ ] added or updated version, license, or notice information in [licenses.yaml](https://github.com/apache/druid/blob/master/dev/license.md)
- [ ] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [ ] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
- [ ] added integration tests.
- [ ] been tested in a test Druid cluster.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] feat: add query/segments/count metric to data nodes (druid)
Posted by "jtuglu1 (via GitHub)" <gi...@apache.org>.
jtuglu1 commented on code in PR #19624:
URL: https://github.com/apache/druid/pull/19624#discussion_r3494207211
##########
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:
The if (segmentCount != null) block is already in the else branch of the (segmentId != null guard) above
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] feat: add query/segment/count metric to data nodes (druid)
Posted by "FrankChen021 (via GitHub)" <gi...@apache.org>.
FrankChen021 commented on code in PR #19624:
URL: https://github.com/apache/druid/pull/19624#discussion_r3474273574
##########
server/src/main/java/org/apache/druid/segment/realtime/appenderator/SinkQuerySegmentWalker.java:
##########
@@ -354,6 +354,8 @@ public <T> QueryRunner<T> getQueryRunnerForSegments(final Query<T> query, final
);
}
+ final int localSegmentCount = allSegmentReferences.size();
Review Comment:
[P2] Count realtime sinks instead of hydrants
This counts `allSegmentReferences`, but those references are hydrant-level: `Sink.acquireSegmentReferences` adds one `SinkSegmentReference` per `FireHydrant`. After a realtime segment has multiple hydrants, for example after persist/swap or with both persisted and current hydrants, a query over one Druid segment emits `query/segment/count` as the hydrant count instead of the segment/Sink count. That disagrees with the metric's documented segment-level meaning and with the existing per-segment metrics that aggregate by `sinkSegmentId`; please count distinct queried sinks/descriptors and add a multi-hydrant test.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] feat: add query/segment/count metric to data nodes (druid)
Posted by "FrankChen021 (via GitHub)" <gi...@apache.org>.
FrankChen021 commented on code in PR #19624:
URL: https://github.com/apache/druid/pull/19624#discussion_r3466974530
##########
server/src/main/java/org/apache/druid/server/ServerManager.java:
##########
@@ -639,6 +639,7 @@ public Sequence<T> run(QueryPlus<T> queryPlus, ResponseContext responseContext)
segmentsBundle,
segmentMapFn
);
+ queryPlus.getQueryMetrics().reportLocalSegmentCount(segmentReferences.size()).emit(emitter);
Review Comment:
[P2] Register segment references before emitting the new metric
This emits after getOrLoadBundleSegments returns but before closer.registerAll(segmentReferences). If reportLocalSegmentCount or the emitter throws, the catch block closes only the still-empty closer, so the acquired SegmentReference objects are leaked. Register the references with the closer before emitting this metric, or otherwise guarantee the references are closed on emission failure.
##########
docs/operations/metrics.md:
##########
@@ -140,6 +141,7 @@ to represent the task ID are deprecated and will be removed in a future release.
|Metric|Description|Dimensions|Normal value|
|------|-----------|----------|------------|
|`query/time`|Milliseconds taken to complete a query.|<p>Common: `dataSource`, `type`, `interval`, `hasFilters`, `duration`, `context`, `remoteAddress`, `id`, `statusCode`.</p><p> Aggregation Queries: `numMetrics`, `numComplexMetrics`.</p><p> GroupBy: `numDimensions`.</p><p> TopN: `threshold`, `dimension`.</p>|< 1s|
+|`query/segment/count`|Number of segments this Peon scans for a query. This is local to this data node; the Broker metric `query/segments/count` reports the distributed query plan.|<p>Common: `dataSource`, `type`, `interval`, `hasFilters`, `duration`, `context`, `remoteAddress`, `id`.</p><p> Aggregation Queries: `numMetrics`, `numComplexMetrics`.</p><p> GroupBy: `numDimensions`. </p><p>TopN: `threshold`, `dimension`.</p>|Varies|
Review Comment:
[P2] Peon metric is documented but not emitted
The PR documents query/segment/count for Peons, but the only new emission is in ServerManager. Peon/realtime queries route through PeonAppenderatorsManager or UnifiedIndexerAppenderatorsManager into SinkQuerySegmentWalker, which still only emits segment time, wait time, and segmentAndCache time. As a result the new metric is absent for Real-time/Peon data nodes despite the docs and emitter allowlists; add equivalent emission in the sink walker or remove the Peon entry.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] feat: add query/segments/count metric to data nodes (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] feat: add query/segment/count metric to data nodes (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #19624:
URL: https://github.com/apache/druid/pull/19624#discussion_r3472557274
##########
docs/operations/metrics.md:
##########
@@ -106,6 +106,7 @@ Most metric values reset each emission period, as specified in `druid.monitoring
|Metric|Description|Dimensions|Normal value|
|------|-----------|----------|------------|
|`query/time`|Milliseconds taken to complete a query.|<p>Common: `dataSource`, `type`, `interval`, `hasFilters`, `duration`, `context`, `remoteAddress`, `id`, `statusCode`.</p><p> Aggregation Queries: `numMetrics`, `numComplexMetrics`.</p><p> GroupBy: `numDimensions`.</p><p> TopN: `threshold`, `dimension`.</p>|< 1s|
+|`query/segment/count`|Number of segments this Historical scans for a query. This is local to this data node; the Broker metric `query/segments/count` reports the distributed query plan.|<p>Common: `dataSource`, `type`, `interval`, `hasFilters`, `duration`, `context`, `remoteAddress`, `id`.</p><p> Aggregation Queries: `numMetrics`, `numComplexMetrics`.</p><p> GroupBy: `numDimensions`.</p><p> TopN: `threshold`, `dimension`.</p>|Varies|
Review Comment:
Ah, the similar metric names `query/segment/count` and `query/segments/count` (with an 'S') can be confusing. I wish the Broker metric name reflected that it was a plan-only metric.
Nit: For the use case in this PR though, I feel that `query/segments/count` probably makes more sense since `query/segment/time` reflects time taken to scan a single segment and not the total time.
##########
server/src/main/java/org/apache/druid/segment/realtime/appenderator/SinkQuerySegmentWalker.java:
##########
@@ -470,6 +474,8 @@ private static class SinkMetricsEmittingQueryRunner<T> implements QueryRunner<T>
private final Set<String> metricsToCompute;
@Nullable
private final String segmentId;
+ @Nullable
+ private final Integer localSegmentCount;
Review Comment:
What does the prefix `local` signify?
##########
server/src/main/java/org/apache/druid/segment/realtime/appenderator/SinkQuerySegmentWalker.java:
##########
@@ -452,7 +455,8 @@ public static String makeHydrantCacheIdentifier(final SegmentId segmentId, final
}
/**
- * This class is responsible for emitting query/segment/time, query/wait/time and query/segmentAndCache/Time metrics for a Sink.
+ * This class is responsible for emitting query/segment/time, query/wait/time, query/segmentAndCache/Time and
+ * query/segment/count metrics for a Sink.
Review Comment:
We should also call out that unlike other metrics emitted here, `query/segment/count` is emitted only when `segmentId` is non-null, since it is a total count.
##########
docs/operations/metrics.md:
##########
@@ -106,6 +106,7 @@ Most metric values reset each emission period, as specified in `druid.monitoring
|Metric|Description|Dimensions|Normal value|
|------|-----------|----------|------------|
|`query/time`|Milliseconds taken to complete a query.|<p>Common: `dataSource`, `type`, `interval`, `hasFilters`, `duration`, `context`, `remoteAddress`, `id`, `statusCode`.</p><p> Aggregation Queries: `numMetrics`, `numComplexMetrics`.</p><p> GroupBy: `numDimensions`.</p><p> TopN: `threshold`, `dimension`.</p>|< 1s|
+|`query/segment/count`|Number of segments this Historical scans for a query. This is local to this data node; the Broker metric `query/segments/count` reports the distributed query plan.|<p>Common: `dataSource`, `type`, `interval`, `hasFilters`, `duration`, `context`, `remoteAddress`, `id`.</p><p> Aggregation Queries: `numMetrics`, `numComplexMetrics`.</p><p> GroupBy: `numDimensions`.</p><p> TopN: `threshold`, `dimension`.</p>|Varies|
Review Comment:
Btw, do we even need to mention the Broker metric here? It seems unrelated despite the similar names. The disambiguation, if needed, may be done in Broker metrics section itself.
##########
server/src/test/java/org/apache/druid/server/coordination/ServerManagerTest.java:
##########
@@ -482,6 +483,29 @@ public void testGetQueryRunnerForIntervals_whenTimelineIsMissingReturningNoopQue
Assert.assertSame(NoopQueryRunner.class, queryRunner.getClass());
}
+ @Test
+ public void testGetQueryRunnerForIntervalsEmitsLocalSegmentCountMetric()
+ {
+ final StubServiceEmitter stubServiceEmitter = StubServiceEmitter.createStarted();
+ serviceEmitter = stubServiceEmitter;
+ factory = new MyQueryRunnerFactory(new CountDownLatch(0), new CountDownLatch(0), new CountDownLatch(0));
+ conglomerate = DefaultQueryRunnerFactoryConglomerate.buildFromQueryRunnerFactories(ImmutableMap.of(
+ SearchQuery.class,
+ factory
+ ));
+ serverManager = Guice.createInjector(BoundFieldModule.of(this)).getInstance(ServerManager.class);
+
+ final Interval interval = Intervals.of("P2d/2011-04-02");
+ final SearchQuery query = searchQuery("test", interval, Granularities.DAY);
+
+ serverManager.getQueryRunnerForIntervals(query, ImmutableList.of(interval))
+ .run(QueryPlus.wrap(query))
+ .toList();
+
+ Assert.assertEquals(1, stubServiceEmitter.getMetricEventCount(DefaultQueryMetrics.QUERY_SEGMENT_COUNT));
+ Assert.assertEquals(2L, stubServiceEmitter.getLatestMetricEventValue(DefaultQueryMetrics.QUERY_SEGMENT_COUNT));
Review Comment:
Nit: You may also use `stubServiceEmitter.verifyEmitted` and `verifyValue`.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] feat: add query/segment/count metric to data nodes (druid)
Posted by "jtuglu1 (via GitHub)" <gi...@apache.org>.
jtuglu1 commented on code in PR #19624:
URL: https://github.com/apache/druid/pull/19624#discussion_r3472734334
##########
server/src/main/java/org/apache/druid/segment/realtime/appenderator/SinkQuerySegmentWalker.java:
##########
@@ -470,6 +474,8 @@ private static class SinkMetricsEmittingQueryRunner<T> implements QueryRunner<T>
private final Set<String> metricsToCompute;
@Nullable
private final String segmentId;
+ @Nullable
+ private final Integer localSegmentCount;
Review Comment:
local to this data node
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] feat: add query/segment/count metric to data nodes (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #19624:
URL: https://github.com/apache/druid/pull/19624#discussion_r3474031233
##########
server/src/main/java/org/apache/druid/segment/realtime/appenderator/SinkQuerySegmentWalker.java:
##########
@@ -470,6 +474,8 @@ private static class SinkMetricsEmittingQueryRunner<T> implements QueryRunner<T>
private final Set<String> metricsToCompute;
@Nullable
private final String segmentId;
+ @Nullable
+ private final Integer localSegmentCount;
Review Comment:
I feel like that is implicit, since the data node is not aware of segments from other nodes anyway.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] feat: add query/segments/count metric to data nodes (druid)
Posted by "jtuglu1 (via GitHub)" <gi...@apache.org>.
jtuglu1 commented on code in PR #19624:
URL: https://github.com/apache/druid/pull/19624#discussion_r3493905570
##########
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:
Generally metric renaming is a pain since we need to break user's metric setups. I feel like they both are measuring the same thing for now, so we can potentially address in a follow-up to rename the broker-side ones.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] feat: add query/segment/count metric to data nodes (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #19624:
URL: https://github.com/apache/druid/pull/19624#discussion_r3479826605
##########
processing/src/main/java/org/apache/druid/query/QueryMetrics.java:
##########
@@ -333,6 +333,14 @@ default void filterBundle(FilterBundle.BundleInfo bundleInfo)
*/
QueryMetrics<QueryType> reportQueriedSegmentCount(long segmentCount);
+ /**
+ * Registers the number of segments scanned by this data node for the query.
+ */
+ default QueryMetrics<QueryType> reportLocalSegmentCount(long segmentCount)
Review Comment:
@jtuglu1 , could you please add some info in the javadoc of the existing `reportQueriedSegmentCount()` metric to clearly distinguish that method from `reportLocalSegmentCount()`?
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org