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

make profiling rate defaults to 101 hz #2211

Merged
merged 7 commits into from
Aug 8, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 8 additions & 3 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,17 @@

## Unreleased

### Fixes
Copy link
Member

Choose a reason for hiding this comment

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

Ah didn't notice the changelog was wrong on main

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, probably could even be when i merged another pr 😅. Not sure, though


- 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

### 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

Expand All @@ -14,13 +22,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)
Expand Down
2 changes: 2 additions & 0 deletions sentry-android-core/api/sentry-android-core.api
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
}

Expand Down
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -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);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 */
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -148,26 +149,26 @@ 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
profiler.onTransactionStart(fixture.transaction1)
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
)
}

Expand All @@ -194,16 +195,27 @@ 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)
val traceData = profiler.onTransactionFinish(fixture.transaction1)
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)
Expand Down