Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

jvm.gc.live.data.size and max not updating for optavgpause/optthruput collectors #2874

Merged
merged 2 commits into from Nov 26, 2021

Conversation

shakuzen
Copy link
Member

With the OpenJ9 non-generational collectors optavgpause and optthruput, a NPE was happening in the notification listener which caused some code in it to not be executed. Namely, setting the live and max data size was being effectively skipped for these collectors.

Now we check for null and only the allocated bytes counter is skipped when no allocation pool name is set.

@shakuzen shakuzen added bug A general bug module: micrometer-core An issue that is related to our core module labels Nov 26, 2021
@shakuzen shakuzen added this to the 1.7.7 milestone Nov 26, 2021
System.gc();
await().timeout(200, TimeUnit.MILLISECONDS).alias("NotificationListener takes time after GC")
.untilAsserted(() ->
assertThat(registry.find("jvm.gc.live.data.size").gauge().value()).isPositive());
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By forcing a GC and waiting for the notification listener to update metrics, this test does a much better job of ensuring expected metrics are available for the configured collector.

await().timeout(200, TimeUnit.MILLISECONDS).alias("NotificationListener takes time after GC")
.untilAsserted(() ->
assertThat(registry.find("jvm.gc.live.data.size").gauge().value()).isPositive());
assertThat(registry.find("jvm.gc.memory.allocated").counter().count()).isPositive();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This metric is still missing for optavgpause and optthruput due to the allocation pool name not being set. We don't have a CI job run with OpenJ9 because we're still waiting for CI images (possibly CircleCI-Public/cimg-openjdk#78). I think we can fix the missing allocated counter separately from this pull request because it is still an improvement to get rid of the NPE which allows updating of the live and max data size gauges.

With the OpenJ9 non-generational collectors `optavgpause` and `optthruput`, a NPE was happening in the notification listener which caused some code in it to not be executed. Namely, setting the live and max data size was being effectively skipped for these collectors.

Now we check for `null` and only the allocated bytes counter is skipped when no allocation pool name is set.

assumeTrue(binder.isGenerationalGc);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the condition failed, the whole test was marked as skipped, but if we want to use this same test for generational and non-generational collectors, we should run the part of this test before here for both and report it as passed (not skipped) for non-generational collectors if all assertions passed before here.

@shakuzen shakuzen changed the title NPE when allocation pool name not set in JvmGcMetrics jvm.gc.live.data.size and max not updating for optavgpause/optthruput collectors Nov 26, 2021
@shakuzen shakuzen merged commit 022f3af into micrometer-metrics:1.7.x Nov 26, 2021
@shakuzen shakuzen deleted the j9-nongen-gc-fixes branch November 26, 2021 10:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A general bug module: micrometer-core An issue that is related to our core module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant