Skip to content

Add metric jvm.file_descriptor.count to jvm runtime experimental metrics #13904

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

Conversation

schmikei
Copy link
Contributor

Apologies for the delay, was battling some larger issues, but I believe that this should add the new experimental metric introduced in #13722

Resolves #13757

@schmikei schmikei changed the title draft: add experimental metric to jvm runtime metrics Add experimental metric to jvm runtime metrics May 22, 2025
@schmikei schmikei changed the title Add experimental metric to jvm runtime metrics Add metric jvm.file_descriptor.count to jvm runtime experimental metrics May 22, 2025
@otelbot-java-instrumentation
Copy link
Contributor

🔧 The result from spotlessApply was committed to the PR branch.

schmikei added 2 commits May 22, 2025 14:08
…n' of github.com:schmikei/opentelemetry-java-instrumentation into feat/add-jvm-file-descriptors-to-runtime-instrumentation
@schmikei schmikei marked this pull request as ready for review May 22, 2025 18:46
@schmikei schmikei requested a review from a team as a code owner May 22, 2025 18:46
@SylvainJuge
Copy link
Contributor

Can you also add this new metric to jvm.yaml and jvm.md in the instrumentation/jmx-metrics so that metric would also be supported when captured through external JMX access ?

@laurit
Copy link
Contributor

laurit commented May 23, 2025

Can you also add this new metric to jvm.yaml and jvm.md in the instrumentation/jmx-metrics so that metric would also be supported when captured through external JMX access ?

It was already added there in #13722

@laurit laurit added the test openj9 This label can be applied to PRs to trigger them to run openj9 tests label May 23, 2025
@SylvainJuge
Copy link
Contributor

#13913 has been merged, you should be able to update it from main to fix the latestDeps* test failures.

Meter meter = JmxRuntimeMetricsUtil.getMeter(openTelemetry);
List<AutoCloseable> observables = new ArrayList<>();

if (osBean instanceof com.sun.management.UnixOperatingSystemMXBean) {
Copy link
Contributor

Choose a reason for hiding this comment

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

what I'm a bit confused about is that in

Supplier<Long> processCpuTimeSupplier =
methodInvoker(osBean, OS_BEAN_HOTSPOT, METHOD_PROCESS_CPU_TIME, Long.class);
if (processCpuTimeSupplier == null) {
// More users will be on hotspot than j9, so check for j9 second
processCpuTimeSupplier =
methodInvoker(osBean, OS_BEAN_J9, METHOD_PROCESS_CPU_TIME, Long.class);
}
uses reflection and falls back to ibm implementation. Current version of the ibm interface extends the sun one https://github.com/eclipse-openj9/openj9/blob/aecc0d86c6d914747dffd4c34dff8672a804e2b9/jcl/src/jdk.management/share/classes/com/ibm/lang/management/OperatingSystemMXBean.java#L82 Idk if this has always been so or this code will fail on early ibm java 8 because com.sun.management.UnixOperatingSystemMXBean is not available. If it has always been so why does CpuMethods use reflection at all?

Copy link
Contributor

Choose a reason for hiding this comment

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

Googling for NoClassDefFoundError UnixOperatingSystemMXBean indicates that even if this class is present in the jdk there could still be a NoClassDefFoundError when the application server does not delegate loading all classes to boot or system loader. This is so for example on wildfly. So I think it could be beneficial to use Class.forName to check for the presence of com.sun.management.UnixOperatingSystemMXBean

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh I was meaning to look into this further cross JDKs today. Thank you so much for making that change 🚀

@laurit laurit added the test windows This label can be applied to PRs to trigger them to run windows smoke tests label May 26, 2025
@laurit laurit removed test windows This label can be applied to PRs to trigger them to run windows smoke tests test openj9 This label can be applied to PRs to trigger them to run openj9 tests labels May 27, 2025
@trask trask merged commit 8604da3 into open-telemetry:main May 27, 2025
135 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add jvm.file_descriptor.count metric to the instrumentation/runtime-telemetry
4 participants