pyroscope-java icon indicating copy to clipboard operation
pyroscope-java copied to clipboard

Jfr profiler support

Open kcrimson opened this issue 1 year ago • 14 comments

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.

kcrimson avatar Dec 29 '23 13:12 kcrimson

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Dec 29 '23 13:12 CLAassistant

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 (https://github.com/grafana/jfr-parser/issues/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 avatar Jan 02 '24 02:01 korniltsev

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 avatar Jan 02 '24 06:01 korniltsev

just FYI I've made some changes to our jfr-parser https://github.com/grafana/jfr-parser/pull/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 avatar Jan 19 '24 05:01 korniltsev

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

korniltsev avatar Jan 19 '24 08:01 korniltsev

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

korniltsev avatar Jun 25 '24 07:06 korniltsev

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

xhumanoid avatar Jul 09 '24 06:07 xhumanoid