Skip to content
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

Create the profiling envelope item on a background thread #3893

Open
philipphofmann opened this issue Apr 24, 2024 · 1 comment
Open

Create the profiling envelope item on a background thread #3893

philipphofmann opened this issue Apr 24, 2024 · 1 comment

Comments

@philipphofmann
Copy link
Member

philipphofmann commented Apr 24, 2024

Description

Currently, the tracer creates the profile envelope item, which calls slicing the profiling data on the calling thread of SentryTracer.finishInternal here

- (void)captureTransactionWithProfile:(SentryTransaction *)transaction
startTimestamp:(NSDate *)startTimestamp
{
SentryEnvelopeItem *profileEnvelopeItem =
[SentryProfiler createProfilingEnvelopeItemForTransaction:transaction
startTimestamp:startTimestamp];
if (!profileEnvelopeItem) {
[_hub captureTransaction:transaction withScope:_hub.scope];
return;
}
SENTRY_LOG_DEBUG(@"Capturing transaction id %@ with profiling data attached for tracer id %@.",
transaction.eventId.sentryIdString, self.internalID.sentryIdString);
[_hub captureTransaction:transaction
withScope:_hub.scope
additionalEnvelopeItems:@[ profileEnvelopeItem ]];
}

When the profile lasts for a few seconds, this could noticeably block the main thread. With #3826 the captureTransaction in the hub already happens on a background thread. Moving createProfilingEnvelopeItemForTransaction to a background thread is a bit trickier, because if the tracer gets deallocated in the meantime, it might remove the profiling payload in dealloc:

- (void)dealloc
{
#if SENTRY_TARGET_PROFILING_SUPPORTED
if (self.isProfiling) {
sentry_discardProfilerForTracer(self.internalID);
}
#endif // SENTRY_TARGET_PROFILING_SUPPORTED
}

While we can somehow fix this problem with the current solution, continuous profiling (#3555) will change our current approach, and the solution could get outdated. @armcknight, does it make sense to consider this point while moving towards continuous profiling (#3555)?

@philipphofmann philipphofmann changed the title Create profile envelope item on a background thread Create the profiling envelope item on a background thread Apr 24, 2024
@armcknight
Copy link
Member

It does make sense to consider for the new implementation, definitely. For legacy profiling, I think it should be fine to move things to a bg thread once we have the transaction created by the tracer. There may be a few accesses of the tracer via the transaction from profiler code, but those could be replaced with parameters in the profiling methods/functions that are called so the values can be captured before offloading to the bg thread, risking deallocation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Backlog
Development

No branches or pull requests

2 participants