You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@iotdb.apache.org by "OneSizeFitsQuorum (via GitHub)" <gi...@apache.org> on 2023/07/23 12:24:03 UTC
[GitHub] [iotdb] OneSizeFitsQuorum commented on a diff in pull request #10623: Fixing partially concurrent GC's duration statistics and adding a new GC monitor
OneSizeFitsQuorum commented on code in PR #10623:
URL: https://github.com/apache/iotdb/pull/10623#discussion_r1271407454
##########
iotdb-core/metrics/interface/src/main/java/org/apache/iotdb/metrics/metricsets/jvm/JvmGcMetrics.java:
##########
@@ -69,6 +72,43 @@ public JvmGcMetrics() {
}
}
+ private static boolean isPartiallyConcurrentGC(GarbageCollectorMXBean gc) {
+ switch (gc.getName()) {
+ // First two are from the serial collector
+ case "Copy":
+ case "MarkSweepCompact":
+ // Parallel collector
+ case "PS MarkSweep":
+ case "PS Scavenge":
+ case "G1 Young Generation":
+ // CMS young generation collector
+ case "ParNew":
+ return false;
+ case "ConcurrentMarkSweep":
+ case "G1 Old Generation":
+ return true;
+ default:
+ // Assume possibly concurrent if unsure
+ return true;
+ }
+ }
+
+ private static boolean isConcurrentPhase(String cause, String name) {
+ return "No GC".equals(cause) || "Shenandoah Cycles".equals(name);
+ }
+
+ private static boolean isYoungGenPool(String name) {
+ return name != null && name.endsWith("Eden Space") && name.endsWith("Survivor Space");
Review Comment:
`name.endsWith("Eden Space") || name.endsWith("Survivor Space")`?
##########
iotdb-core/metrics/interface/src/main/java/org/apache/iotdb/metrics/metricsets/jvm/JvmGcMetrics.java:
##########
@@ -116,17 +163,67 @@ public void bindTo(AbstractMetricService metricService) {
String gcAction = notificationInfo.getGcAction();
GcInfo gcInfo = notificationInfo.getGcInfo();
long duration = gcInfo.getDuration();
+
+ // The duration supplied in the notification info includes more than just
+ // application stopped time for concurrent GCs (since the concurrent phase is not
+ // stop-the-world).
+ // Try and do a better job coming up with a good stopped time
+ // value by asking for and tracking cumulative time spent blocked in GC.
+ if (isPartiallyConcurrentGC(mbean)) {
+ long previousTotal = lastGcTotalDuration.get();
+ long total = mbean.getCollectionTime();
Review Comment:
If there are 2 GCS, and only 1 meets the requirement of 'isPartiallyConcurrentGC', will the result be twice as large?
##########
iotdb-core/metrics/interface/src/main/java/org/apache/iotdb/metrics/metricsets/jvm/JvmGcMetrics.java:
##########
@@ -116,17 +163,67 @@ public void bindTo(AbstractMetricService metricService) {
String gcAction = notificationInfo.getGcAction();
GcInfo gcInfo = notificationInfo.getGcInfo();
long duration = gcInfo.getDuration();
+
+ // The duration supplied in the notification info includes more than just
+ // application stopped time for concurrent GCs (since the concurrent phase is not
+ // stop-the-world).
+ // Try and do a better job coming up with a good stopped time
+ // value by asking for and tracking cumulative time spent blocked in GC.
+ if (isPartiallyConcurrentGC(mbean)) {
+ long previousTotal = lastGcTotalDuration.get();
+ long total = mbean.getCollectionTime();
+ lastGcTotalDuration.set(total);
+ duration = total - previousTotal; // may be zero for a really fast collection
+ }
+ totalGcTimeSpend.set(totalGcTimeSpend.get() + duration);
+
String timerName;
if (isConcurrentPhase(gcCause, notificationInfo.getGcName())) {
- timerName = "jvm_gc_concurrent_phase_time";
+ timerName = "jvm_gc_concurrent_phase_time_";
} else {
- timerName = "jvm_gc_pause";
+ timerName = "jvm_gc_pause_";
}
+ String reformatCause = gcCause.replaceAll(" ", "_");
Review Comment:
do we need a different timerName as you can tell them apart by the tag of gccause?
##########
iotdb-core/metrics/interface/src/main/java/org/apache/iotdb/metrics/metricsets/jvm/JvmGcMetrics.java:
##########
@@ -69,6 +72,43 @@ public JvmGcMetrics() {
}
}
+ private static boolean isPartiallyConcurrentGC(GarbageCollectorMXBean gc) {
Review Comment:
add some detailed comments for Why do some values return false or true?
##########
iotdb-core/node-commons/src/main/java/org/apache/iotdb/commons/service/metric/JvmGcMetrics.java:
##########
@@ -0,0 +1,278 @@
+/*
+ * 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.iotdb.commons.service.metric;
+
+import org.apache.iotdb.commons.concurrent.IoTDBThreadPoolFactory;
+import org.apache.iotdb.commons.concurrent.threadpool.ScheduledExecutorUtil;
+import org.apache.iotdb.metrics.AbstractMetricService;
+import org.apache.iotdb.metrics.metricsets.IMetricSet;
+import org.apache.iotdb.metrics.utils.MetricLevel;
+import org.apache.iotdb.metrics.utils.MetricType;
+
+import com.google.common.base.Preconditions;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.lang.management.GarbageCollectorMXBean;
+import java.lang.management.ManagementFactory;
+import java.util.concurrent.Future;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicLong;
+
+public class JvmGcMetrics implements IMetricSet {
Review Comment:
It is recommended to use a different name, as it would duplicate the classes in the metric package
##########
iotdb-core/metrics/interface/src/main/java/org/apache/iotdb/metrics/metricsets/jvm/JvmGcMetrics.java:
##########
@@ -116,17 +163,67 @@ public void bindTo(AbstractMetricService metricService) {
String gcAction = notificationInfo.getGcAction();
GcInfo gcInfo = notificationInfo.getGcInfo();
long duration = gcInfo.getDuration();
Review Comment:
When does the time get wrong here? Can you add some comments to list an example?
##########
iotdb-core/metrics/interface/src/main/java/org/apache/iotdb/metrics/metricsets/jvm/JvmGcMetrics.java:
##########
@@ -226,11 +328,23 @@ public void unbindFrom(AbstractMetricService metricService) {
String gcAction = notificationInfo.getGcAction();
String timerName;
if (isConcurrentPhase(gcCause, notificationInfo.getGcName())) {
- timerName = "jvm_gc_concurrent_phase_time";
+ timerName = "jvm_gc_concurrent_phase_time_";
} else {
- timerName = "jvm_gc_pause";
+ timerName = "jvm_gc_pause_";
}
+ timerName += gcCause;
Review Comment:
same as above
##########
iotdb-core/metrics/interface/src/main/java/org/apache/iotdb/metrics/metricsets/jvm/JvmGcMetrics.java:
##########
@@ -116,17 +163,67 @@ public void bindTo(AbstractMetricService metricService) {
String gcAction = notificationInfo.getGcAction();
GcInfo gcInfo = notificationInfo.getGcInfo();
long duration = gcInfo.getDuration();
+
+ // The duration supplied in the notification info includes more than just
+ // application stopped time for concurrent GCs (since the concurrent phase is not
+ // stop-the-world).
+ // Try and do a better job coming up with a good stopped time
+ // value by asking for and tracking cumulative time spent blocked in GC.
+ if (isPartiallyConcurrentGC(mbean)) {
+ long previousTotal = lastGcTotalDuration.get();
+ long total = mbean.getCollectionTime();
+ lastGcTotalDuration.set(total);
+ duration = total - previousTotal; // may be zero for a really fast collection
+ }
+ totalGcTimeSpend.set(totalGcTimeSpend.get() + duration);
+
String timerName;
if (isConcurrentPhase(gcCause, notificationInfo.getGcName())) {
- timerName = "jvm_gc_concurrent_phase_time";
+ timerName = "jvm_gc_concurrent_phase_time_";
} else {
- timerName = "jvm_gc_pause";
+ timerName = "jvm_gc_pause_";
}
+ String reformatCause = gcCause.replaceAll(" ", "_");
+ // create a timer for each cause, which binds gcCause with gcDuration
+ timerName += reformatCause;
Timer timer =
metricService.getOrCreateTimer(
timerName, MetricLevel.CORE, "action", gcAction, "cause", gcCause);
timer.update(duration, TimeUnit.MILLISECONDS);
+ // add support for ZGC
+ if (mbean.getName().equals("ZGC Cycles")) {
+ Counter cyclesCount =
+ metricService.getOrCreateCounter("jvm_zgc_cycles_count", MetricLevel.CORE);
+ cyclesCount.inc();
+ } else if (mbean.getName().equals("ZGC Pauses")) {
+ Counter pausesCount =
+ metricService.getOrCreateCounter("jvm_zgc_pauses_count", MetricLevel.CORE);
+ pausesCount.inc();
+ }
+
+ // monitoring old/young GC count, which is helpful for users to locate GC exception.
+ if (GcGenerationAge.fromName(notificationInfo.getGcName()) == GcGenerationAge.OLD) {
+ Counter oldGcCounter =
+ metricService.getOrCreateCounter("jvm_gc_old_gc_count", MetricLevel.CORE);
+ oldGcCounter.inc();
+ } else if (GcGenerationAge.fromName(notificationInfo.getGcName())
+ == GcGenerationAge.YOUNG) {
+ Counter youngGcCounter =
+ metricService.getOrCreateCounter("jvm_gc_young_gc_count", MetricLevel.CORE);
+ youngGcCounter.inc();
Review Comment:
can we record mixed gc count for G1?
##########
iotdb-core/node-commons/src/main/java/org/apache/iotdb/commons/service/metric/JvmGcMetrics.java:
##########
@@ -0,0 +1,278 @@
+/*
+ * 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.iotdb.commons.service.metric;
+
+import org.apache.iotdb.commons.concurrent.IoTDBThreadPoolFactory;
+import org.apache.iotdb.commons.concurrent.threadpool.ScheduledExecutorUtil;
+import org.apache.iotdb.metrics.AbstractMetricService;
+import org.apache.iotdb.metrics.metricsets.IMetricSet;
+import org.apache.iotdb.metrics.utils.MetricLevel;
+import org.apache.iotdb.metrics.utils.MetricType;
+
+import com.google.common.base.Preconditions;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.lang.management.GarbageCollectorMXBean;
+import java.lang.management.ManagementFactory;
+import java.util.concurrent.Future;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicLong;
+
+public class JvmGcMetrics implements IMetricSet {
+ private static final Logger logger = LoggerFactory.getLogger(JvmGcMetrics.class);
+ private final ScheduledExecutorService scheduledGCInfoMonitor =
+ IoTDBThreadPoolFactory.newSingleThreadScheduledExecutor("JVM-GC-Statistics-Monitor");
Review Comment:
move this magic string to `ThreadName`
##########
iotdb-core/node-commons/src/main/java/org/apache/iotdb/commons/service/metric/JvmGcMetrics.java:
##########
@@ -0,0 +1,278 @@
+/*
+ * 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.iotdb.commons.service.metric;
+
+import org.apache.iotdb.commons.concurrent.IoTDBThreadPoolFactory;
+import org.apache.iotdb.commons.concurrent.threadpool.ScheduledExecutorUtil;
+import org.apache.iotdb.metrics.AbstractMetricService;
+import org.apache.iotdb.metrics.metricsets.IMetricSet;
+import org.apache.iotdb.metrics.utils.MetricLevel;
+import org.apache.iotdb.metrics.utils.MetricType;
+
+import com.google.common.base.Preconditions;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.lang.management.GarbageCollectorMXBean;
+import java.lang.management.ManagementFactory;
+import java.util.concurrent.Future;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicLong;
+
+public class JvmGcMetrics implements IMetricSet {
+ private static final Logger logger = LoggerFactory.getLogger(JvmGcMetrics.class);
+ private final ScheduledExecutorService scheduledGCInfoMonitor =
+ IoTDBThreadPoolFactory.newSingleThreadScheduledExecutor("JVM-GC-Statistics-Monitor");
+ private Future<?> scheduledGcMonitorFuture;
+ // Ring buffers containing GC timings and timestamps when timings were taken
+ private final TsAndData[] gcDataBuf;
+ // Max GC time threshold
+ private final long maxGcTimePercentage = 70L;
+ // Duration of observation window
+ private final long observationWindowMs = TimeUnit.MINUTES.toMillis(1);
+ // Interval for data collection
+ private final long sleepIntervalMs = TimeUnit.SECONDS.toMillis(5);
+ // Buffer size
+ private final int bufSize;
+ // Buffer start index
+ private int startIdx;
+ // Buffer end index
+ private int endIdx;
+ // The time when jvm start running
+ private long startTime;
+ // Container to hold collected GC data
+ private final GcData curData = new GcData();
+ // Switch for GC monitor operation
+ private boolean shouldRun = true;
+ // Hook function called with GC exception
+ private final GcTimeAlertHandler alertHandler;
+
+ public JvmGcMetrics() {
+ bufSize = (int) (observationWindowMs / sleepIntervalMs + 2);
+ // Prevent the user from accidentally creating an abnormally big buffer, which will result in
+ // slow calculations and likely inaccuracy.
+ Preconditions.checkArgument(bufSize <= 128 * 1024);
+ gcDataBuf = new TsAndData[bufSize];
+ for (int i = 0; i < bufSize; i++) {
+ gcDataBuf[i] = new TsAndData();
+ }
+
+ alertHandler = new GcTimeAlerter();
+ }
+
+ @Override
+ public void bindTo(AbstractMetricService metricService) {
+ metricService.createAutoGauge(
+ "jvm_gc_accumulated_time_percentage",
+ MetricLevel.CORE,
+ curData,
+ GcData::getGcTimePercentage);
+
+ startTime = System.currentTimeMillis();
+ // current collect time: startTime + start delay(50ms)
+ curData.timestamp.set(startTime + TimeUnit.MILLISECONDS.toMillis(50));
+ gcDataBuf[startIdx].setValues(startTime, 0);
+ scheduledGcMonitorFuture =
+ ScheduledExecutorUtil.safelyScheduleWithFixedDelay(
+ scheduledGCInfoMonitor,
+ this::scheduledMonitoring,
+ TimeUnit.MILLISECONDS.toMillis(50), // to prevent / ZERO exception
+ sleepIntervalMs,
+ TimeUnit.MILLISECONDS);
+ }
+
+ @Override
+ public void unbindFrom(AbstractMetricService metricService) {
+ shouldRun = false;
+ metricService.remove(MetricType.AUTO_GAUGE, "jvm_gc_accumulated_time_percentage");
+ if (scheduledGcMonitorFuture != null) {
+ scheduledGcMonitorFuture.cancel(false);
+ scheduledGcMonitorFuture = null;
+ logger.info("JVM GC scheduled monitor is stopped successfully.");
+ }
+ }
+
+ private void scheduledMonitoring() {
+ if (shouldRun) {
+ calculateGCTimePercentageWithinObservedInterval();
+ }
+ // TODO: we can add an alertHandler here to handle the abnormal GC case.
Review Comment:
remove this comment?
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org