haridsv commented on code in PR #2337: URL: https://github.com/apache/phoenix/pull/2337#discussion_r2651089700
########## phoenix-core-client/src/main/java/org/apache/phoenix/monitoring/ScanMetricsGroup.java: ########## @@ -0,0 +1,163 @@ +/* + * 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.phoenix.monitoring; + +import java.util.Map; +import org.apache.hadoop.hbase.client.metrics.ScanMetrics; +import org.apache.hadoop.hbase.client.metrics.ScanMetricsRegionInfo; +import org.apache.phoenix.compat.hbase.CompatScanMetrics; + +import org.apache.hbase.thirdparty.com.google.gson.JsonArray; +import org.apache.hbase.thirdparty.com.google.gson.JsonObject; + +/** + * Captures scan metrics at both aggregate and region levels for HBase scans created by Phoenix + * clients. + * <p> + * Instances of this class are used to track and identify the slowest-performing scans. The slowness + * of a scan is measured by the total time spent between consecutive calls to + * {@link org.apache.hadoop.hbase.client.ResultScanner#next()} in the HBase client, which represents + * the actual data retrieval latency experienced by the client. + * <p> + * This class supports two modes of metric collection: + * <ul> + * <li>Aggregate metrics: Overall scan metrics across all regions</li> + * <li>Region-level metrics: Detailed metrics broken down by individual HBase regions</li> + * </ul> + * <p> + * Use {@link #toJson()} to obtain a JSON object representation of the scan metrics for + * serialization or reporting purposes. + */ +public class ScanMetricsGroup { + + private final String tableName; + private Map<String, Long> scanMetrics = null; + private Map<ScanMetricsRegionInfo, Map<String, Long>> scanMetricsByRegion = null; + private final Long sumOfMillisSecBetweenNexts; + + public ScanMetricsGroup(String tableName, Map<String, Long> scanMetrics) { + this.tableName = tableName; + this.scanMetrics = scanMetrics; + this.sumOfMillisSecBetweenNexts = scanMetrics.get(ScanMetrics.MILLIS_BETWEEN_NEXTS_METRIC_NAME); + } + + public ScanMetricsGroup(String tableName, + Map<ScanMetricsRegionInfo, Map<String, Long>> scanMetricsByRegion, + Map<String, Long> globalScanMetrics) { + this.tableName = tableName; + this.scanMetricsByRegion = scanMetricsByRegion; + this.sumOfMillisSecBetweenNexts = + globalScanMetrics.get(ScanMetrics.MILLIS_BETWEEN_NEXTS_METRIC_NAME); + } + + private MetricType getMetricType(String metricName) { Review Comment: How about moving this to MetricType enum? You could even add the HBase metric name to the enum constructor and create a lookup table dynamically by introspecting the enums with non-null HBase metric name. You can even have static getters to access the metric names and drive other parts of the code that have a long list of getters and setters to execute the logic reflectively instead of statically. A getter on the enum to access the hbase metric name can also help access the names via the enum for consistency. ########## phoenix-core-client/src/main/java/org/apache/phoenix/monitoring/ScanMetricsHolder.java: ########## @@ -74,10 +74,16 @@ public class ScanMetricsHolder { public static ScanMetricsHolder getInstance(ReadMetricQueue readMetrics, String tableName, Scan scan, LogLevel connectionLogLevel) { + return ScanMetricsHolder.getInstance(readMetrics, tableName, scan, connectionLogLevel, false); + } + + public static ScanMetricsHolder getInstance(ReadMetricQueue readMetrics, String tableName, + Scan scan, LogLevel connectionLogLevel, boolean isScanMetricsByRegionEnabled) { if (connectionLogLevel == LogLevel.OFF && !readMetrics.isRequestMetricsEnabled()) { return NO_OP_INSTANCE; } scan.setScanMetricsEnabled(true); + scan.setEnableScanMetricsByRegion(isScanMetricsByRegionEnabled); Review Comment: These 2 calls as a side effect of the getter method is not a good pattern. -- 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]
