findingrish commented on code in PR #17360:
URL: https://github.com/apache/druid/pull/17360#discussion_r1829303606
##########
processing/src/test/java/org/apache/druid/query/groupby/GroupByQueryRunnerTest.java:
##########
@@ -13682,4 +13755,16 @@ private void cannotVectorize()
expectedException.expectMessage("Cannot vectorize!");
}
}
+
+ private void verifyMetrics(long queries, boolean skipMergeDictionary)
Review Comment:
Renamed, this is basically to skip verifying the merge dictionary size
metric, since some of the queries aggregate on non-string dimensions.
##########
processing/src/test/java/org/apache/druid/query/groupby/GroupByQueryRunnerTest.java:
##########
@@ -12383,6 +12448,10 @@ public void
testGroupByOnNullableDoubleNoLimitPushdown()
Iterable<ResultRow> results =
GroupByQueryRunnerTestHelper.runQuery(factory, runner, query);
TestHelper.assertExpectedObjects(expectedResults, results, "groupBy");
+
+ if (config.equals(V2_SMALL_BUFFER_CONFIG)) {
+ verifyMetrics(1, true);
+ }
Review Comment:
This is the most suited config to verify all the metrics. For example, in
config with larger buffer size it is not possible to verify disk spill metrics.
I am moved this check into the method and renamed the method.
##########
processing/src/main/java/org/apache/druid/query/groupby/GroupByStatsProvider.java:
##########
@@ -0,0 +1,191 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.query.groupby;
+
+import org.apache.druid.guice.LazySingleton;
+import org.apache.druid.query.QueryResourceId;
+
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.atomic.AtomicLong;
+
+/**
+ * Metrics collector for groupBy queries like spilled bytes, merge buffer
acquistion time, dictionary size.
+ */
+@LazySingleton
+public class GroupByStatsProvider
+{
+ private final Map<QueryResourceId, PerQueryStats> perQueryStats;
+ private final AggregateStats aggregateStatsContainer;
+
+ public GroupByStatsProvider()
+ {
+ this.perQueryStats = new ConcurrentHashMap<>();
+ this.aggregateStatsContainer = new AggregateStats();
+ }
+
+ public PerQueryStats getPerQueryStatsContainer(QueryResourceId resourceId)
+ {
+ if (resourceId == null) {
+ return null;
+ }
+ return perQueryStats.computeIfAbsent(resourceId, value -> new
PerQueryStats());
+ }
+
+ public synchronized void closeQuery(QueryResourceId resourceId)
+ {
+ if (resourceId == null || !perQueryStats.containsKey(resourceId)) {
+ return;
+ }
+ PerQueryStats container = perQueryStats.remove(resourceId);
+ aggregateStatsContainer.addQueryStats(container);
+ }
+
+ public synchronized AggregateStats getStatsSince()
+ {
+ return aggregateStatsContainer.reset();
+ }
+
+ public static class AggregateStats
Review Comment:
`PerQueryStats` is created per query and is closed at the end of the query,
`AggregateStats` is "reset" after each monitor cycle and all of the
`PerQueryStats` are accumulated into `AggregateStats` when the query is closed.
Monitor simply reports the aggregate metrics.
##########
processing/src/main/java/org/apache/druid/query/groupby/GroupByStatsProvider.java:
##########
@@ -0,0 +1,191 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.query.groupby;
+
+import org.apache.druid.guice.LazySingleton;
+import org.apache.druid.query.QueryResourceId;
+
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.atomic.AtomicLong;
+
+/**
+ * Metrics collector for groupBy queries like spilled bytes, merge buffer
acquistion time, dictionary size.
+ */
+@LazySingleton
Review Comment:
Yes, it works as expected. Guice should automatically detect classes marked
with this annotation and bind them in singleton scope.
##########
processing/src/main/java/org/apache/druid/query/groupby/GroupByStatsProvider.java:
##########
@@ -0,0 +1,191 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.query.groupby;
+
+import org.apache.druid.guice.LazySingleton;
+import org.apache.druid.query.QueryResourceId;
+
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.atomic.AtomicLong;
+
+/**
+ * Metrics collector for groupBy queries like spilled bytes, merge buffer
acquistion time, dictionary size.
+ */
+@LazySingleton
+public class GroupByStatsProvider
+{
+ private final Map<QueryResourceId, PerQueryStats> perQueryStats;
+ private final AggregateStats aggregateStatsContainer;
+
+ public GroupByStatsProvider()
+ {
+ this.perQueryStats = new ConcurrentHashMap<>();
+ this.aggregateStatsContainer = new AggregateStats();
+ }
+
+ public PerQueryStats getPerQueryStatsContainer(QueryResourceId resourceId)
+ {
+ if (resourceId == null) {
+ return null;
+ }
+ return perQueryStats.computeIfAbsent(resourceId, value -> new
PerQueryStats());
+ }
+
+ public synchronized void closeQuery(QueryResourceId resourceId)
+ {
+ if (resourceId == null || !perQueryStats.containsKey(resourceId)) {
+ return;
+ }
+ PerQueryStats container = perQueryStats.remove(resourceId);
+ aggregateStatsContainer.addQueryStats(container);
+ }
Review Comment:
> This is not intuitive. The callers won't remember calling closeQuery()
Even if it implements closable, it would require to be explicitly registered
with the query `closer`?
I am doing the same thing in `GroupByQueryQueryToolChest`
```
closer.register(() ->
groupByStatsProvider.closeQuery(query.context().getQueryResourceId()));
```
LMK what do you think?
--
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]
