kfaraz commented on code in PR #12456: URL: https://github.com/apache/druid/pull/12456#discussion_r852583990
########## sql/src/main/java/org/apache/druid/sql/calcite/schema/SchemaStatsMonitor.java: ########## @@ -0,0 +1,65 @@ +/* + * 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.sql.calcite.schema; + +import org.apache.druid.java.util.emitter.service.ServiceEmitter; +import org.apache.druid.java.util.emitter.service.ServiceMetricEvent; +import org.apache.druid.java.util.metrics.AbstractMonitor; +import org.apache.druid.query.DruidMetrics; +import org.apache.druid.timeline.SegmentId; + +import javax.inject.Inject; +import java.util.Map; +import java.util.concurrent.ConcurrentSkipListMap; + +/** + * A monitor that provides stats on segments that are visible to the {@link DruidSchema} + */ +public class SchemaStatsMonitor extends AbstractMonitor Review Comment: Nit: I guess a better name would be `SegmentStatsMonitor` as that is what we are really reporting, even though we are tapping into the `DruidSchema` for that info. ########## sql/src/main/java/org/apache/druid/sql/calcite/schema/DruidSchema.java: ########## @@ -825,6 +837,14 @@ Map<SegmentId, AvailableSegmentMetadata> getSegmentMetadataSnapshot() return segmentMetadata; } + /** + * @return segmentMetadataInfo. Callers can only safely read the contents of the map. + */ + public ConcurrentHashMap<String, ConcurrentSkipListMap<SegmentId, AvailableSegmentMetadata>> getSegmentMetadataInfoUnsafe() Review Comment: As we are placing the monitor in the same package and as this is marked to be an unsafe method, it should atleast be package protected. Seeing as this is a sensitive method, - should we annotate it as so? - should we return a copy of the map rather than the actual? ########## docs/operations/metrics.md: ########## @@ -244,7 +251,7 @@ These metrics are for the Druid Coordinator and are reset each time the Coordina |`segment/loadQueue/count`|Number of segments to load.|server.|Varies.| |`segment/dropQueue/count`|Number of segments to drop.|server.|Varies.| |`segment/size`|Total size of used segments in a data source. Emitted only for data sources to which at least one used segment belongs.|dataSource.|Varies.| -|`segment/count`|Number of used segments belonging to a data source. Emitted only for data sources to which at least one used segment belongs.|dataSource.|< max| +|`segsegment/count`|Number of used segments belonging to a data source. Emitted only for data sources to which at least one used segment belongs.|dataSource.|< max| Review Comment: Is this a typo? Reading the description, it seems that this should rather be reported as `segment/used/count`. What do you think? ########## docs/operations/metrics.md: ########## @@ -58,6 +58,13 @@ Metrics may have additional dimensions beyond those listed above. |`query/priority`|Assigned lane and priority, only if Laning strategy is enabled. Refer to [Laning strategies](../configuration/index.md#laning-strategies)|lane, dataSource, type|0| |`sqlQuery/time`|Milliseconds taken to complete a SQL query.|id, nativeQueryIds, dataSource, remoteAddress, success.|< 1s| |`sqlQuery/bytes`|Number of bytes returned in the SQL query response.|id, nativeQueryIds, dataSource, remoteAddress, success.| | +|`segment/detailed/numRows`|Number of rows in a segment. Only reported if `SchemaStatsMonitor` is enabled.|`dataSource`, `segment/binaryVersion`, `segment/hasLastCompactedState`| Varies. Good sized segments should be around 5M rows.| +|`segment/detailed/numReplicas`|Number servers that this segment is available on. Only reported if `SchemaStatsMonitor` is enabled.|`dataSource`, `segment/binaryVersion`, `segment/hasLastCompactedState`| Varies. Should be the number of replicas configured.| +|`segment/detailed/sizeBytes`|Size of the segment in bytes. Only reported if `SchemaStatsMonitor` is enabled.|`dataSource`, `segment/binaryVersion`, `segment/hasLastCompactedState`| Varies.| +|`segment/detailed/numDimensions`|Number of dimensions in the segment metadata. Only reported if `SchemaStatsMonitor` is enabled.|`dataSource`, `segment/binaryVersion`, `segment/hasLastCompactedState`| Varies.| +|`segment/detailed/numMetrics`|Number of metrics in the segment metadata. Only reported if `SchemaStatsMonitor` is enabled.|`dataSource`, `segment/binaryVersion`, `segment/hasLastCompactedState`| Varies.| +|`segment/detailed/durationMillis`|The size of the interval that the segment covers in millis. Only reported if `SchemaStatsMonitor` is enabled.|`dataSource`, `segment/binaryVersion`, `segment/hasLastCompactedState`| Varies.| Review Comment: Suggestion: `durationMillis` seems a little ambiguous. Maybe `segIntervalMillis`? -- 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]
