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

Adding the JFR metrics gatherer to instrumentation/runtime-metrics directory #7851

Closed
roberttoyonaga opened this issue Feb 17, 2023 · 6 comments · Fixed by #8715
Closed

Adding the JFR metrics gatherer to instrumentation/runtime-metrics directory #7851

roberttoyonaga opened this issue Feb 17, 2023 · 6 comments · Fixed by #8715

Comments

@roberttoyonaga
Copy link
Contributor

Should the JFR runtime metrics gatherer be added to the instrumentation/runtime-metrics directory? This topic was discussed in the 2023/02/16 instrumentation SIG. The reason this topic was brought up is because it doesn't seem like the Hotspot JFR team will accept new JFR events to fill in the gaps JFR needs filled to meet the existing semantic conventions. This issue is meant to continue the discussion and flesh out this idea a bit more.

Why do this?
The current metrics gathering implementation uses JMX as the data source. JFR provides data that is not available through JMX (locks, waiting/parked threads etc) and it is probable that new runtime metric info will be added to the JFR system over JMX. This additional JVM info can expand the semantic conventions with helpful metrics.

Details
One possible outcome of this proposed change is that both gatherers can be used in combination. The JMX implementation can be used to gather the set of metrics currently outlined in the semantic conventions and the JFR implementation can supplement with the new metrics that should be proposed additions to the semantic conventions.

Considerations
The JFR gatherer uses "JFR event streaming" which was introduced in JDK 14. Is it possible to restrict usage to JDK14+ runtimes? The semantic conventions could specify which JDK versions particular metrics correspond to.

Alternatives
Keep the JFR implementation in the contrib repo. Additionally, we can fill in the gaps using JMX internally as the data source for missing pieces. This would allow the JFR implementation to fulfill the semantic conventions. I'm not fully convinced this is worth doing (it would have been nicer to have a purely JFR implementation), but afterwards the implementation can be expanded to gather metrics not yet defined in the semantic conventions.

@roberttoyonaga roberttoyonaga changed the title Adding JFR metrics gatherer to instrumentation/runtime-metrics directory Adding the JFR metrics gatherer to instrumentation/runtime-metrics directory Feb 17, 2023
@trask
Copy link
Member

trask commented Feb 17, 2023

The JFR gatherer uses "JFR event streaming" which was introduced in JDK 14.

Maybe we should have runtime-metrics-jmx and runtime-metrics-jfr modules to make it easier to target different JDK versions?

(we'd probably go ahead and target JDK 17+ instead of JDK 14+ since we only test on LTS and latest releases)

@roberttoyonaga
Copy link
Contributor Author

Maybe we should have runtime-metrics-jmx and runtime-metrics-jfr modules to make it easier to target different JDK versions?

Ok yes I think that’s a good idea

@trask
Copy link
Member

trask commented Feb 22, 2023

from discussion in open-telemetry/opentelemetry-java-contrib#749 I think we arrived at runtime-telemetry-jmx and runtime-telemetry-jfr for module names

@roberttoyonaga
Copy link
Contributor Author

from discussion in open-telemetry/opentelemetry-java-contrib#749 I think we arrived at runtime-telemetry-jmx and runtime-telemetry-jfr for module names

Ok I've made a draft PR using the new names here: #7886

@mateuszrzeszutek
Copy link
Member

I think we've lost the -telemetry naming at some point. @open-telemetry/java-instrumentation-maintainers should we make the switch and rename the runtime metrics modules to runtime-telemetry-java(8|17)? (I think we should)

@trask
Copy link
Member

trask commented May 31, 2023

👍

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 a pull request may close this issue.

3 participants