fix(jfr): fixes a parser regression introduced in 1.15.0 (#1050)

* chore: add a java-jfr example * use JFR * fixes a bug, adds tests * makes units and AggregationType proper types * moves adhoc-pull to a separate directory so that linters doesn't complain * fix * fixes * fix * fix Co-authored-by: Dmitry Filimonov <[email protected]>
grafana · Apr 24, 2022 · 946468d · 946468d
1 parent 7d9aed5
commit 946468d
Show file tree

Hide file tree

Showing 43 changed files with 401 additions and 106 deletions.
diff --git a/benchmark/internal/loadgen/loadgen.go b/benchmark/internal/loadgen/loadgen.go
@@ -17,6 +17,7 @@ import (
 	"github.com/pyroscope-io/pyroscope/benchmark/internal/server"
 	"github.com/pyroscope-io/pyroscope/pkg/agent/upstream"
 	"github.com/pyroscope-io/pyroscope/pkg/agent/upstream/remote"
+	"github.com/pyroscope-io/pyroscope/pkg/storage/metadata"
 	"github.com/pyroscope-io/pyroscope/pkg/structs/transporttrie"
 	"github.com/sirupsen/logrus"
 )
@@ -262,8 +263,8 @@ Outside:
 			EndTime:         et,
 			SpyName:         "gospy",
 			SampleRate:      100,
-			Units:           "samples",
-			AggregationType: "sum",
+			Units:           metadata.SamplesUnits,
+			AggregationType: metadata.SumAggregationType,
 			Trie:            t,
 		})
 		l.pauseMutex.RUnlock()

diff --git a/examples/adhoc/adhoc-pull.go → examples/adhoc/pull/adhoc-pull.go b/examples/adhoc/adhoc-pull.go → examples/adhoc/pull/adhoc-pull.go
diff --git a/examples/java-jfr/Dockerfile b/examples/java-jfr/Dockerfile
@@ -0,0 +1,26 @@
+FROM openjdk:17-slim-bullseye
+
+WORKDIR /opt/app
+
+RUN apt-get update && apt-get install ca-certificates -y && update-ca-certificates && apt-get install -y git
+RUN git clone https://github.com/pyroscope-io/pyroscope-java.git && \
+    cd pyroscope-java && \
+    git checkout v0.6.0 && \
+    ./gradlew shadowJar && \
+    cp agent/build/libs/pyroscope.jar /opt/app/pyroscope.jar
+
+COPY Main.java ./
+
+RUN javac Main.java
+
+ENV PYROSCOPE_APPLICATION_NAME=fibonacci.java.push.app
+ENV PYROSCOPE_FORMAT=jfr
+ENV PYROSCOPE_PROFILING_INTERVAL=10ms
+ENV PYROSCOPE_PROFILER_EVENT=cpu
+ENV PYROSCOPE_PROFILER_LOCK=1
+ENV PYROSCOPE_PROFILER_ALLOC=1
+ENV PYROSCOPE_UPLOAD_INTERVAL=10s
+ENV PYROSCOPE_LOG_LEVEL=debug
+ENV PYROSCOPE_SERVER_ADDRESS=http://localhost:4040
+
+CMD ["java", "-XX:-Inline", "-javaagent:pyroscope.jar", "Main"]
diff --git a/examples/java-jfr/Main.java b/examples/java-jfr/Main.java
@@ -0,0 +1,51 @@
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantLock;
+
+class MyRunnable implements Runnable {
+    private Lock lock;
+
+    public MyRunnable(Lock lock) {
+        this.lock = lock;
+    }
+
+    public static long fib(long n) {
+        if (n < 2)
+            return n;
+        return fib(n-1) + fib(n-2);
+    }
+
+    public void run() {
+        while (true) {
+            this.lock.lock();
+            try {
+                fib(40);
+            } finally {
+                this.lock.unlock();
+            }
+        }
+    }
+}
+
+class Main {
+    public static long fib(long n) {
+        if (n < 2)
+            return n;
+        return fib(n-1) + fib(n-2);
+    }
+
+    public static void main(String[] args) {
+        Lock l = new ReentrantLock();
+        Runnable r = new MyRunnable(l);
+        new Thread(r).start();
+
+        while (true) {
+            l.lock();
+            try {
+                fib(40);
+            } finally {
+                l.unlock();
+            }
+        }
+    }
+}
+
diff --git a/examples/java-jfr/docker-compose.yml b/examples/java-jfr/docker-compose.yml
@@ -0,0 +1,19 @@
+---
+version: '3.9'
+services:
+  pyroscope:
+    image: 'pyroscope/pyroscope:latest'
+    ports:
+      - '4040:4040'
+    command:
+      - 'server'
+      - '-no-self-profiling'
+  app:
+    build: .
+    privileged: true
+    environment:
+      - 'PYROSCOPE_APPLICATION_NAME=fibonacci-java-lock-push'
+      - 'PYROSCOPE_PROFILER_EVENT=lock'
+      - 'PYROSCOPE_SERVER_ADDRESS=http://pyroscope:4040'
+      - 'PYROSCOPE_FORMAT=jfr'
+      - 'PYROSCOPE_PROFILER_LOCK=0'
diff --git a/examples/java/Dockerfile b/examples/java/Dockerfile
@@ -1,12 +1,14 @@
 FROM openjdk:11.0.11-jdk
 
 WORKDIR /opt/app
-RUN git clone https://github.com/pyroscope-io/pyroscope-java.git
-RUN cd pyroscope-java && \
+RUN git clone https://github.com/pyroscope-io/pyroscope-java.git && \
+    cd pyroscope-java && \
+    git checkout v0.6.0 && \
     ./gradlew shadowJar && \
     cp agent/build/libs/pyroscope.jar /opt/app/pyroscope.jar
 
 COPY Main.java ./Main.java
+RUN javac Main.java
 
 ENV PYROSCOPE_APPLICATION_NAME=simple.java.app
 ENV PYROSCOPE_PROFILING_INTERVAL=10ms
@@ -15,6 +17,4 @@ ENV PYROSCOPE_UPLOAD_INTERVAL=10s
 ENV PYROSCOPE_LOG_LEVEL=debug
 ENV PYROSCOPE_SERVER_ADDRESS=http://pyroscope:4040
 
-RUN javac Main.java
-
 CMD ["java", "-XX:-Inline", "-javaagent:pyroscope.jar", "Main"]
diff --git a/go.mod b/go.mod
@@ -47,7 +47,7 @@ require (
 	github.com/pyroscope-io/client v0.0.0-20211206204731-3fd0a4b8239c
 	github.com/pyroscope-io/dotnetdiag v1.2.1
 	github.com/pyroscope-io/goldga v0.4.2-0.20220218190441-817afcc3a7f1
-	github.com/pyroscope-io/jfr-parser v0.4.0
+	github.com/pyroscope-io/jfr-parser v0.4.1
 	github.com/rlmcpherson/s3gof3r v0.5.0
 	github.com/shirou/gopsutil v3.21.4+incompatible
 	github.com/sirupsen/logrus v1.8.1

diff --git a/go.sum b/go.sum
@@ -618,8 +618,8 @@ github.com/pyroscope-io/dotnetdiag v1.2.1 h1:3XEMrfFJnZ87BiEhozyQKmCUAuMd/Spq7KC
 github.com/pyroscope-io/dotnetdiag v1.2.1/go.mod h1:eFUEHCp4eD1TgcXMlJihC+R4MrqGf7nTRdWxNADbDHA=
 github.com/pyroscope-io/goldga v0.4.2-0.20220218190441-817afcc3a7f1 h1:T1fDdt3E3UpaGZ7tRF2IYrUFwNyuPlIWGeCOjfINp1s=
 github.com/pyroscope-io/goldga v0.4.2-0.20220218190441-817afcc3a7f1/go.mod h1:PbX5bxlj/WxyKIEAxYgNMNWUpXP4rt9GqtjfvTf8m+I=
-github.com/pyroscope-io/jfr-parser v0.4.0 h1:K5daiLhW4XP2miyCAlw67hqpd7WSo4ROVc/DXSfpECk=
-github.com/pyroscope-io/jfr-parser v0.4.0/go.mod h1:ZMcbJjfDkOwElEK8CvUJbpetztRWRXszCmf5WU0erV8=
+github.com/pyroscope-io/jfr-parser v0.4.1 h1:lYFQHIQC3YkjlQhqf5fsObyW7sxAjc6NsFodOMNg9js=
+github.com/pyroscope-io/jfr-parser v0.4.1/go.mod h1:ZMcbJjfDkOwElEK8CvUJbpetztRWRXszCmf5WU0erV8=
 github.com/pyroscope-io/revive v1.0.6-0.20210330033039-4a71146f9dc1 h1:0v9lBNgdmVtpyyk9PP/DfpJlOHkXriu5YgNlrhQw5YE=
 github.com/pyroscope-io/revive v1.0.6-0.20210330033039-4a71146f9dc1/go.mod h1:tSw34BaGZ0iF+oVKDOjq1/LuxGifgW7shaJ6+dBYFXg=
 github.com/rivo/uniseg v0.1.0/go.mod h1:J6wj4VEh+S6ZtnVlnTBMWIodfgj8LQOQFoIToxlJtxc=

diff --git a/packages/pyroscope-flamegraph/src/FlameGraph/FlameGraphComponent/Header.tsx b/packages/pyroscope-flamegraph/src/FlameGraph/FlameGraphComponent/Header.tsx
@@ -20,9 +20,11 @@ export default function Header(props: HeaderProps) {
   const { format, units, ExportData = <></>, palette, setPalette } = props;
 
   const unitsToFlamegraphTitle = {
-    objects: 'amount of objects in RAM per function',
+    objects: 'number of objects in RAM per function',
     bytes: 'amount of RAM per function',
     samples: 'CPU time per function',
+    lock_nanoseconds: 'time spent waiting on locks per function',
+    lock_samples: 'number of contended locks per function',
     '': '',
   };
 

diff --git a/packages/pyroscope-flamegraph/src/format/format.ts b/packages/pyroscope-flamegraph/src/format/format.ts
@@ -22,6 +22,10 @@ export function getFormatter(max: number, sampleRate: number, unit: Units) {
       return new ObjectsFormatter(max);
     case 'bytes':
       return new BytesFormatter(max);
+    case 'lock_nanoseconds':
+      return new NanosecondsFormatter(max);
+    case 'lock_samples':
+      return new ObjectsFormatter(max);
     default:
       console.warn(`Unsupported unit: '${unit}'. Defaulting to 'samples'`);
       return new DurationFormatter(max / sampleRate);
@@ -77,6 +81,57 @@ class DurationFormatter {
   }
 }
 
+// this is a class and not a function because we can save some time by
+//   precalculating divider and suffix and not doing it on each iteration
+class NanosecondsFormatter {
+  divider = 1;
+  multiplier = 1;
+
+  suffix: string = 'second';
+
+  durations: [number, string][] = [
+    [60, 'minute'],
+    [60, 'hour'],
+    [24, 'day'],
+    [30, 'month'],
+    [12, 'year'],
+  ];
+
+  constructor(maxDur: number) {
+    maxDur /= 1000000000;
+    // eslint-disable-next-line no-plusplus
+    for (let i = 0; i < this.durations.length; i++) {
+      const level = this.durations[i];
+      if (!level) {
+        console.warn('Could not calculate level');
+        break;
+      }
+
+      if (maxDur >= level[0]) {
+        this.divider *= level[0];
+        maxDur /= level[0];
+        // eslint-disable-next-line prefer-destructuring
+        this.suffix = level[1];
+      } else {
+        break;
+      }
+    }
+  }
+
+  format(samples: number, sampleRate: number) {
+    const n = samples / 1000000000 / this.divider;
+    let nStr = n.toFixed(2);
+
+    if (n >= 0 && n < 0.01) {
+      nStr = '< 0.01';
+    } else if (n <= 0 && n > -0.01) {
+      nStr = '< 0.01';
+    }
+
+    return `${nStr} ${this.suffix}${n === 1 ? '' : 's'}`;
+  }
+}
+
 export class ObjectsFormatter {
   divider = 1;
 

diff --git a/packages/pyroscope-models/src/flamebearer.ts b/packages/pyroscope-models/src/flamebearer.ts
@@ -20,7 +20,13 @@ export type Flamebearer = {
    * Sample Rate, used in text information
    */
   sampleRate: number;
-  units: 'samples' | 'objects' | 'bytes' | '';
+  units:
+    | 'samples'
+    | 'objects'
+    | 'bytes'
+    | 'lock_samples'
+    | 'lock_nanoseconds'
+    | '';
   spyName:
     | 'dotneyspy'
     | 'ebpfspy'

diff --git a/packages/pyroscope-models/src/profile.ts b/packages/pyroscope-models/src/profile.ts
@@ -10,14 +10,20 @@ export const FlamebearerSchema = z.object({
 // accept the defined units
 // and convert anything else to empty string
 const UnitsSchema = z.preprocess((u) => {
-  const units = ['samples', 'objects', 'bytes'];
+  const units = [
+    'samples',
+    'objects',
+    'bytes',
+    'lock_samples',
+    'lock_nanoseconds',
+  ];
   if (typeof u === 'string') {
     if (units.includes(u)) {
       return u;
     }
   }
   return '';
-}, z.enum(['samples', 'objects', 'bytes', '']));
+}, z.enum(['samples', 'objects', 'bytes', 'lock_samples', 'lock_nanoseconds', '']));
 
 export const MetadataSchema = z.object({
   // Optional fields since adhoc may be missing them

diff --git a/pkg/adhoc/external_writer.go b/pkg/adhoc/external_writer.go
@@ -80,7 +80,9 @@ func (w *externalWriter) write(name string, out *storage.GetOutput) error {
 	switch w.format {
 	case "pprof":
 		pprof := out.Tree.Pprof(&tree.PprofMetadata{
-			Unit:      out.Units,
+			// TODO(petethepig): check if this conversion always makes sense
+			//   e.g are these units defined in pprof somewhere?
+			Unit:      string(out.Units),
 			StartTime: w.now,
 		})
 		out, err := proto.Marshal(pprof)

diff --git a/pkg/adhoc/server/convert.go b/pkg/adhoc/server/convert.go
@@ -12,6 +12,7 @@ import (
 	"github.com/pyroscope-io/pyroscope/pkg/agent/spy"
 	"github.com/pyroscope-io/pyroscope/pkg/convert"
 	"github.com/pyroscope-io/pyroscope/pkg/storage"
+	"github.com/pyroscope-io/pyroscope/pkg/storage/metadata"
 	"github.com/pyroscope-io/pyroscope/pkg/storage/tree"
 	"github.com/pyroscope-io/pyroscope/pkg/structs/flamebearer"
 )
@@ -38,7 +39,7 @@ func PprofToProfileV1(b []byte, name string, maxNodes int) (*flamebearer.Flamebe
 	// TODO(abeaumont): Support multiple sample types
 	for _, stype := range p.SampleTypes() {
 		sampleRate := uint32(100)
-		units := "samples"
+		units := metadata.SamplesUnits
 		if c, ok := tree.DefaultSampleTypeMapping[stype]; ok {
 			units = c.Units
 			if c.Sampled && p.Period > 0 {

diff --git a/pkg/agent/spy/spy.go b/pkg/agent/spy/spy.go
@@ -3,6 +3,8 @@ package spy
 
 import (
 	"fmt"
+
+	"github.com/pyroscope-io/pyroscope/pkg/storage/metadata"
 )
 
 type Spy interface {
@@ -32,23 +34,23 @@ func (t ProfileType) IsCumulative() bool {
 	return t == ProfileAllocObjects || t == ProfileAllocSpace
 }
 
-func (t ProfileType) Units() string {
+func (t ProfileType) Units() metadata.Units {
 	if t == ProfileInuseObjects || t == ProfileAllocObjects {
-		return "objects"
+		return metadata.ObjectsUnits
 	}
 	if t == ProfileInuseSpace || t == ProfileAllocSpace {
-		return "bytes"
+		return metadata.BytesUnits
 	}
 
-	return "samples"
+	return metadata.SamplesUnits
 }
 
-func (t ProfileType) AggregationType() string {
+func (t ProfileType) AggregationType() metadata.AggregationType {
 	if t == ProfileInuseObjects || t == ProfileInuseSpace {
-		return "average"
+		return metadata.AverageAggregationType
 	}
 
-	return "sum"
+	return metadata.SumAggregationType
 }
 
 // TODO: this interface is not the best as different spies have different arguments

diff --git a/pkg/agent/target/target.go b/pkg/agent/target/target.go
@@ -73,7 +73,7 @@ func (mgr *Manager) canonise(t *config.Target) error {
 		t.SampleRate = types.DefaultSampleRate
 	}
 	if t.ApplicationName == "" {
-		t.ApplicationName = t.SpyName + "." + names.GetRandomName(generateSeed(t.ServiceName, t.SpyName))
+		t.ApplicationName = string(t.SpyName) + "." + names.GetRandomName(generateSeed(t.ServiceName, t.SpyName))
 		logger := mgr.logger.WithField("spy-name", t.SpyName)
 		if t.ServiceName != "" {
 			logger = logger.WithField("service-name", t.ServiceName)

diff --git a/pkg/agent/upstream/remote/remote.go b/pkg/agent/upstream/remote/remote.go
@@ -110,8 +110,8 @@ func (r *Remote) uploadProfile(j *upstream.UploadJob) error {
 	q.Set("until", strconv.Itoa(int(j.EndTime.Unix())))
 	q.Set("spyName", j.SpyName)
 	q.Set("sampleRate", strconv.Itoa(int(j.SampleRate)))
-	q.Set("units", j.Units)
-	q.Set("aggregationType", j.AggregationType)
+	q.Set("units", string(j.Units))
+	q.Set("aggregationType", string(j.AggregationType))
 
 	u.Path = path.Join(u.Path, "/ingest")
 	u.RawQuery = q.Encode()

diff --git a/pkg/agent/upstream/remote/remote_test.go b/pkg/agent/upstream/remote/remote_test.go
@@ -12,6 +12,7 @@ import (
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
 	"github.com/pyroscope-io/pyroscope/pkg/agent/upstream"
+	"github.com/pyroscope-io/pyroscope/pkg/storage/metadata"
 	"github.com/pyroscope-io/pyroscope/pkg/structs/transporttrie"
 	"github.com/pyroscope-io/pyroscope/pkg/testing"
 	"github.com/sirupsen/logrus"
@@ -61,7 +62,7 @@ var _ = Describe("remote.Remote", func() {
 						EndTime:    testing.SimpleTime(10),
 						SpyName:    "debugspy",
 						SampleRate: 100,
-						Units:      "samples",
+						Units:      metadata.SamplesUnits,
 						Trie:       t,
 					})
 				}