You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2022/04/25 11:38:42 UTC
[GitHub] [druid] liujianhuanzz opened a new pull request, #12481: Use MXBeans to get GC metrics #12476
liujianhuanzz opened a new pull request, #12481:
URL: https://github.com/apache/druid/pull/12481
Fixes src/main/java/org/apache/druid/java/util/metrics/JvmMonitor.java
### Description
<!-- Describe the goal of this PR, what problem are you fixing. If there is a corresponding issue (referenced above), it's not necessary to repeat the description here, however, you may choose to keep one summary sentence. -->
<!-- Describe your patch: what did you change in code? How did you fix the problem? -->
<!-- If there are several relatively logically separate changes in this PR, create a mini-section for each of them. For example: -->
#### Use MXBeans to get GC metrics
#12476
<!--
In each section, please describe design decisions made, including:
- Choice of algorithms
- Behavioral aspects. What configuration values are acceptable? How are corner cases and error conditions handled, such as when there are insufficient resources?
- Class organization and design (how the logic is split between classes, inheritance, composition, design patterns)
- Method organization and design (how the logic is split between methods, parameters and return types)
- Naming (class, method, API, configuration, HTTP endpoint, names of emitted metrics)
-->
<!-- It's good to describe an alternative design (or mention an alternative name) for every design (or naming) decision point and compare the alternatives with the designs that you've implemented (or the names you've chosen) to highlight the advantages of the chosen designs and names. -->
<!-- If there was a discussion of the design of the feature implemented in this PR elsewhere (e. g. a "Proposal" issue, any other issue, or a thread in the development mailing list), link to that discussion from this PR description and explain what have changed in your final design compared to your original proposal or the consensus version in the end of the discussion. If something hasn't changed since the original discussion, you can omit a detailed discussion of those aspects of the design here, perhaps apart from brief mentioning for the sake of readability of this PR description. -->
<!-- Some of the aspects mentioned above may be omitted for simple and small changes. -->
<hr>
##### Key changed/added classes in this PR
* `JvmMonitor`
<hr>
<!-- Check the items by putting "x" in the brackets for the done things. Not all of these items apply to every PR. Remove the items which are not done or not relevant to the PR. None of the items from the checklist below are strictly necessary, but it would be very helpful if you at least self-review the PR. -->
This PR has:
- [x] been self-reviewed.
- [ ] added documentation for new or modified features or behaviors.
- [ ] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
- [ ] added or updated version, license, or notice information in [licenses.yaml](https://github.com/apache/druid/blob/master/dev/license.md)
- [ ] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [ ] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
- [ ] added integration tests.
- [x] been tested in a test Druid cluster.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on pull request #12481: Use MXBeans to get GC metrics #12476
Posted by GitBox <gi...@apache.org>.
gianm commented on PR #12481:
URL: https://github.com/apache/druid/pull/12481#issuecomment-1110559737
@liujianhuanzz and @FrankChen021 it looks like this patch is a better alternative to my quick fix #12469. Should I close that one in favor of this one? Or is there some reason to do both?
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] liujianhuanzz commented on pull request #12481: Use MXBeans to get GC metrics #12476
Posted by GitBox <gi...@apache.org>.
liujianhuanzz commented on PR #12481:
URL: https://github.com/apache/druid/pull/12481#issuecomment-1109260632
> I took a quick look at the implementation, there're two things we need to do:
>
> 1. [JvmMonitor: Handle more generation and collector scenarios. #12469](https://github.com/apache/druid/pull/12469) contains some changes that add the extra `--add-exports` JVM argument. We should remove these changes in this PR
> 2. [JvmMonitor: Handle more generation and collector scenarios. #12469](https://github.com/apache/druid/pull/12469) and [Metrics for shenandoah #12369](https://github.com/apache/druid/pull/12369) added ZGC and Shenandoah, we should not forget adding support for these GC in this PR.
For ZGC and Shenandoah, there is a problem: how to correspond to the young/old generation.
In the current implementation
- a ZGC generation variable is created
- "Shenandoah Cycles" -> young
- "Shenandoah Pauses" -> old
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 commented on a diff in pull request #12481: Use MXBeans to get GC metrics #12476
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on code in PR #12481:
URL: https://github.com/apache/druid/pull/12481#discussion_r864471109
##########
core/src/main/java/org/apache/druid/java/util/metrics/JvmMonitor.java:
##########
@@ -167,118 +144,117 @@ private void emitGcMetrics(ServiceEmitter emitter)
}
}
- @Nullable
- private GcCounters tryCreateGcCounters()
- {
- try {
- return new GcCounters();
- }
- catch (RuntimeException e) {
- // in JDK11 jdk.internal.perf.Perf is not accessible, unless
- // --add-exports java.base/jdk.internal.perf=ALL-UNNAMED is set
- log.warn("Cannot initialize GC counters. If running JDK11 and above,"
- + " add --add-exports=java.base/jdk.internal.perf=ALL-UNNAMED"
- + " to the JVM arguments to enable GC counters.");
- }
- return null;
- }
-
- @VisibleForTesting
- static IntSet getGcGenerations(final Set<String> jStatCounterNames)
- {
- final IntSet retVal = new IntRBTreeSet();
-
- for (final String counterName : jStatCounterNames) {
- final Matcher m = PATTERN_GC_GENERATION.matcher(counterName);
- if (m.matches()) {
- retVal.add(Integer.parseInt(m.group(1)));
- }
- }
-
- return retVal;
- }
-
- @VisibleForTesting
- static String getGcGenerationName(final int genIndex)
- {
- switch (genIndex) {
- case 0:
- return "young";
- case 1:
- return "old";
- case 2:
- // Removed in Java 8 but still actual for previous Java versions
- return "perm";
- default:
- return String.valueOf(genIndex);
- }
- }
-
/**
* The following GC-related code is partially based on
* https://github.com/aragozin/jvm-tools/blob/e0e37692648951440aa1a4ea5046261cb360df70/
* sjk-core/src/main/java/org/gridkit/jvmtool/PerfCounterGcCpuUsageMonitor.java
*/
private class GcCounters
{
- private final List<GcGeneration> generations = new ArrayList<>();
+ private final List<GcCollectors> generations = new ArrayList<>();
GcCounters()
{
- // connect to itself
- final JStatData jStatData = JStatData.connect(pid);
- final Map<String, JStatData.Counter<?>> jStatCounters = jStatData.getAllCounters();
-
- for (int genIndex : getGcGenerations(jStatCounters.keySet())) {
- generations.add(new GcGeneration(jStatCounters, genIndex, getGcGenerationName(genIndex)));
+ List<GarbageCollectorMXBean> collectorMxBeans = ManagementFactory.getGarbageCollectorMXBeans();
+ for (GarbageCollectorMXBean collectorMxBean : collectorMxBeans) {
+ generations.add(new GcCollectors(collectorMxBean));
}
+
}
void emit(ServiceEmitter emitter, Map<String, String[]> dimensions)
{
- for (GcGeneration generation : generations) {
+ for (GcCollectors generation : generations) {
generation.emit(emitter, dimensions);
}
}
}
- private class GcGeneration
+ private class GcCollectors
{
- private final String name;
- @Nullable
+ private final String generation;
+ private final String collectorName;
private final GcGenerationCollector collector;
private final List<GcGenerationSpace> spaces = new ArrayList<>();
- GcGeneration(Map<String, JStatData.Counter<?>> jStatCounters, long genIndex, String name)
+ private static final String GC_YOUNG_GENERATION_NAME = "young";
+ private static final String GC_OLD_GENERATION_NAME = "old";
+ private static final String GC_ZGC_GENERATION_NAME = "zgc";
+
+ private static final String GC_CMS_NAME = "cms";
+ private static final String GC_G1_NAME = "g1";
+ private static final String GC_PARALLEL_NAME = "parallel";
+ private static final String GC_SERIAL_NAME = "serial";
+ private static final String GC_ZGC_NAME = "zgc";
+ private static final String GC_SHENANDOAN_NAME = "shenandoah";
Review Comment:
It's better to rename these variables as XXX_COLLECTOR_NAME since the 'name' itself is too ambiguous.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 commented on a diff in pull request #12481: Use MXBeans to get GC metrics #12476
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on code in PR #12481:
URL: https://github.com/apache/druid/pull/12481#discussion_r864470259
##########
core/src/main/java/org/apache/druid/java/util/metrics/JvmMonitor.java:
##########
@@ -167,118 +144,117 @@ private void emitGcMetrics(ServiceEmitter emitter)
}
}
- @Nullable
- private GcCounters tryCreateGcCounters()
- {
- try {
- return new GcCounters();
- }
- catch (RuntimeException e) {
- // in JDK11 jdk.internal.perf.Perf is not accessible, unless
- // --add-exports java.base/jdk.internal.perf=ALL-UNNAMED is set
- log.warn("Cannot initialize GC counters. If running JDK11 and above,"
- + " add --add-exports=java.base/jdk.internal.perf=ALL-UNNAMED"
- + " to the JVM arguments to enable GC counters.");
- }
- return null;
- }
-
- @VisibleForTesting
- static IntSet getGcGenerations(final Set<String> jStatCounterNames)
- {
- final IntSet retVal = new IntRBTreeSet();
-
- for (final String counterName : jStatCounterNames) {
- final Matcher m = PATTERN_GC_GENERATION.matcher(counterName);
- if (m.matches()) {
- retVal.add(Integer.parseInt(m.group(1)));
- }
- }
-
- return retVal;
- }
-
- @VisibleForTesting
- static String getGcGenerationName(final int genIndex)
- {
- switch (genIndex) {
- case 0:
- return "young";
- case 1:
- return "old";
- case 2:
- // Removed in Java 8 but still actual for previous Java versions
- return "perm";
- default:
- return String.valueOf(genIndex);
- }
- }
-
/**
* The following GC-related code is partially based on
* https://github.com/aragozin/jvm-tools/blob/e0e37692648951440aa1a4ea5046261cb360df70/
* sjk-core/src/main/java/org/gridkit/jvmtool/PerfCounterGcCpuUsageMonitor.java
*/
private class GcCounters
{
- private final List<GcGeneration> generations = new ArrayList<>();
+ private final List<GcCollectors> generations = new ArrayList<>();
GcCounters()
{
- // connect to itself
- final JStatData jStatData = JStatData.connect(pid);
- final Map<String, JStatData.Counter<?>> jStatCounters = jStatData.getAllCounters();
-
- for (int genIndex : getGcGenerations(jStatCounters.keySet())) {
- generations.add(new GcGeneration(jStatCounters, genIndex, getGcGenerationName(genIndex)));
+ List<GarbageCollectorMXBean> collectorMxBeans = ManagementFactory.getGarbageCollectorMXBeans();
+ for (GarbageCollectorMXBean collectorMxBean : collectorMxBeans) {
+ generations.add(new GcCollectors(collectorMxBean));
}
+
}
void emit(ServiceEmitter emitter, Map<String, String[]> dimensions)
{
- for (GcGeneration generation : generations) {
+ for (GcCollectors generation : generations) {
generation.emit(emitter, dimensions);
}
}
}
- private class GcGeneration
+ private class GcCollectors
Review Comment:
Should be `GcCollector`
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 commented on pull request #12481: Use MXBeans to get GC metrics #12476
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on PR #12481:
URL: https://github.com/apache/druid/pull/12481#issuecomment-1177168256
Hi @abhishekagarwal87 , Since @gianm made some changes to this part in a previous PR, I think it's better for him to take a look.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] liujianhuanzz commented on pull request #12481: Use MXBeans to get GC metrics #12476
Posted by GitBox <gi...@apache.org>.
liujianhuanzz commented on PR #12481:
URL: https://github.com/apache/druid/pull/12481#issuecomment-1177112815
@gianm Could you also review this PR when you're at convenience?
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 commented on a diff in pull request #12481: Use MXBeans to get GC metrics #12476
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on code in PR #12481:
URL: https://github.com/apache/druid/pull/12481#discussion_r908598571
##########
core/src/main/java/org/apache/druid/java/util/metrics/JvmMonitor.java:
##########
@@ -162,224 +143,184 @@ private void emitDirectMemMetrics(ServiceEmitter emitter)
private void emitGcMetrics(ServiceEmitter emitter)
{
- if (gcCounters != null) {
- gcCounters.emit(emitter, dimensions);
- }
- }
-
- @Nullable
- private GcCounters tryCreateGcCounters()
- {
- try {
- return new GcCounters();
+ if (gcCollectors != null) {
+ gcCollectors.emit(emitter, dimensions);
}
- catch (RuntimeException e) {
- // in JDK11 jdk.internal.perf.Perf is not accessible, unless
- // --add-exports java.base/jdk.internal.perf=ALL-UNNAMED is set
- log.warn("Cannot initialize GC counters. If running JDK11 and above,"
- + " add --add-exports=java.base/jdk.internal.perf=ALL-UNNAMED"
- + " to the JVM arguments to enable GC counters.");
- }
- return null;
}
- @VisibleForTesting
- static IntSet getGcGenerations(final Set<String> jStatCounterNames)
+ private class GcCollectors
{
- final IntSet retVal = new IntRBTreeSet();
+ private final List<GcCollector> collectors = new ArrayList<>();
+ private final List<GcSpaceCollector> spaceCollectors = new ArrayList<>();
- for (final String counterName : jStatCounterNames) {
- final Matcher m = PATTERN_GC_GENERATION.matcher(counterName);
- if (m.matches()) {
- retVal.add(Integer.parseInt(m.group(1)));
+ GcCollectors()
+ {
+ List<GarbageCollectorMXBean> collectorMxBeans = ManagementFactory.getGarbageCollectorMXBeans();
+ for (GarbageCollectorMXBean collectorMxBean : collectorMxBeans) {
+ collectors.add(new GcCollector(collectorMxBean));
}
- }
- return retVal;
- }
+ List<MemoryPoolMXBean> memoryPoolMXBeans = ManagementFactory.getMemoryPoolMXBeans();
+ for (MemoryPoolMXBean memoryPoolMXBean : memoryPoolMXBeans) {
+ spaceCollectors.add(new GcSpaceCollector(memoryPoolMXBean));
+ }
- @VisibleForTesting
- static String getGcGenerationName(final int genIndex)
- {
- switch (genIndex) {
- case 0:
- return "young";
- case 1:
- return "old";
- case 2:
- // Removed in Java 8 but still actual for previous Java versions
- return "perm";
- default:
- return String.valueOf(genIndex);
}
- }
-
- /**
- * The following GC-related code is partially based on
- * https://github.com/aragozin/jvm-tools/blob/e0e37692648951440aa1a4ea5046261cb360df70/
- * sjk-core/src/main/java/org/gridkit/jvmtool/PerfCounterGcCpuUsageMonitor.java
- */
- private class GcCounters
- {
- private final List<GcGeneration> generations = new ArrayList<>();
- GcCounters()
+ void emit(ServiceEmitter emitter, Map<String, String[]> dimensions)
{
- // connect to itself
- final JStatData jStatData = JStatData.connect(pid);
- final Map<String, JStatData.Counter<?>> jStatCounters = jStatData.getAllCounters();
-
- for (int genIndex : getGcGenerations(jStatCounters.keySet())) {
- generations.add(new GcGeneration(jStatCounters, genIndex, getGcGenerationName(genIndex)));
+ for (GcCollector collector : collectors) {
+ collector.emit(emitter, dimensions);
}
- }
- void emit(ServiceEmitter emitter, Map<String, String[]> dimensions)
- {
- for (GcGeneration generation : generations) {
- generation.emit(emitter, dimensions);
+ for (GcSpaceCollector spaceCollector : spaceCollectors) {
+ spaceCollector.emit(emitter, dimensions);
}
}
}
- private class GcGeneration
+ private class GcCollector
Review Comment:
This `GcCollector` and its member `GcGenerationCollector collector` is a little bit confusing. I think we can blend the `GcGenerationCollector` into this class, and call current `GcCollector` as `GcGenerationCollector`.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] liujianhuanzz commented on a diff in pull request #12481: Use MXBeans to get GC metrics #12476
Posted by GitBox <gi...@apache.org>.
liujianhuanzz commented on code in PR #12481:
URL: https://github.com/apache/druid/pull/12481#discussion_r859436720
##########
core/src/main/java/org/apache/druid/java/util/metrics/JvmMonitor.java:
##########
@@ -183,64 +169,110 @@ private GcCounters tryCreateGcCounters()
*/
private class GcCounters
{
- private final List<GcGeneration> generations = new ArrayList<>();
+ private final List<GcCollector> generations = new ArrayList<>();
GcCounters()
Review Comment:
This patch is tested on jdk11 and jdk15. Both `--add-exports java.base/jdk.internal.perf=ALL-UNNAMED` and `tryCreateGcCounters` are no longer needed
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 commented on pull request #12481: Use MXBeans to get GC metrics #12476
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on PR #12481:
URL: https://github.com/apache/druid/pull/12481#issuecomment-1110565819
@gianm The work in your patch is completed, right? If so I think we can first merge that one.
For this one, I have not checked the implementation for ZGC and Shenandoah has the same behaviour with previouse PRs.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 commented on pull request #12481: Use MXBeans to get GC metrics #12476
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on PR #12481:
URL: https://github.com/apache/druid/pull/12481#issuecomment-1168851744
@liujianhuanzz Thanks for your work. Sorry for the long time waiting for review. Generally is good, and some comments left for you for final revision.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] abhishekagarwal87 commented on pull request #12481: Use MXBeans to get GC metrics #12476
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on PR #12481:
URL: https://github.com/apache/druid/pull/12481#issuecomment-1177166178
@liujianhuanzz - thank you for your patience. can you address the conflicts?
@FrankChen021 - if it looks good to you, do we another approval from @gianm ?
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on pull request #12481: Use MXBeans to get GC metrics #12476
Posted by GitBox <gi...@apache.org>.
gianm commented on PR #12481:
URL: https://github.com/apache/druid/pull/12481#issuecomment-1168264415
Where did we end up on this patch? @FrankChen021 does it look good to you?
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 merged pull request #12481: Use MXBeans to get GC metrics #12476
Posted by GitBox <gi...@apache.org>.
FrankChen021 merged PR #12481:
URL: https://github.com/apache/druid/pull/12481
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on a diff in pull request #12481: Use MXBeans to get GC metrics #12476
Posted by GitBox <gi...@apache.org>.
gianm commented on code in PR #12481:
URL: https://github.com/apache/druid/pull/12481#discussion_r916876455
##########
pom.xml:
##########
@@ -120,7 +120,6 @@
<com.google.apis.client.version>1.26.0</com.google.apis.client.version>
<com.google.apis.compute.version>v1-rev20190607-${com.google.apis.client.version}</com.google.apis.compute.version>
<com.google.apis.storage.version>v1-rev20190523-${com.google.apis.client.version}</com.google.apis.storage.version>
- <jdk.surefire.argLine><!-- empty placeholder --></jdk.surefire.argLine>
Review Comment:
thanks!
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 commented on pull request #12481: Use MXBeans to get GC metrics #12476
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on PR #12481:
URL: https://github.com/apache/druid/pull/12481#issuecomment-1108614371
Thank you @liujianhuanzz for taking this job.
This PR is a follow up of #12469, and should be merged after that PR.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on a diff in pull request #12481: Use MXBeans to get GC metrics #12476
Posted by GitBox <gi...@apache.org>.
gianm commented on code in PR #12481:
URL: https://github.com/apache/druid/pull/12481#discussion_r916144321
##########
pom.xml:
##########
@@ -120,7 +120,6 @@
<com.google.apis.client.version>1.26.0</com.google.apis.client.version>
<com.google.apis.compute.version>v1-rev20190607-${com.google.apis.client.version}</com.google.apis.compute.version>
<com.google.apis.storage.version>v1-rev20190523-${com.google.apis.client.version}</com.google.apis.storage.version>
- <jdk.surefire.argLine><!-- empty placeholder --></jdk.surefire.argLine>
Review Comment:
I see CI failures due to this being missing:
```
Error: Could not find or load main class ${jdk.surefire.argLine}
```
Can you keep this placeholder here?
If you want to remove lines that are no longer needed from the `java-9+` profile, go for it. However note that we do still need some of the lines, due to datasketches. The readme at https://github.com/apache/datasketches-memory mentions that some `--add-opens` and `--add-exports` are needed to work with direct memory and memory maps on JDK 11. And we need the empty placeholder for JDK 8.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] liujianhuanzz commented on a diff in pull request #12481: Use MXBeans to get GC metrics #12476
Posted by GitBox <gi...@apache.org>.
liujianhuanzz commented on code in PR #12481:
URL: https://github.com/apache/druid/pull/12481#discussion_r916492048
##########
pom.xml:
##########
@@ -120,7 +120,6 @@
<com.google.apis.client.version>1.26.0</com.google.apis.client.version>
<com.google.apis.compute.version>v1-rev20190607-${com.google.apis.client.version}</com.google.apis.compute.version>
<com.google.apis.storage.version>v1-rev20190523-${com.google.apis.client.version}</com.google.apis.storage.version>
- <jdk.surefire.argLine><!-- empty placeholder --></jdk.surefire.argLine>
Review Comment:
@gianm All checks have passed
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 commented on pull request #12481: Use MXBeans to get GC metrics #12476
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on PR #12481:
URL: https://github.com/apache/druid/pull/12481#issuecomment-1178609112
Thank you @liujianhuanzz for your work.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on a diff in pull request #12481: Use MXBeans to get GC metrics #12476
Posted by GitBox <gi...@apache.org>.
gianm commented on code in PR #12481:
URL: https://github.com/apache/druid/pull/12481#discussion_r916144321
##########
pom.xml:
##########
@@ -120,7 +120,6 @@
<com.google.apis.client.version>1.26.0</com.google.apis.client.version>
<com.google.apis.compute.version>v1-rev20190607-${com.google.apis.client.version}</com.google.apis.compute.version>
<com.google.apis.storage.version>v1-rev20190523-${com.google.apis.client.version}</com.google.apis.storage.version>
- <jdk.surefire.argLine><!-- empty placeholder --></jdk.surefire.argLine>
Review Comment:
I see CI failures due to this being missing:
```
Error: Could not find or load main class ${jdk.surefire.argLine}
```
Can you keep this placeholder here, and then remove the specific lines that are no longer needed from the `java-9+` profile?
We do still need some of the lines, due to datasketches. The readme at https://github.com/apache/datasketches-memory mentions that some `--add-opens` and `--add-exports` are needed to work with direct memory and memory maps on JDK 11. And we need the empty placeholder for JDK 8.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on a diff in pull request #12481: Use MXBeans to get GC metrics #12476
Posted by GitBox <gi...@apache.org>.
gianm commented on code in PR #12481:
URL: https://github.com/apache/druid/pull/12481#discussion_r859394873
##########
core/src/main/java/org/apache/druid/java/util/metrics/JvmMonitor.java:
##########
@@ -183,64 +169,110 @@ private GcCounters tryCreateGcCounters()
*/
private class GcCounters
{
- private final List<GcGeneration> generations = new ArrayList<>();
+ private final List<GcCollector> generations = new ArrayList<>();
GcCounters()
{
- // connect to itself
- final JStatData jStatData = JStatData.connect(pid);
- final Map<String, JStatData.Counter<?>> jStatCounters = jStatData.getAllCounters();
-
- generations.add(new GcGeneration(jStatCounters, 0, "young"));
- generations.add(new GcGeneration(jStatCounters, 1, "old"));
- // Removed in Java 8 but still actual for previous Java versions
- if (jStatCounters.containsKey("sun.gc.generation.2.name")) {
- generations.add(new GcGeneration(jStatCounters, 2, "perm"));
+ List<GarbageCollectorMXBean> collectorMxBeans = ManagementFactory.getGarbageCollectorMXBeans();
+ for (GarbageCollectorMXBean collectorMxBean : collectorMxBeans) {
+ generations.add(new GcCollector(collectorMxBean));
}
+
}
void emit(ServiceEmitter emitter, Map<String, String[]> dimensions)
{
- for (GcGeneration generation : generations) {
+ for (GcCollector generation : generations) {
generation.emit(emitter, dimensions);
}
}
}
- private class GcGeneration
+ private class GcCollector
{
- private final String name;
+ private final String generation;
+ private final String collectorName;
private final GcGenerationCollector collector;
private final List<GcGenerationSpace> spaces = new ArrayList<>();
- GcGeneration(Map<String, JStatData.Counter<?>> jStatCounters, long genIndex, String name)
- {
- this.name = StringUtils.toLowerCase(name);
+ private static final String GC_YOUNG_GENERATION_NAME = "young";
+ private static final String GC_OLD_GENERATION_NAME = "old";
+ private static final String GC_ZGC_GENERATION_NAME = "zgc";
+
+ private static final String GC_CMS_NAME = "cms";
+ private static final String GC_G1_NAME = "g1";
+ private static final String GC_PARALLEL_NAME = "parallel";
+ private static final String GC_SERIAL_NAME = "serial";
+ private static final String GC_ZGC_NAME = "zgc";
+ private static final String GC_SHENANDOAN_NAME = "shenandoah";
- long spacesCount = ((JStatData.LongCounter) jStatCounters.get(
- StringUtils.format("sun.gc.generation.%d.spaces", genIndex)
- )).getLong();
- for (long spaceIndex = 0; spaceIndex < spacesCount; spaceIndex++) {
- spaces.add(new GcGenerationSpace(jStatCounters, genIndex, spaceIndex));
+ GcCollector(GarbageCollectorMXBean gcBean)
+ {
+ Pair<String, String> gcNamePair = getReadableName(gcBean.getName());
+ this.generation = gcNamePair.lhs;
+ this.collectorName = gcNamePair.rhs;
+
+ collector = new GcGenerationCollector(gcBean);
+
+ List<MemoryPoolMXBean> memoryPoolMxBeans = ManagementFactory.getMemoryPoolMXBeans();
+ for (MemoryPoolMXBean memoryPoolMxBean : memoryPoolMxBeans) {
+ MemoryUsage collectionUsage = memoryPoolMxBean.getCollectionUsage();
+ if (collectionUsage != null) {
+ spaces.add(new GcGenerationSpace(collectionUsage, memoryPoolMxBean.getName()));
+ }
}
+ }
+
+ private Pair<String, String> getReadableName(String name)
+ {
+ switch (name) {
+ //CMS
+ case "ParNew":
+ return new Pair<>(GC_YOUNG_GENERATION_NAME, GC_CMS_NAME);
+ case "ConcurrentMarkSweep":
+ return new Pair<>(GC_OLD_GENERATION_NAME, GC_CMS_NAME);
+
+ // G1
+ case "G1 Young Generation":
+ return new Pair<>(GC_YOUNG_GENERATION_NAME, GC_G1_NAME);
+ case "G1 Old Generation":
+ return new Pair<>(GC_OLD_GENERATION_NAME, GC_G1_NAME);
+
+ // Parallel
+ case "PS Scavenge":
+ return new Pair<>(GC_YOUNG_GENERATION_NAME, GC_PARALLEL_NAME);
+ case "PS MarkSweep":
+ return new Pair<>(GC_OLD_GENERATION_NAME, GC_PARALLEL_NAME);
+
+ // Serial
+ case "Copy":
+ return new Pair<>(GC_YOUNG_GENERATION_NAME, GC_SERIAL_NAME);
+ case "MarkSweepCompact":
+ return new Pair<>(GC_OLD_GENERATION_NAME, GC_SERIAL_NAME);
+
+ //zgc
+ case "ZGC":
+ return new Pair<>(GC_ZGC_GENERATION_NAME, GC_ZGC_NAME);
+
+ //shenanoan
Review Comment:
Shenandoah (spelling)
##########
core/src/main/java/org/apache/druid/java/util/metrics/JvmMonitor.java:
##########
@@ -46,7 +43,6 @@ public class JvmMonitor extends FeedDefiningMonitor
private static final Logger log = new Logger(JvmMonitor.class);
private final Map<String, String[]> dimensions;
- private final long pid;
Review Comment:
🎉
##########
core/src/main/java/org/apache/druid/java/util/metrics/JvmMonitor.java:
##########
@@ -183,64 +169,110 @@ private GcCounters tryCreateGcCounters()
*/
private class GcCounters
{
- private final List<GcGeneration> generations = new ArrayList<>();
+ private final List<GcCollector> generations = new ArrayList<>();
GcCounters()
Review Comment:
Couple questions about the new mechanism:
1. Do we still need `--add-exports java.base/jdk.internal.perf=ALL-UNNAMED` on Java 11+ after these changes?
2. Can `new GcCounters()` still fail on some JVM versions or configurations?
If either of these is "no" then we should adjust the behavior of `tryCreateGcCounters`.
##########
core/src/main/java/org/apache/druid/java/util/metrics/JvmMonitor.java:
##########
@@ -183,64 +169,110 @@ private GcCounters tryCreateGcCounters()
*/
private class GcCounters
{
- private final List<GcGeneration> generations = new ArrayList<>();
+ private final List<GcCollector> generations = new ArrayList<>();
Review Comment:
Rename as `collectors`?
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 commented on pull request #12481: Use MXBeans to get GC metrics #12476
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on PR #12481:
URL: https://github.com/apache/druid/pull/12481#issuecomment-1108620718
I took a quick look at the implementation, there're two things we need to do:
1. #12469 contains some changes that add the extra `--add-exports` JVM argument. We should remove these changes in this PR
2. #12469 and #12369 added ZGC and Shenandoah, we should not forget adding support for these GC in this PR.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 commented on a diff in pull request #12481: Use MXBeans to get GC metrics #12476
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on code in PR #12481:
URL: https://github.com/apache/druid/pull/12481#discussion_r864468780
##########
core/src/main/java/org/apache/druid/java/util/metrics/JvmMonitor.java:
##########
@@ -167,118 +144,117 @@ private void emitGcMetrics(ServiceEmitter emitter)
}
}
- @Nullable
- private GcCounters tryCreateGcCounters()
- {
- try {
- return new GcCounters();
- }
- catch (RuntimeException e) {
- // in JDK11 jdk.internal.perf.Perf is not accessible, unless
- // --add-exports java.base/jdk.internal.perf=ALL-UNNAMED is set
- log.warn("Cannot initialize GC counters. If running JDK11 and above,"
- + " add --add-exports=java.base/jdk.internal.perf=ALL-UNNAMED"
- + " to the JVM arguments to enable GC counters.");
- }
- return null;
- }
-
- @VisibleForTesting
- static IntSet getGcGenerations(final Set<String> jStatCounterNames)
- {
- final IntSet retVal = new IntRBTreeSet();
-
- for (final String counterName : jStatCounterNames) {
- final Matcher m = PATTERN_GC_GENERATION.matcher(counterName);
- if (m.matches()) {
- retVal.add(Integer.parseInt(m.group(1)));
- }
- }
-
- return retVal;
- }
-
- @VisibleForTesting
- static String getGcGenerationName(final int genIndex)
- {
- switch (genIndex) {
- case 0:
- return "young";
- case 1:
- return "old";
- case 2:
- // Removed in Java 8 but still actual for previous Java versions
- return "perm";
- default:
- return String.valueOf(genIndex);
- }
- }
-
/**
* The following GC-related code is partially based on
* https://github.com/aragozin/jvm-tools/blob/e0e37692648951440aa1a4ea5046261cb360df70/
* sjk-core/src/main/java/org/gridkit/jvmtool/PerfCounterGcCpuUsageMonitor.java
*/
Review Comment:
These comments can be removed since JStatData is no longer used.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 commented on a diff in pull request #12481: Use MXBeans to get GC metrics #12476
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on code in PR #12481:
URL: https://github.com/apache/druid/pull/12481#discussion_r864474730
##########
core/src/main/java/org/apache/druid/java/util/metrics/JvmMonitor.java:
##########
@@ -108,10 +89,6 @@ private void emitThreadAllocationMetrics(ServiceEmitter emitter)
}
}
- /**
- * These metrics are going to be replaced by new jvm/gc/mem/* metrics
- */
- @Deprecated
Review Comment:
Don't know why it's marked as deprecated, and why you delete these
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] liujianhuanzz commented on pull request #12481: Use MXBeans to get GC metrics #12476
Posted by GitBox <gi...@apache.org>.
liujianhuanzz commented on PR #12481:
URL: https://github.com/apache/druid/pull/12481#issuecomment-1120216091
> Thank you @liujianhuanzz for working on the improvements. Some comments are left.
Done
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 commented on a diff in pull request #12481: Use MXBeans to get GC metrics #12476
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on code in PR #12481:
URL: https://github.com/apache/druid/pull/12481#discussion_r859420400
##########
core/src/main/java/org/apache/druid/java/util/metrics/JvmMonitor.java:
##########
@@ -183,64 +169,110 @@ private GcCounters tryCreateGcCounters()
*/
private class GcCounters
{
- private final List<GcGeneration> generations = new ArrayList<>();
+ private final List<GcCollector> generations = new ArrayList<>();
GcCounters()
Review Comment:
For the first question, I'm 100% sure that this JVM argument is no longer needed. Because the code in my repo supports JDK11 and plus, and I have never added such flag.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 commented on a diff in pull request #12481: Use MXBeans to get GC metrics #12476
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on code in PR #12481:
URL: https://github.com/apache/druid/pull/12481#discussion_r864469538
##########
core/src/main/java/org/apache/druid/java/util/metrics/JvmMonitor.java:
##########
@@ -167,118 +144,117 @@ private void emitGcMetrics(ServiceEmitter emitter)
}
}
- @Nullable
- private GcCounters tryCreateGcCounters()
- {
- try {
- return new GcCounters();
- }
- catch (RuntimeException e) {
- // in JDK11 jdk.internal.perf.Perf is not accessible, unless
- // --add-exports java.base/jdk.internal.perf=ALL-UNNAMED is set
- log.warn("Cannot initialize GC counters. If running JDK11 and above,"
- + " add --add-exports=java.base/jdk.internal.perf=ALL-UNNAMED"
- + " to the JVM arguments to enable GC counters.");
- }
- return null;
- }
-
- @VisibleForTesting
- static IntSet getGcGenerations(final Set<String> jStatCounterNames)
- {
- final IntSet retVal = new IntRBTreeSet();
-
- for (final String counterName : jStatCounterNames) {
- final Matcher m = PATTERN_GC_GENERATION.matcher(counterName);
- if (m.matches()) {
- retVal.add(Integer.parseInt(m.group(1)));
- }
- }
-
- return retVal;
- }
-
- @VisibleForTesting
- static String getGcGenerationName(final int genIndex)
- {
- switch (genIndex) {
- case 0:
- return "young";
- case 1:
- return "old";
- case 2:
- // Removed in Java 8 but still actual for previous Java versions
- return "perm";
- default:
- return String.valueOf(genIndex);
- }
- }
-
/**
* The following GC-related code is partially based on
* https://github.com/aragozin/jvm-tools/blob/e0e37692648951440aa1a4ea5046261cb360df70/
* sjk-core/src/main/java/org/gridkit/jvmtool/PerfCounterGcCpuUsageMonitor.java
*/
private class GcCounters
{
- private final List<GcGeneration> generations = new ArrayList<>();
+ private final List<GcCollectors> generations = new ArrayList<>();
Review Comment:
The variable should be renamed to `collectors`.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on pull request #12481: Use MXBeans to get GC metrics #12476
Posted by GitBox <gi...@apache.org>.
gianm commented on PR #12481:
URL: https://github.com/apache/druid/pull/12481#issuecomment-1168439238
> @gianm I have not checked this PR after last review. Let me pick up this PR again. I will try to finish the review in this week.
🙏 Thank you!
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 commented on a diff in pull request #12481: Use MXBeans to get GC metrics #12476
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on code in PR #12481:
URL: https://github.com/apache/druid/pull/12481#discussion_r908592680
##########
core/src/main/java/org/apache/druid/java/util/metrics/JvmMonitor.java:
##########
@@ -162,224 +143,184 @@ private void emitDirectMemMetrics(ServiceEmitter emitter)
private void emitGcMetrics(ServiceEmitter emitter)
{
- if (gcCounters != null) {
- gcCounters.emit(emitter, dimensions);
- }
- }
-
- @Nullable
- private GcCounters tryCreateGcCounters()
- {
- try {
- return new GcCounters();
+ if (gcCollectors != null) {
+ gcCollectors.emit(emitter, dimensions);
}
- catch (RuntimeException e) {
- // in JDK11 jdk.internal.perf.Perf is not accessible, unless
- // --add-exports java.base/jdk.internal.perf=ALL-UNNAMED is set
- log.warn("Cannot initialize GC counters. If running JDK11 and above,"
- + " add --add-exports=java.base/jdk.internal.perf=ALL-UNNAMED"
- + " to the JVM arguments to enable GC counters.");
- }
- return null;
}
- @VisibleForTesting
- static IntSet getGcGenerations(final Set<String> jStatCounterNames)
+ private class GcCollectors
{
- final IntSet retVal = new IntRBTreeSet();
+ private final List<GcCollector> collectors = new ArrayList<>();
+ private final List<GcSpaceCollector> spaceCollectors = new ArrayList<>();
- for (final String counterName : jStatCounterNames) {
- final Matcher m = PATTERN_GC_GENERATION.matcher(counterName);
- if (m.matches()) {
- retVal.add(Integer.parseInt(m.group(1)));
+ GcCollectors()
+ {
+ List<GarbageCollectorMXBean> collectorMxBeans = ManagementFactory.getGarbageCollectorMXBeans();
+ for (GarbageCollectorMXBean collectorMxBean : collectorMxBeans) {
+ collectors.add(new GcCollector(collectorMxBean));
}
- }
- return retVal;
- }
+ List<MemoryPoolMXBean> memoryPoolMXBeans = ManagementFactory.getMemoryPoolMXBeans();
+ for (MemoryPoolMXBean memoryPoolMXBean : memoryPoolMXBeans) {
+ spaceCollectors.add(new GcSpaceCollector(memoryPoolMXBean));
+ }
- @VisibleForTesting
- static String getGcGenerationName(final int genIndex)
- {
- switch (genIndex) {
- case 0:
- return "young";
- case 1:
- return "old";
- case 2:
- // Removed in Java 8 but still actual for previous Java versions
- return "perm";
- default:
- return String.valueOf(genIndex);
}
- }
-
- /**
- * The following GC-related code is partially based on
- * https://github.com/aragozin/jvm-tools/blob/e0e37692648951440aa1a4ea5046261cb360df70/
- * sjk-core/src/main/java/org/gridkit/jvmtool/PerfCounterGcCpuUsageMonitor.java
- */
- private class GcCounters
- {
- private final List<GcGeneration> generations = new ArrayList<>();
- GcCounters()
+ void emit(ServiceEmitter emitter, Map<String, String[]> dimensions)
{
- // connect to itself
- final JStatData jStatData = JStatData.connect(pid);
- final Map<String, JStatData.Counter<?>> jStatCounters = jStatData.getAllCounters();
-
- for (int genIndex : getGcGenerations(jStatCounters.keySet())) {
- generations.add(new GcGeneration(jStatCounters, genIndex, getGcGenerationName(genIndex)));
+ for (GcCollector collector : collectors) {
+ collector.emit(emitter, dimensions);
}
- }
- void emit(ServiceEmitter emitter, Map<String, String[]> dimensions)
- {
- for (GcGeneration generation : generations) {
- generation.emit(emitter, dimensions);
+ for (GcSpaceCollector spaceCollector : spaceCollectors) {
+ spaceCollector.emit(emitter, dimensions);
}
}
}
- private class GcGeneration
+ private class GcCollector
{
- private final String name;
- @Nullable
+ private final String generation;
+ private final String collectorName;
private final GcGenerationCollector collector;
- private final List<GcGenerationSpace> spaces = new ArrayList<>();
- GcGeneration(Map<String, JStatData.Counter<?>> jStatCounters, long genIndex, String name)
+ private static final String GC_YOUNG_GENERATION_NAME = "young";
+ private static final String GC_OLD_GENERATION_NAME = "old";
+ private static final String GC_ZGC_GENERATION_NAME = "zgc";
+
+ private static final String CMS_COLLECTOR_NAME = "cms";
+ private static final String G1_COLLECTOR_NAME = "g1";
+ private static final String PARALLEL_COLLECTOR_NAME = "parallel";
+ private static final String SERIAL_COLLECTOR_NAME = "serial";
+ private static final String ZGC_COLLECTOR_NAME = "zgc";
+ private static final String SHENANDOAN_COLLECTOR_NAME = "shenandoah";
+
+ GcCollector(GarbageCollectorMXBean gcBean)
{
- this.name = StringUtils.toLowerCase(name);
+ Pair<String, String> gcNamePair = getReadableName(gcBean.getName());
+ this.generation = gcNamePair.lhs;
+ this.collectorName = gcNamePair.rhs;
- final String spacesCountKey = StringUtils.format("sun.gc.generation.%d.spaces", genIndex);
+ collector = new GcGenerationCollector(gcBean);
+ }
- if (jStatCounters.containsKey(spacesCountKey)) {
- final long spacesCount = ((JStatData.LongCounter) jStatCounters.get(spacesCountKey)).getLong();
- for (long spaceIndex = 0; spaceIndex < spacesCount; spaceIndex++) {
- spaces.add(new GcGenerationSpace(jStatCounters, genIndex, spaceIndex));
- }
- }
+ private Pair<String, String> getReadableName(String name)
+ {
+ switch (name) {
+ //CMS
+ case "ParNew":
+ return new Pair<>(GC_YOUNG_GENERATION_NAME, CMS_COLLECTOR_NAME);
+ case "ConcurrentMarkSweep":
+ return new Pair<>(GC_OLD_GENERATION_NAME, CMS_COLLECTOR_NAME);
+
+ // G1
+ case "G1 Young Generation":
+ return new Pair<>(GC_YOUNG_GENERATION_NAME, G1_COLLECTOR_NAME);
+ case "G1 Old Generation":
+ return new Pair<>(GC_OLD_GENERATION_NAME, G1_COLLECTOR_NAME);
+
+ // Parallel
+ case "PS Scavenge":
+ return new Pair<>(GC_YOUNG_GENERATION_NAME, PARALLEL_COLLECTOR_NAME);
+ case "PS MarkSweep":
+ return new Pair<>(GC_OLD_GENERATION_NAME, PARALLEL_COLLECTOR_NAME);
+
+ // Serial
+ case "Copy":
+ return new Pair<>(GC_YOUNG_GENERATION_NAME, SERIAL_COLLECTOR_NAME);
+ case "MarkSweepCompact":
+ return new Pair<>(GC_OLD_GENERATION_NAME, SERIAL_COLLECTOR_NAME);
- if (jStatCounters.containsKey(StringUtils.format("sun.gc.collector.%d.name", genIndex))) {
- collector = new GcGenerationCollector(jStatCounters, genIndex);
- } else {
- collector = null;
+ //zgc
+ case "ZGC":
+ return new Pair<>(GC_ZGC_GENERATION_NAME, ZGC_COLLECTOR_NAME);
+
+ //Shenandoah
+ case "Shenandoah Cycles":
+ return new Pair<>(GC_YOUNG_GENERATION_NAME, SHENANDOAN_COLLECTOR_NAME);
+ case "Shenandoah Pauses":
+ return new Pair<>(GC_OLD_GENERATION_NAME, SHENANDOAN_COLLECTOR_NAME);
+
+ default:
+ return new Pair<>(name, name);
}
}
void emit(ServiceEmitter emitter, Map<String, String[]> dimensions)
{
ImmutableMap.Builder<String, String[]> dimensionsCopyBuilder = ImmutableMap
- .<String, String[]>builder()
- .putAll(dimensions)
- .put("gcGen", new String[]{name});
+ .<String, String[]>builder()
+ .putAll(dimensions)
+ .put("gcGen", new String[]{generation});
- if (collector != null) {
- dimensionsCopyBuilder.put("gcName", new String[]{collector.name});
- }
+ dimensionsCopyBuilder.put("gcName", new String[]{collectorName});
Map<String, String[]> dimensionsCopy = dimensionsCopyBuilder.build();
if (collector != null) {
collector.emit(emitter, dimensionsCopy);
}
- for (GcGenerationSpace space : spaces) {
- space.emit(emitter, dimensionsCopy);
- }
}
}
private class GcGenerationCollector
{
- private final String name;
- private final LongCounter invocationsCounter;
- private final TickCounter cpuCounter;
private long lastInvocations = 0;
private long lastCpuNanos = 0;
+ private final GarbageCollectorMXBean gcBean;
- GcGenerationCollector(Map<String, JStatData.Counter<?>> jStatCounters, long genIndex)
+ GcGenerationCollector(GarbageCollectorMXBean gcBean)
{
- String collectorKeyPrefix = StringUtils.format("sun.gc.collector.%d", genIndex);
-
- String nameKey = StringUtils.format("%s.name", collectorKeyPrefix);
- StringCounter nameCounter = (StringCounter) jStatCounters.get(nameKey);
- name = getReadableName(nameCounter.getString());
-
- invocationsCounter = (LongCounter) jStatCounters.get(StringUtils.format("%s.invocations", collectorKeyPrefix));
- cpuCounter = (TickCounter) jStatCounters.get(StringUtils.format("%s.time", collectorKeyPrefix));
+ this.gcBean = gcBean;
}
void emit(ServiceEmitter emitter, Map<String, String[]> dimensions)
{
final ServiceMetricEvent.Builder builder = builder();
MonitorUtils.addDimensionsToBuilder(builder, dimensions);
- long newInvocations = invocationsCounter.getLong();
+ long newInvocations = gcBean.getCollectionCount();
emitter.emit(builder.build("jvm/gc/count", newInvocations - lastInvocations));
lastInvocations = newInvocations;
- long newCpuNanos = cpuCounter.getNanos();
+ long newCpuNanos = gcBean.getCollectionTime();
emitter.emit(builder.build("jvm/gc/cpu", newCpuNanos - lastCpuNanos));
lastCpuNanos = newCpuNanos;
}
+ }
- private String getReadableName(String name)
+ private class GcSpaceCollector
+ {
+
+ private final List<GcGenerationSpace> spaces = new ArrayList<>();
+
+ public GcSpaceCollector(MemoryPoolMXBean memoryPoolMxBean)
{
- switch (name) {
- // Young gen
- case "Copy":
- return "serial";
- case "PSScavenge":
- return "parallel";
- case "PCopy":
- return "cms";
- case "G1 incremental collections":
- return "g1";
- case "Shenandoah partial":
- return "shenandoah";
-
- // Old gen
- case "MCS":
- return "serial";
- case "PSParallelCompact":
- return "parallel";
- case "CMS":
- return "cms";
- case "G1 stop-the-world full collections":
- return "g1";
- case "Shenandoah full":
- return "shenandoah";
+ MemoryUsage collectionUsage = memoryPoolMxBean.getCollectionUsage();
+ if (collectionUsage != null) {
Review Comment:
I think we can get `MemoryUsage` object out of this ctor and check it if is null before constructing this `GcSpaceCollector` object. In this way, if the `collectionUsage` is not null, there is no `GcSpaceCollector` object creation.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 commented on a diff in pull request #12481: Use MXBeans to get GC metrics #12476
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on code in PR #12481:
URL: https://github.com/apache/druid/pull/12481#discussion_r908582005
##########
core/src/main/java/org/apache/druid/java/util/metrics/JvmMonitor.java:
##########
@@ -162,224 +143,184 @@ private void emitDirectMemMetrics(ServiceEmitter emitter)
private void emitGcMetrics(ServiceEmitter emitter)
{
- if (gcCounters != null) {
- gcCounters.emit(emitter, dimensions);
- }
- }
-
- @Nullable
- private GcCounters tryCreateGcCounters()
- {
- try {
- return new GcCounters();
+ if (gcCollectors != null) {
Review Comment:
Is there any chance that `gcCollectors` could be `null`?
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 commented on a diff in pull request #12481: Use MXBeans to get GC metrics #12476
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on code in PR #12481:
URL: https://github.com/apache/druid/pull/12481#discussion_r864474388
##########
core/src/main/java/org/apache/druid/java/util/metrics/JvmMonitor.java:
##########
@@ -167,118 +144,117 @@ private void emitGcMetrics(ServiceEmitter emitter)
}
}
- @Nullable
- private GcCounters tryCreateGcCounters()
- {
- try {
- return new GcCounters();
- }
- catch (RuntimeException e) {
- // in JDK11 jdk.internal.perf.Perf is not accessible, unless
- // --add-exports java.base/jdk.internal.perf=ALL-UNNAMED is set
- log.warn("Cannot initialize GC counters. If running JDK11 and above,"
- + " add --add-exports=java.base/jdk.internal.perf=ALL-UNNAMED"
- + " to the JVM arguments to enable GC counters.");
- }
- return null;
- }
-
- @VisibleForTesting
- static IntSet getGcGenerations(final Set<String> jStatCounterNames)
- {
- final IntSet retVal = new IntRBTreeSet();
-
- for (final String counterName : jStatCounterNames) {
- final Matcher m = PATTERN_GC_GENERATION.matcher(counterName);
- if (m.matches()) {
- retVal.add(Integer.parseInt(m.group(1)));
- }
- }
-
- return retVal;
- }
-
- @VisibleForTesting
- static String getGcGenerationName(final int genIndex)
- {
- switch (genIndex) {
- case 0:
- return "young";
- case 1:
- return "old";
- case 2:
- // Removed in Java 8 but still actual for previous Java versions
- return "perm";
- default:
- return String.valueOf(genIndex);
- }
- }
-
/**
* The following GC-related code is partially based on
* https://github.com/aragozin/jvm-tools/blob/e0e37692648951440aa1a4ea5046261cb360df70/
* sjk-core/src/main/java/org/gridkit/jvmtool/PerfCounterGcCpuUsageMonitor.java
*/
private class GcCounters
{
- private final List<GcGeneration> generations = new ArrayList<>();
+ private final List<GcCollectors> generations = new ArrayList<>();
GcCounters()
{
- // connect to itself
- final JStatData jStatData = JStatData.connect(pid);
- final Map<String, JStatData.Counter<?>> jStatCounters = jStatData.getAllCounters();
-
- for (int genIndex : getGcGenerations(jStatCounters.keySet())) {
- generations.add(new GcGeneration(jStatCounters, genIndex, getGcGenerationName(genIndex)));
+ List<GarbageCollectorMXBean> collectorMxBeans = ManagementFactory.getGarbageCollectorMXBeans();
+ for (GarbageCollectorMXBean collectorMxBean : collectorMxBeans) {
+ generations.add(new GcCollectors(collectorMxBean));
}
+
}
void emit(ServiceEmitter emitter, Map<String, String[]> dimensions)
{
- for (GcGeneration generation : generations) {
+ for (GcCollectors generation : generations) {
generation.emit(emitter, dimensions);
}
}
}
- private class GcGeneration
+ private class GcCollectors
{
- private final String name;
- @Nullable
+ private final String generation;
+ private final String collectorName;
private final GcGenerationCollector collector;
private final List<GcGenerationSpace> spaces = new ArrayList<>();
- GcGeneration(Map<String, JStatData.Counter<?>> jStatCounters, long genIndex, String name)
+ private static final String GC_YOUNG_GENERATION_NAME = "young";
+ private static final String GC_OLD_GENERATION_NAME = "old";
+ private static final String GC_ZGC_GENERATION_NAME = "zgc";
+
+ private static final String GC_CMS_NAME = "cms";
+ private static final String GC_G1_NAME = "g1";
+ private static final String GC_PARALLEL_NAME = "parallel";
+ private static final String GC_SERIAL_NAME = "serial";
+ private static final String GC_ZGC_NAME = "zgc";
+ private static final String GC_SHENANDOAN_NAME = "shenandoah";
+
+ GcCollectors(GarbageCollectorMXBean gcBean)
{
- this.name = StringUtils.toLowerCase(name);
+ Pair<String, String> gcNamePair = getReadableName(gcBean.getName());
+ this.generation = gcNamePair.lhs;
+ this.collectorName = gcNamePair.rhs;
- final String spacesCountKey = StringUtils.format("sun.gc.generation.%d.spaces", genIndex);
+ collector = new GcGenerationCollector(gcBean);
- if (jStatCounters.containsKey(spacesCountKey)) {
- final long spacesCount = ((JStatData.LongCounter) jStatCounters.get(spacesCountKey)).getLong();
- for (long spaceIndex = 0; spaceIndex < spacesCount; spaceIndex++) {
- spaces.add(new GcGenerationSpace(jStatCounters, genIndex, spaceIndex));
+ List<MemoryPoolMXBean> memoryPoolMxBeans = ManagementFactory.getMemoryPoolMXBeans();
Review Comment:
Putting the gc generation collector here will cause metrics for the same generation be emitted for more than 1 times.
For example, If CMS is used, there will be two `GcCollectors` instances, one for the young, one for the old. Obviously, Each `GcCollectors` will hold same `GcGenerationSpace` by the code here.
I think we need to separate the space collector from the current `GcCollectors`. My suggestion is:
1. GcCollector, a collector that collects the gc count and time metrics
2. GcSpaceCollector, a collector that collects gc memory space
3. GcCollectors, a facade class that holds collectors for gc count metrics and space metrics collector
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] liujianhuanzz commented on a diff in pull request #12481: Use MXBeans to get GC metrics #12476
Posted by GitBox <gi...@apache.org>.
liujianhuanzz commented on code in PR #12481:
URL: https://github.com/apache/druid/pull/12481#discussion_r865519914
##########
core/src/main/java/org/apache/druid/java/util/metrics/JvmMonitor.java:
##########
@@ -108,10 +89,6 @@ private void emitThreadAllocationMetrics(ServiceEmitter emitter)
}
}
- /**
- * These metrics are going to be replaced by new jvm/gc/mem/* metrics
- */
- @Deprecated
Review Comment:
This comment was deleted by mistake.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on pull request #12481: Use MXBeans to get GC metrics #12476
Posted by GitBox <gi...@apache.org>.
gianm commented on PR #12481:
URL: https://github.com/apache/druid/pull/12481#issuecomment-1111333925
> @gianm The work in your patch is completed, right? If so I think we can first merge that one.
Yes, it is. I'll merge that, then, and then we can do this patch on top.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 commented on pull request #12481: Use MXBeans to get GC metrics #12476
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on PR #12481:
URL: https://github.com/apache/druid/pull/12481#issuecomment-1168361997
@gianm I have not checked this PR after last review. Let me pick up this PR again. I will try to finish the review in this week.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 commented on pull request #12481: Use MXBeans to get GC metrics #12476
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on PR #12481:
URL: https://github.com/apache/druid/pull/12481#issuecomment-1170105792
@gianm Could you also review this PR when you're at convenience?
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] liujianhuanzz commented on pull request #12481: Use MXBeans to get GC metrics #12476
Posted by GitBox <gi...@apache.org>.
liujianhuanzz commented on PR #12481:
URL: https://github.com/apache/druid/pull/12481#issuecomment-1178616732
>
Thanks for your help. @FrankChen021 @gianm
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org