Prototype sending allocation stacks as pprof #684

laurit · 2022-02-23T14:52:44Z

@gsmirnov-splk @vovencij please review

profiler/src/main/java/com/splunk/opentelemetry/profiler/JfrActivator.java

...main/java/com/splunk/opentelemetry/profiler/allocation/exporter/AllocationEventExporter.java

.../com/splunk/opentelemetry/profiler/allocation/exporter/PlainTextAllocationEventExporter.java

...java/com/splunk/opentelemetry/profiler/allocation/exporter/PprofAllocationEventExporter.java

profiler/src/main/proto/profile.proto

...java/com/splunk/opentelemetry/profiler/allocation/exporter/PprofAllocationEventExporter.java

profiler/src/main/java/com/splunk/opentelemetry/profiler/Configuration.java

profiler/src/main/java/com/splunk/opentelemetry/profiler/JfrActivator.java

...java/com/splunk/opentelemetry/profiler/allocation/exporter/PprofAllocationEventExporter.java

...er/src/main/java/com/splunk/opentelemetry/profiler/exporter/PprofProfilingEventExporter.java

gsmirnov-splk · 2022-03-23T18:11:41Z

...er/src/main/java/com/splunk/opentelemetry/profiler/exporter/PprofProfilingEventExporter.java

+    pprof.getProfileBuilder().addSample(sample);
+  }
+
+  private static StackTrace handleStackTrace(String stackTrace) {


Hey @vovencij please 👀

...er/src/main/java/com/splunk/opentelemetry/profiler/exporter/PprofProfilingEventExporter.java

...java/com/splunk/opentelemetry/profiler/allocation/exporter/PprofAllocationEventExporter.java

profiler/src/main/java/com/splunk/opentelemetry/profiler/ProfilingDataType.java

profiler/src/main/java/com/splunk/opentelemetry/profiler/Configuration.java

...java/com/splunk/opentelemetry/profiler/allocation/exporter/PprofAllocationEventExporter.java

profiler/src/main/java/com/splunk/opentelemetry/profiler/Configuration.java

mateuszrzeszutek · 2022-03-31T13:58:37Z

profiler/src/main/java/com/splunk/opentelemetry/profiler/JfrActivator.java

+    private static LogProcessor INSTANCE;
+
+    static LogProcessor get(LogExporter logsExporter) {
+      if (INSTANCE == null) {


Nit: you can make it final and initialize it in a static block, there's no need for the null-check here

The idea behind this is to initialize BatchingLogsProcessor only if it is needed. And if it is needed only create one copy of it. BatchingLogsProcessor is not used by the pprof exporters. I'll add a comment.

mateuszrzeszutek · 2022-03-31T14:04:18Z

profiler/src/main/java/com/splunk/opentelemetry/profiler/exporter/PprofCpuEventExporter.java

+    this.eventPeriods = builder.eventPeriods;
+    this.pprofLogDataExporter =
+        new PprofLogDataExporter(
+            builder.logProcessor, builder.resource, ProfilingDataType.CPU, builder.dataFormat);


The only possible value for dataFormat is PPROF_GZIP_BASE64 - does it make sense to pass it?

This is because initially we had more formats. I'd keep it in case we need to experiment with different format, it would be nice if we could get rid of the base64.

profiler/src/main/java/com/splunk/opentelemetry/profiler/exporter/PprofLogDataExporter.java

mateuszrzeszutek · 2022-03-31T14:22:38Z

profiler/src/main/java/com/splunk/opentelemetry/profiler/exporter/StackTraceParser.java

+
+  public static StackTrace parse(String stackTrace) {
+    // \\R - Any Unicode linebreak sequence
+    String[] lines = stackTrace.split("\\R");


Here's an idea for improvement (not this PR though): we could parse the stack trace without this split, just using the start/end indexes, and save some memory

profiler/src/main/java/com/splunk/opentelemetry/profiler/exporter/StackTraceParser.java

mateuszrzeszutek · 2022-03-31T14:47:40Z

profiler/src/test/java/com/splunk/opentelemetry/profiler/exporter/StackTraceParserTest.java

+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+
+class StackTraceParserTest {


I think this class would benefit from more extensive unit test - since it contains a ton of parsing logic. Currently there are no assertions for stack trace lines, and most of the other properties just get the "not null" treatment.

I added assertions for stack trace lines. Test data contains multiple stack traces, test verifies actual values for only one stack trace, for the rest it just tests for not null.

mateuszrzeszutek · 2022-03-31T14:57:35Z

.../com/splunk/opentelemetry/profiler/allocation/exporter/PlainTextAllocationEventExporter.java

+    String result = "\"" + name + "\"" + " #" + id;
+    if (thread != null) {
+      result += " nid=0x" + Long.toHexString(thread.getOSThreadId());
+    }
+    result += "\n";
+    result += "   java.lang.Thread.State: UNKNOWN\n" + stack;
+    return result;


How about using a StringBuilder?

I would leave it as it is because StringBuilder reduces the readability. Also the plan is to use pprof exporter by default so this class really won't be used by anything else besides our tests if all goes as planned. Perhaps we can even delete it at some point.

profiler/src/test/java/com/splunk/opentelemetry/profiler/exporter/StackTraceParserTest.java

gsmirnov-splk reviewed Feb 28, 2022

View reviewed changes

vsor-splk reviewed Mar 2, 2022

View reviewed changes

...java/com/splunk/opentelemetry/profiler/allocation/exporter/PprofAllocationEventExporter.java Outdated Show resolved Hide resolved

vsor-splk reviewed Mar 2, 2022

View reviewed changes

...java/com/splunk/opentelemetry/profiler/allocation/exporter/PprofAllocationEventExporter.java Outdated Show resolved Hide resolved

vsor-splk reviewed Mar 3, 2022

View reviewed changes

...java/com/splunk/opentelemetry/profiler/allocation/exporter/PprofAllocationEventExporter.java Outdated Show resolved Hide resolved

laurit force-pushed the pprof-proto branch from f0d894c to 59f762b Compare March 3, 2022 10:41

laurit force-pushed the pprof-proto branch from b6b40b8 to cc62293 Compare March 18, 2022 21:46

gsmirnov-splk reviewed Mar 23, 2022

View reviewed changes

laurit marked this pull request as ready for review March 25, 2022 09:17

laurit requested review from a team as code owners March 25, 2022 09:17

gsmirnov-splk reviewed Mar 28, 2022

View reviewed changes

...java/com/splunk/opentelemetry/profiler/allocation/exporter/PprofAllocationEventExporter.java Outdated Show resolved Hide resolved

vsor-splk reviewed Mar 28, 2022

View reviewed changes

profiler/src/main/java/com/splunk/opentelemetry/profiler/ProfilingDataType.java Outdated Show resolved Hide resolved

laurit force-pushed the pprof-proto branch from 46004b3 to 67aa6b6 Compare March 28, 2022 15:48

vsor-splk reviewed Mar 29, 2022

View reviewed changes

profiler/src/main/java/com/splunk/opentelemetry/profiler/Configuration.java Outdated Show resolved Hide resolved

vsor-splk approved these changes Mar 29, 2022

View reviewed changes

...java/com/splunk/opentelemetry/profiler/allocation/exporter/PprofAllocationEventExporter.java Outdated Show resolved Hide resolved

mateuszrzeszutek reviewed Mar 31, 2022

View reviewed changes

laurit force-pushed the pprof-proto branch 2 times, most recently from c842cd9 to 3fc1470 Compare April 12, 2022 14:50

mateuszrzeszutek approved these changes Apr 13, 2022

View reviewed changes

profiler/src/test/java/com/splunk/opentelemetry/profiler/exporter/StackTraceParserTest.java Outdated Show resolved Hide resolved

laurit added 11 commits April 14, 2022 13:25

Prototype sending allocation stacks as pprof

f8dcc54

Fix test

229765d

address review comments

34042c6

Fix test

ae4824c

set file name to unknown instead of blank string

443e58c

add idea plugin

7cfafe6

Set numeric values for labels as numbers

3658008

spotless

9d1ef85

always use unknown as file name

793246f

fix merge

5f7e589

remove unused import

e5aa28b

laurit added 18 commits April 14, 2022 13:25

thread stacks in pprof

fd7829c

remove unused files

7a231ce

review comments

0b34b75

move stack trace parsing to seaparate file

7311a51

polish

23d0a3f

change class back to package private

41cc737

allocation profiling does not have periodic events

4f2d65b

Rename profiling -> cpu, remove unused data formats

c541e33

test needs to use text format for allocation profiling

c5825b3

test needs to use text format for allocation profiling

8577811

make field final

d40e699

add thread status to allocation pprof

b72c189

sometimes intellij fails to find generated classes

f712bdf

replace thread status with thread state

f76ca5d

Send pprof only when it has samples

f1ff732

spec changes

311c8d4

review comments

1a31b62

use static import

cd4ffac

laurit force-pushed the pprof-proto branch from 8570442 to cd4ffac Compare April 14, 2022 10:25

laurit merged commit 30b6564 into signalfx:main Apr 14, 2022

laurit deleted the pprof-proto branch April 14, 2022 11:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prototype sending allocation stacks as pprof #684

Prototype sending allocation stacks as pprof #684

laurit commented Feb 23, 2022 •

edited

Loading

gsmirnov-splk Mar 23, 2022

mateuszrzeszutek Mar 31, 2022

laurit Apr 12, 2022

mateuszrzeszutek Mar 31, 2022

laurit Apr 12, 2022

mateuszrzeszutek Mar 31, 2022

mateuszrzeszutek Mar 31, 2022

laurit Apr 12, 2022

mateuszrzeszutek Mar 31, 2022

laurit Apr 12, 2022

Prototype sending allocation stacks as pprof #684

Prototype sending allocation stacks as pprof #684

Conversation

laurit commented Feb 23, 2022 • edited Loading

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

Choose a reason for hiding this comment

laurit commented Feb 23, 2022 •

edited

Loading