-
-
Notifications
You must be signed in to change notification settings - Fork 454
Support ProfileLifecycle.TRACE #4576
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/poc-continuous-profiling
Are you sure you want to change the base?
Support ProfileLifecycle.TRACE #4576
Conversation
…TRACE is used to have the profile ID when SentryTracer is created
…ingBoot autoconfig
…and make fields private
…the interval separately, this seems to work better and create more samples
…is not configured
Instructions and example for changelogPlease add an entry to Example: ## Unreleased
- Support ProfileLifecycle.TRACE ([#4576](https://github.com/getsentry/sentry-java/pull/4576)) If none of the above apply, you can opt out of this check by adding |
Performance metrics 🚀
|
…entry-java into feat/continuous-profiling-02
@sentry review |
cursor review |
String profileLifecycleString = propertiesProvider.getProperty("profile-lifecycle"); | ||
if (profileLifecycleString != null && !profileLifecycleString.isEmpty()) { | ||
options.setProfileLifecycle(ProfileLifecycle.valueOf(profileLifecycleString.toUpperCase())); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Profile Lifecycle Parsing Error Handling
Missing error handling when parsing the profile-lifecycle
property. If an invalid string value is provided, ProfileLifecycle.valueOf(profileLifecycleString.toUpperCase())
throws an IllegalArgumentException
, causing the application to crash during initialization. Error handling should be added to prevent crashes, log a warning, and optionally fall back to a default value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adinauer WDYT, what should we do here. catch and ignore the exception if an unsupported value is present? That is basically what we do in getDoubleProperty
in the ExternalOptions
as well if its not a number we just set it to null
if (profilingTracesDirPath == null) { | ||
profilingTracesDirPath = | ||
Files.createTempDirectory("profiling_traces").toAbsolutePath().toString(); | ||
options.setProfilingTracesDirPath(profilingTracesDirPath); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential bug: Repeated `Sentry.init()` calls create new temporary profiling directories without cleaning up old ones, leading to a disk space leak over time.
- Description: When continuous profiling is enabled and
profilingTracesDirPath
is not configured,Sentry.init()
callsFiles.createTempDirectory()
to create a directory for traces. However, ifSentry.init()
is called multiple times during the application's lifecycle, a new temporary directory is created each time without removing the previous one. This leads to an accumulation of directories on the disk, which can exhaust disk space in long-running applications, potentially causing the application or server to fail. ATODO
comment inJavaContinuousProfiler.java
acknowledges this lack of cleanup on restart. - Suggested fix: Implement a cleanup mechanism. Before creating a new temporary directory in
initJvmContinuousProfiling
, check if a temporary path was already set by a previousinit()
call and reuse or clean it up. Alternatively, register a shutdown hook to delete the created temporary directory when the application exits. The directory path could be stored in a static field to be managed acrossinit()
calls.
severity: 0.6, confidence: 0.95
Did we get this right? 👍 / 👎 to inform future reviews.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Empty dirs shouldn't be an issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would think so too. If this becomes a problem we can come up with a better way to handle this
@@ -283,14 +272,21 @@ private void stop(final boolean restartProfiler) { | |||
// start profiling), meaning there's no scopes to send the chunks. In that case, we store | |||
// the data in a list and send it when the next chunk is finished. | |||
try (final @NotNull ISentryLifecycleToken ignored2 = payloadLock.acquire()) { | |||
File jfrFile = new File(filename); | |||
// TODO: should we add deleteOnExit() here to let the JVM clean up the file? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, deleting sounds like the way to go here since we likely can't find it on restart anyways.
@@ -159,6 +171,13 @@ public boolean isStartRequired() { | |||
public void onEnd(final @NotNull ReadableSpan spanBeingEnded) { | |||
final @Nullable IOtelSpanWrapper sentrySpan = | |||
spanStorage.getSentrySpan(spanBeingEnded.getSpanContext()); | |||
|
|||
if (isRootSpan(spanBeingEnded.toSpanData()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
l
So, if a span is never ended we keep the profiler running indefinitely, correct? We might need to fix this at some point in the future. For now I'd say we can keep it as is.
@@ -82,6 +83,17 @@ public void onStart(final @NotNull Context parentContext, final @NotNull ReadWri | |||
|
|||
final @Nullable Boolean sampled = isSampled(otelSpan, samplingDecision); | |||
|
|||
if (Boolean.TRUE.equals(sampled) && isRootSpan(otelSpan.toSpanData())) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
l
Is there much benefit to only starting/stopping the profiler for root spans?
My thoughts here:
- might help avoid keeping the profiler running if a span isn't finished (see comment further down)
- might cause issues when we move to span streaming (not sure if there's ever a case where we already have a finished root span but one/multiple children are still running)
We can keep the implementation as is for now, I just wanted to dump my thoughts here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've taken that logic from how it works with Transactions in Sentry. Whenever we start/finish a new Transaction the profiler is started/stopped. So in OTEL it should now work the same as in Sentry without OTEL.
If we ever have Spans that are running without a Transaction or Root span we need to rethink the logic in both Sentry Tracing and OTEL Tracing.
I'm with you to keep it that way for now.
@@ -202,6 +203,33 @@ public ProfileChunk build(SentryOptions options) { | |||
} | |||
} | |||
|
|||
public enum Platform implements JsonSerializable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
h
Will this cause problems with other SDKs going through Java SDKs de/serialization? Is a String more flexible here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, good point.... so, WDYT, is it safer to revert the changes around the enum and just use string constants for "android" and "java" instead?
if (profilingTracesDirPath == null) { | ||
profilingTracesDirPath = | ||
Files.createTempDirectory("profiling_traces").toAbsolutePath().toString(); | ||
options.setProfilingTracesDirPath(profilingTracesDirPath); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Empty dirs shouldn't be an issue.
📜 Description
ProfileChunk
💡 Motivation and Context
Follow-Up to #4556
💚 How did you test it?
📝 Checklist
sendDefaultPII
is enabled.🔮 Next steps