-
-
Notifications
You must be signed in to change notification settings - Fork 457
Profiling - Deduplication and cleanup #4681
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
base: feat/continuous-profiling-02
Are you sure you want to change the base?
Changes from 21 commits
bb12990
14d2f3b
bdd7b3a
1b88a3d
81aef38
8f32429
b7645f2
66ae530
037b237
2979b16
a32b71e
4b1e57e
8992018
c9a6bc0
451f191
885f0ab
7ae393e
b1701c5
30cb14b
94a37b1
524a32a
2cfe307
a559bb3
94a0097
0179ea3
66a6c33
bb10259
7a9c931
bcb7feb
0ea8a2d
2e54dde
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,22 +8,27 @@ | |
import io.sentry.asyncprofiler.vendor.asyncprofiler.jfr.JfrReader; | ||
import io.sentry.asyncprofiler.vendor.asyncprofiler.jfr.StackTrace; | ||
import io.sentry.asyncprofiler.vendor.asyncprofiler.jfr.event.Event; | ||
import io.sentry.asyncprofiler.vendor.asyncprofiler.jfr.event.EventCollector; | ||
import io.sentry.protocol.SentryStackFrame; | ||
import io.sentry.protocol.profiling.SentryProfile; | ||
import io.sentry.protocol.profiling.SentrySample; | ||
import io.sentry.protocol.profiling.SentryThreadMetadata; | ||
import java.io.IOException; | ||
import java.nio.file.Path; | ||
import java.util.ArrayList; | ||
import java.util.HashMap; | ||
import java.util.List; | ||
import java.util.Map; | ||
Comment on lines
18
to
+23
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The frame deduplication using Did we get this right? 👍 / 👎 to inform future reviews. |
||
import org.jetbrains.annotations.NotNull; | ||
import org.jetbrains.annotations.Nullable; | ||
|
||
public final class JfrAsyncProfilerToSentryProfileConverter extends JfrConverter { | ||
private static final long NANOS_PER_SECOND = 1_000_000_000L; | ||
private static final double NANOS_PER_SECOND = 1_000_000_000.0; | ||
|
||
private final @NotNull SentryProfile sentryProfile = new SentryProfile(); | ||
private final @NotNull SentryStackTraceFactory stackTraceFactory; | ||
private final @NotNull Map<SentryStackFrame, Integer> frameDeduplicationMap = new HashMap<>(); | ||
private final @NotNull Map<List<Integer>, Integer> stackDeduplicationMap = new HashMap<>(); | ||
|
||
public JfrAsyncProfilerToSentryProfileConverter( | ||
JfrReader jfr, Arguments args, @NotNull SentryStackTraceFactory stackTraceFactory) { | ||
|
@@ -36,12 +41,18 @@ protected void convertChunk() { | |
collector.forEach(new ProfileEventVisitor(sentryProfile, stackTraceFactory, jfr, args)); | ||
} | ||
|
||
@Override | ||
protected EventCollector createCollector(Arguments args) { | ||
return new NonAggregatingEventCollector(); | ||
} | ||
|
||
public static @NotNull SentryProfile convertFromFileStatic(@NotNull Path jfrFilePath) | ||
throws IOException { | ||
JfrAsyncProfilerToSentryProfileConverter converter; | ||
try (JfrReader jfrReader = new JfrReader(jfrFilePath.toString())) { | ||
Arguments args = new Arguments(); | ||
args.cpu = false; | ||
args.wall = true; | ||
args.alloc = false; | ||
args.threads = true; | ||
args.lines = true; | ||
|
@@ -56,11 +67,12 @@ protected void convertChunk() { | |
return converter.sentryProfile; | ||
} | ||
|
||
private class ProfileEventVisitor extends AggregatedEventVisitor { | ||
private class ProfileEventVisitor implements EventCollector.Visitor { | ||
private final @NotNull SentryProfile sentryProfile; | ||
private final @NotNull SentryStackTraceFactory stackTraceFactory; | ||
private final @NotNull JfrReader jfr; | ||
private final @NotNull Arguments args; | ||
private final double ticksPerNanosecond; | ||
|
||
public ProfileEventVisitor( | ||
@NotNull SentryProfile sentryProfile, | ||
|
@@ -71,10 +83,11 @@ public ProfileEventVisitor( | |
this.stackTraceFactory = stackTraceFactory; | ||
this.jfr = jfr; | ||
this.args = args; | ||
ticksPerNanosecond = jfr.ticksPerSec / NANOS_PER_SECOND; | ||
} | ||
|
||
@Override | ||
public void visit(Event event, long value) { | ||
public void visit(Event event, long samples, long value) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How detailed do you think we should do that? just drop the whole event? i.e. catch any exception inside the visit method and just keep going? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. opted to drop the event if an error occurs |
||
StackTrace stackTrace = jfr.stackTraces.get(event.stackTraceId); | ||
long threadId = resolveThreadId(event.tid); | ||
|
||
|
@@ -83,12 +96,16 @@ public void visit(Event event, long value) { | |
processThreadMetadata(event, threadId); | ||
} | ||
|
||
createSample(event, threadId); | ||
|
||
buildStackTraceAndFrames(stackTrace); | ||
processSampleWithStack(event, threadId, stackTrace); | ||
} | ||
} | ||
|
||
private long resolveThreadId(int eventThreadId) { | ||
return jfr.threads.get(eventThreadId) != null | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, potentially, if the orig. threadId somehow matches a different javaThreadId then we might map it to the wrong thread. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sending negative ids seems to work and Sentry Backend correctly matches them if both the Thread in the profile and in the Transaction have negative ids. So both options seem to be valid. However, I'm not sure if it helps to have a "Fallback" Thread that basically gets all unmapped events, as that graph would not make any sense. I think discarding the event makes more sense, WDYT? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Set threadId to -1 if it cannot be found in the map in follow-up PR: #4746 |
||
? jfr.javaThreads.get(eventThreadId) | ||
: eventThreadId; | ||
} | ||
|
||
private void processThreadMetadata(Event event, long threadId) { | ||
final String threadName = getPlainThreadName(event.tid); | ||
sentryProfile | ||
|
@@ -103,28 +120,73 @@ private void processThreadMetadata(Event event, long threadId) { | |
}); | ||
} | ||
|
||
private void buildStackTraceAndFrames(StackTrace stackTrace) { | ||
List<Integer> stack = new ArrayList<>(); | ||
int currentFrame = sentryProfile.getFrames().size(); | ||
private void processSampleWithStack(Event event, long threadId, StackTrace stackTrace) { | ||
int stackIndex = addStackTrace(stackTrace); | ||
|
||
SentrySample sample = new SentrySample(); | ||
sample.setTimestamp(calculateTimestamp(event)); | ||
sample.setThreadId(String.valueOf(threadId)); | ||
sample.setStackId(stackIndex); | ||
|
||
sentryProfile.getSamples().add(sample); | ||
} | ||
|
||
private double calculateTimestamp(Event event) { | ||
long nanosFromStart = (long) ((event.time - jfr.chunkStartTicks) / ticksPerNanosecond); | ||
|
||
long timeNs = jfr.chunkStartNanos + nanosFromStart; | ||
|
||
return DateUtils.nanosToSeconds(timeNs); | ||
} | ||
|
||
private int addStackTrace(StackTrace stackTrace) { | ||
List<Integer> callStack = createFramesAndCallStack(stackTrace); | ||
|
||
Integer existingIndex = stackDeduplicationMap.get(callStack); | ||
if (existingIndex != null) { | ||
return existingIndex; | ||
} | ||
|
||
int stackIndex = sentryProfile.getStacks().size(); | ||
sentryProfile.getStacks().add(callStack); | ||
stackDeduplicationMap.put(callStack, stackIndex); | ||
return stackIndex; | ||
} | ||
|
||
private List<Integer> createFramesAndCallStack(StackTrace stackTrace) { | ||
List<Integer> callStack = new ArrayList<>(); | ||
|
||
long[] methods = stackTrace.methods; | ||
byte[] types = stackTrace.types; | ||
int[] locations = stackTrace.locations; | ||
|
||
for (int i = 0; i < methods.length; i++) { | ||
StackTraceElement element = getStackTraceElement(methods[i], types[i], locations[i]); | ||
if (element.isNativeMethod()) { | ||
if (element.isNativeMethod() || isNativeFrame(types[i])) { | ||
continue; | ||
} | ||
|
||
SentryStackFrame frame = createStackFrame(element); | ||
sentryProfile.getFrames().add(frame); | ||
frame.setNative(isNativeFrame(types[i])); | ||
lbloder marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
int frameIndex = getOrAddFrame(frame); | ||
callStack.add(frameIndex); | ||
} | ||
|
||
stack.add(currentFrame); | ||
currentFrame++; | ||
return callStack; | ||
} | ||
|
||
// Get existing frame index or add new frame and return its index | ||
private int getOrAddFrame(SentryStackFrame frame) { | ||
Integer existingIndex = frameDeduplicationMap.get(frame); | ||
|
||
if (existingIndex != null) { | ||
return existingIndex; | ||
} | ||
|
||
sentryProfile.getStacks().add(stack); | ||
int newIndex = sentryProfile.getFrames().size(); | ||
sentryProfile.getFrames().add(frame); | ||
frameDeduplicationMap.put(frame, newIndex); | ||
return newIndex; | ||
} | ||
|
||
private SentryStackFrame createStackFrame(StackTraceElement element) { | ||
|
@@ -176,36 +238,12 @@ private boolean isRegularClassWithoutPackage(String className) { | |
return !className.startsWith("["); | ||
} | ||
|
||
private void createSample(Event event, long threadId) { | ||
int stackId = sentryProfile.getStacks().size(); | ||
SentrySample sample = new SentrySample(); | ||
|
||
// Calculate timestamp from JFR event time | ||
long nsFromStart = | ||
(event.time - jfr.chunkStartTicks) | ||
* JfrAsyncProfilerToSentryProfileConverter.NANOS_PER_SECOND | ||
/ jfr.ticksPerSec; | ||
long timeNs = jfr.chunkStartNanos + nsFromStart; | ||
sample.setTimestamp(DateUtils.nanosToSeconds(timeNs)); | ||
|
||
sample.setThreadId(String.valueOf(threadId)); | ||
sample.setStackId(stackId); | ||
|
||
sentryProfile.getSamples().add(sample); | ||
} | ||
|
||
private boolean shouldMarkAsSystemFrame(StackTraceElement element, String className) { | ||
return element.isNativeMethod() || className.isEmpty(); | ||
} | ||
|
||
private @Nullable Integer extractLineNumber(StackTraceElement element) { | ||
return element.getLineNumber() != 0 ? element.getLineNumber() : null; | ||
} | ||
|
||
private long resolveThreadId(int eventThreadId) { | ||
return jfr.threads.get(eventThreadId) != null | ||
? jfr.javaThreads.get(eventThreadId) | ||
: eventThreadId; | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,37 @@ | ||
package io.sentry.asyncprofiler.convert; | ||
|
||
import io.sentry.asyncprofiler.vendor.asyncprofiler.jfr.event.Event; | ||
import io.sentry.asyncprofiler.vendor.asyncprofiler.jfr.event.EventCollector; | ||
import java.util.ArrayList; | ||
import java.util.List; | ||
|
||
public final class NonAggregatingEventCollector implements EventCollector { | ||
final List<Event> events = new ArrayList<>(); | ||
|
||
@Override | ||
public void collect(Event e) { | ||
events.add(e); | ||
} | ||
|
||
@Override | ||
public void beforeChunk() { | ||
// No-op | ||
} | ||
|
||
@Override | ||
public void afterChunk() { | ||
// No-op | ||
} | ||
|
||
@Override | ||
public boolean finish() { | ||
return false; | ||
} | ||
|
||
@Override | ||
public void forEach(Visitor visitor) { | ||
for (Event event : events) { | ||
visitor.visit(event, event.samples(), event.value()); | ||
} | ||
} | ||
} |
Uh oh!
There was an error while loading. Please reload this page.