Jfr profiler support #136

kcrimson · 2023-12-29T13:12:43Z

This change adds support for JFR recordings, it allows to run agents on platforms that don't support async profiler (like Windows).
We have introduced ProfilerDelegate abstraction, allowing us to use different implementations to collect profiler recordings.
This requires running JFR commands with specific settings, which only collect events supported by the JFR parser on the pyroscope server side. This configuration can be found pyroscope.jfc file.
Any changes/suggestions are more than welcomed.

CLAassistant · 2023-12-29T13:12:50Z

All committers have signed the CLA.

korniltsev · 2024-01-02T02:49:49Z

Thanks for the PR. Look forward to merge it. I will take a look soon.
Quick question: Does ingestion to pyroscope work?. Lat time I've checked the previous version of jfr-parser was unable to parse jcmd produced JFR files (grafana/jfr-parser#20 ) . The parser was fully rewritten so it may or may not work, idk, I will do some tests.
Nevermind just read the PR description

korniltsev

Looks good and simple.

I have some minor comments.

I will get a windows VM for manual testing.

It would be nice to have some tests running on windows in github actions. However I realize the project does not have much tests right now so I can not request you to write them. It would be nice to have them though.

korniltsev · 2024-01-02T03:15:36Z

agent/src/main/java/io/pyroscope/javaagent/CurrentPidProvider.java

+import java.lang.reflect.Method;
+
+public class CurrentPidProvider {
+    public static int getCurrentProcessId() {


Is it possible to use long pid = ProcessHandle.current().pid(); on Java>=9 ?

I got this on correto 17.0.9 win x64

java.lang.reflect.InaccessibleObjectException: Unable to make field private final sun.management.VMManagement sun.management.RuntimeImpl.jvm accessible: module java.management does not "opens sun.management" to unnamed module @4bb4de6a at java.base/java.lang.reflect.AccessibleObject.checkCanSetAccessible(AccessibleObject.java:354) at java.base/java.lang.reflect.AccessibleObject.checkCanSetAccessible(AccessibleObject.java:297) at java.base/java.lang.reflect.Field.checkCanSetAccessible(Field.java:178) at java.base/java.lang.reflect.Field.setAccessible(Field.java:172) at io.pyroscope.javaagent.CurrentPidProvider.getCurrentProcessId(CurrentPidProvider.java:17) at io.pyroscope.javaagent.JFRProfilerDelegate.start(JFRProfilerDelegate.java:45) at io.pyroscope.javaagent.impl.ContinuousProfilingScheduler.startFirst(ContinuousProfilingScheduler.java:96) at io.pyroscope.javaagent.impl.ContinuousProfilingScheduler.start(ContinuousProfilingScheduler.java:44) at io.pyroscope.javaagent.PyroscopeAgent.start(PyroscopeAgent.java:51) at App.main(App.java:21) ```

lets add a unit test for this function and run it in gh actions

ProcessHandle.current().pid() is available > Java 9, are ok with bumping the jdk version to 9, or we consider using multi release JAR? with two implementations of pid discovery?

can't we do it at runtime? or maybe also with reflection?
I dont like the idea to have multiple jars.
I dont realy mind how it works, it should just work and currently it throws exception.

multirelease is a single jar, which holds different versions of classes for different JDK versions, https://openjdk.org/jeps/238, so it is single jar :)

multirelease is a single jar, which holds different versions of classes for different JDK versions, https://openjdk.org/jeps/238, so it is single jar :)

Nice. Did not know about it. This should do the trick.

agent/src/main/java/io/pyroscope/javaagent/ProfilerDelegate.java

korniltsev · 2024-01-02T03:19:14Z

agent/src/main/java/io/pyroscope/javaagent/PyroscopeAgent.java

@@ -114,6 +114,9 @@ public Options build() {
                        scheduler = new SamplingProfilingScheduler(config, exporter, logger);
                    }
                }
+                if (profiler == null) {


should we add a setter for profiler field in the builder? Otherwise it looks like profiler is always null here

agent/src/main/java/io/pyroscope/javaagent/JFRProfilerDelegate.java

korniltsev · 2024-01-02T06:39:32Z

I also noticed LabelsWrapper does not work. I think we should make it not crash and maybe print a warning once.

With some little changes (mostly for debugging purposes) to workaround issues I encounted the profiling seem to start.

diff --git a/agent/src/main/java/io/pyroscope/javaagent/CurrentPidProvider.java b/agent/src/main/java/io/pyroscope/javaagent/CurrentPidProvider.java
index 089651e..60ba240 100644
--- a/agent/src/main/java/io/pyroscope/javaagent/CurrentPidProvider.java
+++ b/agent/src/main/java/io/pyroscope/javaagent/CurrentPidProvider.java
@@ -10,19 +10,6 @@ import java.lang.reflect.Method;
 
 public class CurrentPidProvider {
     public static int getCurrentProcessId() {
-        RuntimeMXBean runtime = ManagementFactory.getRuntimeMXBean();
-        Field jvm = null;
-        try {
-            jvm = runtime.getClass().getDeclaredField("jvm");
-            jvm.setAccessible(true);
-
-            VMManagement management = (VMManagement) jvm.get(runtime);
-            Method method = management.getClass().getDeclaredMethod("getProcessId");
-            method.setAccessible(true);
-
-            return (Integer) method.invoke(management);
-        } catch (NoSuchFieldException | InvocationTargetException | IllegalAccessException | NoSuchMethodException e) {
-            throw new RuntimeException(e);
-        }
+        return (int) ProcessHandle.current().pid();
     }
 }
diff --git a/agent/src/main/java/io/pyroscope/javaagent/JFRProfilerDelegate.java b/agent/src/main/java/io/pyroscope/javaagent/JFRProfilerDelegate.java
index aefc735..2557b4d 100644
--- a/agent/src/main/java/io/pyroscope/javaagent/JFRProfilerDelegate.java
+++ b/agent/src/main/java/io/pyroscope/javaagent/JFRProfilerDelegate.java
@@ -30,6 +30,7 @@ public final class JFRProfilerDelegate implements ProfilerDelegate {
             // flight recorder is built on top of a file descriptor, so we need a file.
             tempJFRFile = File.createTempFile("pyroscope", ".jfr");
             tempJFRFile.deleteOnExit();
+            System.out.println(tempJFRFile);
         } catch (IOException e) {
             throw new IllegalStateException(e);
         }
@@ -46,9 +47,11 @@ public final class JFRProfilerDelegate implements ProfilerDelegate {
             commands.add("JFR.start");
             commands.add("name=" + RECORDING_NAME);
             commands.add("filename=" + tempJFRFile.getAbsolutePath());
-            commands.add("settings=pyroscope");
+            commands.add("settings=C:\\Users\\huihu\\pyro\\pyroscope-java\\agent\\build\\resources\\main\\pyroscope.jfc");
             ProcessBuilder processBuilder = new ProcessBuilder(commands);
             Process process = processBuilder.start();
+            System.out.println(new String(process.getInputStream().readAllBytes()));
+            System.out.println(new String(process.getErrorStream().readAllBytes()));
             int exitCode = process.waitFor();
             if (exitCode != 0) {
                 throw new RuntimeException("Invalid exit code: " + exitCode);
@@ -72,6 +75,8 @@ public final class JFRProfilerDelegate implements ProfilerDelegate {
             commands.add("name=" + RECORDING_NAME);
             ProcessBuilder processBuilder = new ProcessBuilder(commands);
             Process process = processBuilder.start();
+            System.out.println(new String(process.getInputStream().readAllBytes()));
+            System.out.println(new String(process.getErrorStream().readAllBytes()));
             int exitCode = process.waitFor();
             if (exitCode != 0) {
                 throw new RuntimeException("Invalid exit code: " + exitCode);
@@ -114,7 +119,7 @@ public final class JFRProfilerDelegate implements ProfilerDelegate {
     private static Path findJcmdBin() {
         Path javaHome = Paths.get(System.getProperty("java.home"));
         //find jcmd binary
-        Path jcmdBin = javaHome.resolve("bin/jcmd");
+        Path jcmdBin = javaHome.resolve("bin/jcmd.exe");
         if (!Files.isExecutable(jcmdBin)) {
             jcmdBin = javaHome.getParent().resolve("bin/jcmd");
             if (!Files.isExecutable(jcmdBin)) {
diff --git a/agent/src/main/java/io/pyroscope/javaagent/impl/PyroscopeExporter.java b/agent/src/main/java/io/pyroscope/javaagent/impl/PyroscopeExporter.java
index 850ab4d..71f9ef6 100644
--- a/agent/src/main/java/io/pyroscope/javaagent/impl/PyroscopeExporter.java
+++ b/agent/src/main/java/io/pyroscope/javaagent/impl/PyroscopeExporter.java
@@ -9,6 +9,7 @@ import io.pyroscope.javaagent.util.zip.GzipSink;
 import io.pyroscope.labels.Pyroscope;
 import okhttp3.*;
 
+import java.io.FileOutputStream;
 import java.io.IOException;
 import java.time.Duration;
 import java.time.Instant;
@@ -43,7 +44,16 @@ public class PyroscopeExporter implements Exporter {
         }
     }
 
+    int counter;
     private void uploadSnapshot(final Snapshot snapshot) throws InterruptedException {
+        String fname = String.format("dump_%d.jfr", counter++);
+        try {
+            FileOutputStream fos = new FileOutputStream(fname);
+            fos.write(snapshot.data);
+            fos.close();
+        } catch (IOException e) {
+            throw new RuntimeException(e);
+        }
         final HttpUrl url = urlForSnapshot(snapshot);
         final ExponentialBackoff exponentialBackoff = new ExponentialBackoff(1_000, 30_000, new Random());
         boolean retry = true;
diff --git a/demo/src/main/java/App.java b/demo/src/main/java/App.java
index cf9480e..2295e34 100644
--- a/demo/src/main/java/App.java
+++ b/demo/src/main/java/App.java
@@ -4,6 +4,7 @@ import io.pyroscope.javaagent.Snapshot;
 import io.pyroscope.javaagent.api.Exporter;
 import io.pyroscope.javaagent.api.Logger;
 import io.pyroscope.javaagent.config.Config;
+import io.pyroscope.javaagent.config.ProfilerType;
 import io.pyroscope.javaagent.impl.DefaultConfigurationProvider;
 import io.pyroscope.labels.Pyroscope;
 import io.pyroscope.labels.LabelsSet;
@@ -20,9 +21,10 @@ public class App {
         PyroscopeAgent.start(
             new PyroscopeAgent.Options.Builder(
                 new Config.Builder()
-                    .setApplicationName("demo.app{qweqwe=asdasd}")
-                    .setServerAddress("http://localhost:4040")
+                    .setApplicationName("windows.app")
+                    .setServerAddress("http://192.168.0.13:4040")
                     .setFormat(Format.JFR)
+                    .setProfilerType(ProfilerType.JFR)
                     .setLogLevel(Logger.Level.DEBUG)
                     .setLabels(mapOf("user", "tolyan"))
                     .build())
@@ -38,7 +40,7 @@ public class App {
         ExecutorService executors = Executors.newFixedThreadPool(N_THREADS);
         for (int i = 0; i < N_THREADS; i++) {
             executors.submit(() -> {
-                Pyroscope.LabelsWrapper.run(new LabelsSet("thread_name", Thread.currentThread().getName()), () -> {
+//                Pyroscope.LabelsWrapper.run(new LabelsSet("thread_name", Thread.currentThread().getName()), () -> {
                         while (true) {
                             try {
                                 fib(32L);
@@ -47,8 +49,8 @@ public class App {
                                 break;
                             }
                         }
-                    }
-                );
+//                    }
+//                );
             });
         }
     }
@@ -68,7 +70,6 @@ public class App {
         if (n == 1L) {
             return 1L;
         }
-        Thread.sleep(100);
         return fib(n - 1) + fib(n - 2);
     }

It started profiling on my VM.

However it failed to ingest

2024-01-01 22:25:01.512 [ERROR] Error uploading snapshot: 422 {"code":"unknown","message":"parsing IngestInput-pprof failed jfr parser ParseEvent error: error reading metadata: unknown string type 4"}

dump_2.jfr.zip

we will need to update jfr-parser I guess

korniltsev · 2024-01-19T05:56:34Z

just FYI I've made some changes to our jfr-parser
grafana/jfr-parser#27
The jfr-parser may now be a little bit closer to be able to parse Flight Recorder's jfr files. Did not test it yet though.

korniltsev · 2024-01-19T08:02:50Z

agent/src/main/java/io/pyroscope/javaagent/JFRProfilerDelegate.java

            ProcessBuilder processBuilder = new ProcessBuilder(commands);
-            Process process = processBuilder.start();
+            Process process = processBuilder.inheritIO().start();


am i understanding correctly that this will redirect the commands stdout to the java process stdout?

If so, I suggest we read it manualy and log it with our logger interface so users can disable it

korniltsev · 2024-01-19T08:03:27Z

please let me know when the PR is ready for second round review. look forward to merge it.

korniltsev · 2024-06-25T07:43:23Z

Hey, thanks for updating. I'll try to look next week.

xhumanoid · 2024-07-09T06:27:43Z

do we really need to execute jcmd command?

did you investigate works with jdk.jfr.Recording; directly?

based on implementation of JFR.start command
https://github.com/openjdk/jdk/blob/master/src/jdk.jfr/share/classes/jdk/jfr/internal/dcmd/DCmdStart.java

they need to access to jdk.jfr.internal, so at least it can require some --add-opens for jvm module
but remove requirements to have jdk (because only have jcmd command, not jre) and proper configured path with shell

kcrimson mentioned this pull request Dec 29, 2023

Support windows as operating system #116

Open

korniltsev requested changes Jan 2, 2024

View reviewed changes

korniltsev reviewed Jan 2, 2024

View reviewed changes

agent/src/main/java/io/pyroscope/javaagent/JFRProfilerDelegate.java Outdated Show resolved Hide resolved

kcrimson force-pushed the jfr-profiler-support branch from 2bf10e0 to 304bd08 Compare January 10, 2024 14:38

korniltsev reviewed Jan 19, 2024

View reviewed changes

kcrimson force-pushed the jfr-profiler-support branch from 42ac936 to 8928419 Compare February 2, 2024 08:01

korniltsev mentioned this pull request Apr 23, 2024

Could we support JFR as a alternative profiling data source for continues profiling #148

Open

kcrimson and others added 8 commits June 24, 2024 09:34

extracted ProfilerDelegate interface

fd1c877

skeleton of JFR profiler

d4fac07

JFR delegate implementation

4af47b3

added JFR configuration which enables only supported JFR event types

05f3853

addressed comments to handle custom JFR settings

8acf3c4

updated README on how to use JFR based agent profiler

d90cb70

proper name for exectuable under windows os

295783a

rebased latest version

2b7069c

kcrimson force-pushed the jfr-profiler-support branch from 8928419 to 2b7069c Compare June 24, 2024 13:58

kcrimson requested a review from a team as a code owner June 24, 2024 13:58

fixed failing JFR event parsing

9521d60

kcrimson added 2 commits July 3, 2024 12:00

printing jcmd command output only on error

eb36c90

added config parameter to overwrite JFR settings

6713966

kcrimson mentioned this pull request Oct 25, 2024

Profiler throws overflow error when running for a long time #166

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Jfr profiler support #136

Jfr profiler support #136

kcrimson commented Dec 29, 2023

CLAassistant commented Dec 29, 2023 •

edited

Loading

korniltsev commented Jan 2, 2024 •

edited

Loading

korniltsev left a comment

korniltsev Jan 2, 2024

korniltsev Jan 2, 2024

korniltsev Jan 2, 2024

kcrimson Jan 8, 2024

korniltsev Jan 9, 2024

kcrimson Jan 10, 2024

korniltsev Jan 11, 2024

korniltsev Jan 2, 2024

korniltsev commented Jan 2, 2024 •

edited

Loading

korniltsev commented Jan 19, 2024

korniltsev Jan 19, 2024

korniltsev commented Jan 19, 2024

korniltsev commented Jun 25, 2024

xhumanoid commented Jul 9, 2024

Jfr profiler support #136

Are you sure you want to change the base?

Jfr profiler support #136

Conversation

kcrimson commented Dec 29, 2023

CLAassistant commented Dec 29, 2023 • edited Loading

korniltsev commented Jan 2, 2024 • edited Loading

korniltsev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

korniltsev commented Jan 2, 2024 • edited Loading

korniltsev commented Jan 19, 2024

Choose a reason for hiding this comment

korniltsev commented Jan 19, 2024

korniltsev commented Jun 25, 2024

xhumanoid commented Jul 9, 2024

CLAassistant commented Dec 29, 2023 •

edited

Loading

korniltsev commented Jan 2, 2024 •

edited

Loading

korniltsev commented Jan 2, 2024 •

edited

Loading