From bcf51c02e7ebb0f3c9ce8a3ccb1fc5c13714c28b Mon Sep 17 00:00:00 2001 From: stefanosiano Date: Fri, 5 Aug 2022 18:05:55 +0200 Subject: [PATCH 1/4] deprecated profilingTracesIntervalMillis option added internal profilingTracesHz option default profilingTracesHz is now 101, to avoid lockstep sampling --- .../api/sentry-android-core.api | 2 ++ .../core/AndroidTransactionProfiler.java | 12 +++---- .../android/core/SentryAndroidOptions.java | 34 ++++++++++++++++--- .../core/AndroidTransactionProfilerTest.kt | 28 ++++++++++----- 4 files changed, 57 insertions(+), 19 deletions(-) diff --git a/sentry-android-core/api/sentry-android-core.api b/sentry-android-core/api/sentry-android-core.api index be61d862e7..0fac802055 100644 --- a/sentry-android-core/api/sentry-android-core.api +++ b/sentry-android-core/api/sentry-android-core.api @@ -125,6 +125,7 @@ public final class io/sentry/android/core/SentryAndroidOptions : io/sentry/Sentr public fun enableAllAutoBreadcrumbs (Z)V public fun getAnrTimeoutIntervalMillis ()J public fun getDebugImagesLoader ()Lio/sentry/android/core/IDebugImagesLoader; + public fun getProfilingTracesHz ()I public fun getProfilingTracesIntervalMillis ()I public fun isAnrEnabled ()Z public fun isAnrReportInDebug ()Z @@ -152,6 +153,7 @@ public final class io/sentry/android/core/SentryAndroidOptions : io/sentry/Sentr public fun setEnableSystemEventBreadcrumbs (Z)V public fun setEnableUserInteractionBreadcrumbs (Z)V public fun setEnableUserInteractionTracing (Z)V + public fun setProfilingTracesHz (I)V public fun setProfilingTracesIntervalMillis (I)V } diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/AndroidTransactionProfiler.java b/sentry-android-core/src/main/java/io/sentry/android/core/AndroidTransactionProfiler.java index 22eeec671b..5028fd6361 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/AndroidTransactionProfiler.java +++ b/sentry-android-core/src/main/java/io/sentry/android/core/AndroidTransactionProfiler.java @@ -1,7 +1,7 @@ package io.sentry.android.core; import static android.content.Context.ACTIVITY_SERVICE; -import static java.util.concurrent.TimeUnit.MILLISECONDS; +import static java.util.concurrent.TimeUnit.SECONDS; import android.annotation.SuppressLint; import android.app.ActivityManager; @@ -81,17 +81,17 @@ private void init() { "Disabling profiling because no profiling traces dir path is defined in options."); return; } - long intervalMillis = options.getProfilingTracesIntervalMillis(); - if (intervalMillis <= 0) { + final int intervalHz = options.getProfilingTracesHz(); + if (intervalHz <= 0) { options .getLogger() .log( SentryLevel.WARNING, - "Disabling profiling because trace interval is set to %d milliseconds", - intervalMillis); + "Disabling profiling because trace rate is set to %d", + intervalHz); return; } - intervalUs = (int) MILLISECONDS.toMicros(intervalMillis); + intervalUs = (int) SECONDS.toMicros(1) / intervalHz; traceFilesDir = new File(tracesFilesDirPath); } diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/SentryAndroidOptions.java b/sentry-android-core/src/main/java/io/sentry/android/core/SentryAndroidOptions.java index e2c2e8b7ec..32eb2e01ed 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/SentryAndroidOptions.java +++ b/sentry-android-core/src/main/java/io/sentry/android/core/SentryAndroidOptions.java @@ -6,6 +6,7 @@ import io.sentry.SentryOptions; import io.sentry.SpanStatus; import io.sentry.protocol.SdkVersion; +import org.jetbrains.annotations.ApiStatus; import org.jetbrains.annotations.NotNull; /** Sentry SDK options for Android */ @@ -84,8 +85,12 @@ public final class SentryAndroidOptions extends SentryOptions { */ private boolean enableActivityLifecycleTracingAutoFinish = true; - /** Interval for profiling traces in milliseconds. Defaults to 100 times per second */ - private int profilingTracesIntervalMillis = 1_000 / 100; + /** + * Profiling traces rate. 101 hz means 101 traces in 1 second. Defaults to 101 to avoid possible + * lockstep sampling. More on + * https://stackoverflow.com/questions/45470758/what-is-lockstep-sampling + */ + private int profilingTracesHz = 101; /** Enables the Auto instrumentation for user interaction tracing. */ private boolean enableUserInteractionTracing = false; @@ -235,18 +240,37 @@ public void enableAllAutoBreadcrumbs(boolean enable) { * Returns the interval for profiling traces in milliseconds. * * @return the interval for profiling traces in milliseconds. + * @deprecated has no effect and will be removed in future versions. It now just returns 0. */ + @Deprecated + @SuppressWarnings("InlineMeSuggester") public int getProfilingTracesIntervalMillis() { - return profilingTracesIntervalMillis; + return 0; } /** * Sets the interval for profiling traces in milliseconds. * * @param profilingTracesIntervalMillis - the interval for profiling traces in milliseconds. + * @deprecated has no effect and will be removed in future versions. + */ + @Deprecated + public void setProfilingTracesIntervalMillis(final int profilingTracesIntervalMillis) {} + + /** + * Returns the rate the profiler will sample rates at. 100 hz means 100 traces in 1 second. + * + * @return Rate the profiler will sample rates at. */ - public void setProfilingTracesIntervalMillis(final int profilingTracesIntervalMillis) { - this.profilingTracesIntervalMillis = profilingTracesIntervalMillis; + @ApiStatus.Internal + public int getProfilingTracesHz() { + return profilingTracesHz; + } + + /** Sets the rate the profiler will sample rates at. 100 hz means 100 traces in 1 second. */ + @ApiStatus.Internal + public void setProfilingTracesHz(final int profilingTracesHz) { + this.profilingTracesHz = profilingTracesHz; } /** diff --git a/sentry-android-core/src/test/java/io/sentry/android/core/AndroidTransactionProfilerTest.kt b/sentry-android-core/src/test/java/io/sentry/android/core/AndroidTransactionProfilerTest.kt index bc54ef4a6d..22a704b4ca 100644 --- a/sentry-android-core/src/test/java/io/sentry/android/core/AndroidTransactionProfilerTest.kt +++ b/sentry-android-core/src/test/java/io/sentry/android/core/AndroidTransactionProfilerTest.kt @@ -21,6 +21,7 @@ import kotlin.test.BeforeTest import kotlin.test.Test import kotlin.test.assertEquals import kotlin.test.assertFailsWith +import kotlin.test.assertNotNull import kotlin.test.assertNull @RunWith(AndroidJUnit4::class) @@ -148,17 +149,17 @@ class AndroidTransactionProfilerTest { } @Test - fun `profiler evaluates profilingTracesIntervalMillis options only on first transaction profiling`() { + fun `profiler evaluates profilingTracesHz options only on first transaction profiling`() { fixture.options.apply { - profilingTracesIntervalMillis = 0 + profilingTracesHz = 0 } // We create the profiler, and nothing goes wrong val profiler = fixture.getSut(context) verify(fixture.mockLogger, never()).log( SentryLevel.WARNING, - "Disabling profiling because trace interval is set to %d milliseconds", - 0L + "Disabling profiling because trace rate is set to %d", + 0 ) // Regardless of how many times the profiler is started, the option is evaluated and logged only once @@ -166,8 +167,8 @@ class AndroidTransactionProfilerTest { profiler.onTransactionStart(fixture.transaction1) verify(fixture.mockLogger, times(1)).log( SentryLevel.WARNING, - "Disabling profiling because trace interval is set to %d milliseconds", - 0L + "Disabling profiling because trace rate is set to %d", + 0 ) } @@ -194,9 +195,9 @@ class AndroidTransactionProfilerTest { } @Test - fun `profiler on profilingTracesIntervalMillis 0`() { + fun `profiler on profilingTracesHz 0`() { fixture.options.apply { - profilingTracesIntervalMillis = 0 + profilingTracesHz = 0 } val profiler = fixture.getSut(context) profiler.onTransactionStart(fixture.transaction1) @@ -204,6 +205,17 @@ class AndroidTransactionProfilerTest { assertNull(traceData) } + @Test + fun `profiler ignores profilingTracesIntervalMillis`() { + fixture.options.apply { + profilingTracesIntervalMillis = 0 + } + val profiler = fixture.getSut(context) + profiler.onTransactionStart(fixture.transaction1) + val traceData = profiler.onTransactionFinish(fixture.transaction1) + assertNotNull(traceData) + } + @Test fun `onTransactionFinish works only if previously started`() { val profiler = fixture.getSut(context) From c8f1d560a610eeef05de6b5e13b96e0540455c97 Mon Sep 17 00:00:00 2001 From: stefanosiano Date: Fri, 5 Aug 2022 18:16:02 +0200 Subject: [PATCH 2/4] deprecated profilingTracesIntervalMillis option added internal profilingTracesHz option default profilingTracesHz is now 101, to avoid lockstep sampling --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 38e94d456c..b7b85ca9ad 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,10 @@ ## Unreleased +### Fixes + +- make profiling rate defaults to 101 hz ([#2211](https://github.com/getsentry/sentry-java/pull/2211)) + ### Features - Send source for transactions ([#2180](https://github.com/getsentry/sentry-java/pull/2180)) From 4e4e931561b44389e1eaa4740d255864bbea84fe Mon Sep 17 00:00:00 2001 From: stefanosiano Date: Fri, 5 Aug 2022 18:17:24 +0200 Subject: [PATCH 3/4] deprecated profilingTracesIntervalMillis option added internal profilingTracesHz option default profilingTracesHz is now 101, to avoid lockstep sampling --- CHANGELOG.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b7b85ca9ad..8b164ee9cd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,10 +5,13 @@ ### Fixes - make profiling rate defaults to 101 hz ([#2211](https://github.com/getsentry/sentry-java/pull/2211)) +- Added cpu architecture and default environment in profiles envelope ([#2207](https://github.com/getsentry/sentry-java/pull/2207)) +- SentryOptions.setProfilingEnabled has been deprecated in favor of setProfilesSampleRate ### Features - Send source for transactions ([#2180](https://github.com/getsentry/sentry-java/pull/2180)) +- Add profilesSampleRate and profileSampler options for Android sdk ([#2184](https://github.com/getsentry/sentry-java/pull/2184)) ## 6.3.1 @@ -18,13 +21,10 @@ - Weakly reference Activity for transaction finished callback ([#2203](https://github.com/getsentry/sentry-java/pull/2203)) - `attach-screenshot` set on Manual init. didn't work ([#2186](https://github.com/getsentry/sentry-java/pull/2186)) - Remove extra space from `spring.factories` causing issues in old versions of Spring Boot ([#2181](https://github.com/getsentry/sentry-java/pull/2181)) -- Added cpu architecture and default environment in profiles envelope ([#2207](https://github.com/getsentry/sentry-java/pull/2207)) ### Features -- Add profilesSampleRate and profileSampler options for Android sdk ([#2184](https://github.com/getsentry/sentry-java/pull/2184)) -- SentryOptions.setProfilingEnabled has been deprecated in favor of setProfilesSampleRate - Bump Native SDK to v0.4.18 ([#2154](https://github.com/getsentry/sentry-java/pull/2154)) - [changelog](https://github.com/getsentry/sentry-native/blob/master/CHANGELOG.md#0418) - [diff](https://github.com/getsentry/sentry-native/compare/0.4.17...0.4.18) From 87ae5ed47d7a89114dd1c6c9482e41a6a58eca61 Mon Sep 17 00:00:00 2001 From: stefanosiano Date: Mon, 8 Aug 2022 10:22:09 +0200 Subject: [PATCH 4/4] updated changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8b164ee9cd..4e03654aac 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ ### Fixes - make profiling rate defaults to 101 hz ([#2211](https://github.com/getsentry/sentry-java/pull/2211)) +- SentryOptions.setProfilingTracesIntervalMillis has been deprecated - Added cpu architecture and default environment in profiles envelope ([#2207](https://github.com/getsentry/sentry-java/pull/2207)) - SentryOptions.setProfilingEnabled has been deprecated in favor of setProfilesSampleRate