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

Add Continuous Profiling support (v8) #3710

Open
wants to merge 7 commits into
base: 8.x.x
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions sentry-android-core/api/sentry-android-core.api
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,15 @@ public final class io/sentry/android/core/ActivityLifecycleIntegration : android
public fun register (Lio/sentry/IScopes;Lio/sentry/SentryOptions;)V
}

public class io/sentry/android/core/AndroidContinuousProfiler : io/sentry/IContinuousProfiler {
public fun <init> (Lio/sentry/android/core/BuildInfoProvider;Lio/sentry/android/core/internal/util/SentryFrameMetricsCollector;Lio/sentry/ILogger;Ljava/lang/String;ZILio/sentry/ISentryExecutorService;)V
public fun close ()V
public fun isRunning ()Z
public fun setScopes (Lio/sentry/IScopes;)V
public fun start ()V
public fun stop ()V
}

public final class io/sentry/android/core/AndroidCpuCollector : io/sentry/IPerformanceSnapshotCollector {
public fun <init> (Lio/sentry/ILogger;Lio/sentry/android/core/BuildInfoProvider;)V
public fun collect (Lio/sentry/PerformanceCollectionData;)V
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,170 @@
package io.sentry.android.core;

import static java.util.concurrent.TimeUnit.SECONDS;

import android.annotation.SuppressLint;
import android.os.Build;
import io.sentry.IContinuousProfiler;
import io.sentry.ILogger;
import io.sentry.IScopes;
import io.sentry.ISentryExecutorService;
import io.sentry.SentryLevel;
import io.sentry.android.core.internal.util.SentryFrameMetricsCollector;
import java.util.concurrent.Future;
import org.jetbrains.annotations.ApiStatus;
import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;
import org.jetbrains.annotations.VisibleForTesting;

@ApiStatus.Internal
public class AndroidContinuousProfiler implements IContinuousProfiler {
private static final long MAX_CHUNK_DURATION_MILLIS = 10000;

private final @NotNull ILogger logger;
private final @Nullable String profilingTracesDirPath;
private final boolean isProfilingEnabled;
private final int profilingTracesHz;
private final @NotNull ISentryExecutorService executorService;
private final @NotNull BuildInfoProvider buildInfoProvider;
private boolean isInitialized = false;
private final @NotNull SentryFrameMetricsCollector frameMetricsCollector;
private @Nullable AndroidProfiler profiler = null;
private boolean isRunning = false;
private @Nullable IScopes scopes;
private @Nullable Future<?> closeFuture;

public AndroidContinuousProfiler(
final @NotNull BuildInfoProvider buildInfoProvider,
final @NotNull SentryFrameMetricsCollector frameMetricsCollector,
final @NotNull ILogger logger,
final @Nullable String profilingTracesDirPath,
final boolean isProfilingEnabled,
final int profilingTracesHz,
final @NotNull ISentryExecutorService executorService) {
this.logger = logger;
this.frameMetricsCollector = frameMetricsCollector;
this.buildInfoProvider = buildInfoProvider;
this.profilingTracesDirPath = profilingTracesDirPath;
this.isProfilingEnabled = isProfilingEnabled;
this.profilingTracesHz = profilingTracesHz;
this.executorService = executorService;
}

private void init() {
// We initialize it only once
if (isInitialized) {
return;
}
isInitialized = true;
if (!isProfilingEnabled) {
logger.log(SentryLevel.INFO, "Profiling is disabled in options.");
return;
}
if (profilingTracesDirPath == null) {
logger.log(
SentryLevel.WARNING,
"Disabling profiling because no profiling traces dir path is defined in options.");
return;
}
if (profilingTracesHz <= 0) {
logger.log(
SentryLevel.WARNING,
"Disabling profiling because trace rate is set to %d",
profilingTracesHz);
return;
}

profiler =
new AndroidProfiler(
profilingTracesDirPath,
(int) SECONDS.toMicros(1) / profilingTracesHz,
frameMetricsCollector,
null,
logger,
buildInfoProvider);
}

public synchronized void setScopes(final @NotNull IScopes scopes) {
this.scopes = scopes;
}

public synchronized void start() {
// Debug.startMethodTracingSampling() is only available since Lollipop, but Android Profiler
// causes crashes on api 21 -> https://github.com/getsentry/sentry-java/issues/3392
if (buildInfoProvider.getSdkInfoVersion() < Build.VERSION_CODES.LOLLIPOP_MR1) return;
stefanosiano marked this conversation as resolved.
Show resolved Hide resolved

// Let's initialize trace folder and profiling interval
init();
// init() didn't create profiler, should never happen
if (profiler == null) {
return;
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure if we should add an if (isRunning) { return; } check here to not call profiler.start again in case of sequential start calls

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's already checked inside the AndroidProfiler.start(), and logs a message and returns null in that case

final AndroidProfiler.ProfileStartData startData = profiler.start();
// check if profiling started
if (startData == null) {
return;
}
isRunning = true;

closeFuture = executorService.schedule(() -> stop(true), MAX_CHUNK_DURATION_MILLIS);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wondering if we should start using dedicated executors per feature - our main one is getting busier and busier, so if it's not a big stretch I'd create a new executorService for the whole profiling thing (if it doesn't depend on other things being executed by the main one)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also, this should be in try-catch probably, because of RejectedExecutionException

}

public synchronized void stop() {
stop(false);
}

@SuppressLint("NewApi")
private synchronized void stop(final boolean restartProfiler) {
if (closeFuture != null) {
closeFuture.cancel(true);
}
// check if profiler was created and it's running
if (profiler == null || !isRunning) {
return;
}

// onTransactionStart() is only available since Lollipop_MR1
// and SystemClock.elapsedRealtimeNanos() since Jelly Bean
if (buildInfoProvider.getSdkInfoVersion() < Build.VERSION_CODES.LOLLIPOP_MR1) {
romtsn marked this conversation as resolved.
Show resolved Hide resolved
return;
}

// todo add PerformanceCollectionData
final AndroidProfiler.ProfileEndData endData = profiler.endAndCollect(false, null);

// check if profiler end successfully
if (endData == null) {
return;
}

isRunning = false;

// todo schedule capture profile chunk envelope

if (restartProfiler) {
logger.log(SentryLevel.DEBUG, "Profile chunk finished. Starting a new one.");
start();
} else {
logger.log(SentryLevel.DEBUG, "Profile chunk finished.");
}
}

public synchronized void close() {
if (closeFuture != null) {
closeFuture.cancel(true);
}
stefanosiano marked this conversation as resolved.
Show resolved Hide resolved
stop();
}

@Override
public boolean isRunning() {
return isRunning;
}

@VisibleForTesting
@Nullable
Future<?> getCloseFuture() {
return closeFuture;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -93,22 +93,23 @@ public ProfileEndData(
new ArrayDeque<>();
private final @NotNull Map<String, ProfileMeasurement> measurementsMap = new HashMap<>();
private final @NotNull BuildInfoProvider buildInfoProvider;
private final @NotNull ISentryExecutorService executorService;
private final @Nullable ISentryExecutorService timeoutExecutorService;
private final @NotNull ILogger logger;
private boolean isRunning = false;

public AndroidProfiler(
final @NotNull String tracesFilesDirPath,
final int intervalUs,
final @NotNull SentryFrameMetricsCollector frameMetricsCollector,
final @NotNull ISentryExecutorService executorService,
final @Nullable ISentryExecutorService timeoutExecutorService,
final @NotNull ILogger logger,
final @NotNull BuildInfoProvider buildInfoProvider) {
this.traceFilesDir =
new File(Objects.requireNonNull(tracesFilesDirPath, "TracesFilesDirPath is required"));
this.intervalUs = intervalUs;
this.logger = Objects.requireNonNull(logger, "Logger is required");
this.executorService = Objects.requireNonNull(executorService, "ExecutorService is required.");
// Timeout executor is nullable, as timeouts will not be there for continuous profiling
this.timeoutExecutorService = timeoutExecutorService;
this.frameMetricsCollector =
Objects.requireNonNull(frameMetricsCollector, "SentryFrameMetricsCollector is required");
this.buildInfoProvider =
Expand Down Expand Up @@ -185,8 +186,11 @@ public void onFrameMetricCollected(

// We stop profiling after a timeout to avoid huge profiles to be sent
try {
scheduledFinish =
executorService.schedule(() -> endAndCollect(true, null), PROFILING_TIMEOUT_MILLIS);
if (timeoutExecutorService != null) {
scheduledFinish =
timeoutExecutorService.schedule(
() -> endAndCollect(true, null), PROFILING_TIMEOUT_MILLIS);
}
} catch (RejectedExecutionException e) {
logger.log(
SentryLevel.ERROR,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,7 @@ public synchronized void bindTransaction(final @NotNull ITransaction transaction

// onTransactionStart() is only available since Lollipop_MR1
// and SystemClock.elapsedRealtimeNanos() since Jelly Bean
// and SUPPORTED_ABIS since KITKAT
if (buildInfoProvider.getSdkInfoVersion() < Build.VERSION_CODES.LOLLIPOP_MR1) return null;

// Transaction finished, but it's not in the current profile
Expand Down
Loading
Loading