-
Notifications
You must be signed in to change notification settings - Fork 170
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
Add Java Flight Recorder support #1154
Conversation
That is fine for testing, but we do not want users to have to deal with this. They shouldn't need to care how we get the data and I don't want to create a separate copy that could start getting some use and require migration efforts. |
spectator-ext-jvm/src/main/java17/com/netflix/spectator/jvm/JavaFlightRecorder.java
Outdated
Show resolved
Hide resolved
spectator-ext-jvm/src/main/java17/com/netflix/spectator/jvm/JavaFlightRecorder.java
Outdated
Show resolved
Hide resolved
spectator-ext-jvm/src/main/java17/com/netflix/spectator/jvm/JavaFlightRecorder.java
Outdated
Show resolved
Hide resolved
spectator-ext-jvm/src/main/java17/com/netflix/spectator/jvm/JavaFlightRecorder.java
Outdated
Show resolved
Hide resolved
spectator-ext-jvm/src/main/java17/com/netflix/spectator/jvm/JavaFlightRecorder.java
Show resolved
Hide resolved
spectator-ext-jvm/src/test/java17/com/netflix/spectator/jvm/JavaFlightRecorderTest.java
Outdated
Show resolved
Hide resolved
spectator-ext-jvm/src/main/java17/com/netflix/spectator/jvm/JavaFlightRecorder.java
Outdated
Show resolved
Hide resolved
Oh yeah, totally the intention that's temporary and then rename them to |
f060c09
to
f670643
Compare
In fact, looks like getting parity with |
f670643
to
ae60d90
Compare
Interesting, do you know if that is something that the JDK team is looking at? What are the specific gaps? I think jdk.GarbageCollection would cover the main pause times. Concurrent phase time and before and after memory pool sizes is a bit less clear from my quick skimming. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since Jmx.registerStandardMXBeans
is the main entry point used by various integrations, I think we can have it choose JFR internally when available. This would allow it to automatically transition without having to update all of different uses. Might need to have some flag to force the old behavior for testing, I'll think about that.
The GcLogger is a bit trickier, but since that isn't in scope for now can worry about that later.
spectator-ext-jvm/src/main/java17/com/netflix/spectator/jvm/JavaFlightRecorder.java
Show resolved
Hide resolved
spectator-ext-jvm/src/main/java17/com/netflix/spectator/jvm/JavaFlightRecorder.java
Show resolved
Hide resolved
spectator-ext-jvm/src/main/java17/com/netflix/spectator/jvm/JavaFlightRecorder.java
Show resolved
Hide resolved
spectator-ext-jvm/src/main/java17/com/netflix/spectator/jvm/JavaFlightRecorder.java
Outdated
Show resolved
Hide resolved
spectator-ext-jvm/src/main/java17/com/netflix/spectator/jvm/JavaFlightRecorder.java
Outdated
Show resolved
Hide resolved
spectator-ext-jvm/src/test/java17/com/netflix/spectator/jvm/JavaFlightRecorderTest.java
Show resolved
Hide resolved
I'll put a ticket in my backlog to follow up with them on lack of support for these events in the other collectors. (Edit: JVM-335) |
8b0774b
to
6f70c0a
Compare
6f70c0a
to
5b74da6
Compare
add comments
spectator-ext-jvm/src/main/java/com/netflix/spectator/jvm/Jmx.java
Outdated
Show resolved
Hide resolved
spectator-ext-jvm/src/main/java17/com/netflix/spectator/jvm/JavaFlightRecorder.java
Outdated
Show resolved
Hide resolved
make indentation consistent
Hold reference to thread count gauges
This adds Java Flight Recorder support using JFR Event Streaming to begin adding measures that are not available via other means. For starters this includes per-process CPU load, Virtual Threads and ZGC.
Event Streaming was added in JDK 14, so I've anchored this to the nearest LTS using a Multi-Release jar so the classes are only visible on supported releases.
I need to dig deeper, but at a glance all of the current
registerStandardMXBeans
andGcLogger
measures could be handled by JFR events instead. Unless there are any concerns, I'll follow this up with replacements for all of these standard measures with anjfr
prefix so we can confirm parity with thejvm
metrics in the real world.