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: [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]
