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

Allow opting out of device info collection that requires Inter-Process Communication (IPC) #2100

Merged
merged 6 commits into from
Jun 17, 2022
Merged
Show file tree
Hide file tree
Changes from 2 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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
### Features

- Replace `tracestate` header with `baggage` header ([#2078](https://github.com/getsentry/sentry-java/pull/2078))
- Allow opting out of device info collection that requires Inter-Process Communication (IPC) ([#2100](https://github.com/getsentry/sentry-java/pull/2100))

## 6.1.0

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 @@ -129,6 +129,7 @@ public final class io/sentry/android/core/SentryAndroidOptions : io/sentry/Sentr
public fun isAnrEnabled ()Z
public fun isAnrReportInDebug ()Z
public fun isAttachScreenshot ()Z
public fun isCollectIpcDeviceInfo ()Z
public fun isEnableActivityLifecycleBreadcrumbs ()Z
public fun isEnableActivityLifecycleTracingAutoFinish ()Z
public fun isEnableAppComponentBreadcrumbs ()Z
Expand All @@ -141,6 +142,7 @@ public final class io/sentry/android/core/SentryAndroidOptions : io/sentry/Sentr
public fun setAnrReportInDebug (Z)V
public fun setAnrTimeoutIntervalMillis (J)V
public fun setAttachScreenshot (Z)V
public fun setCollectIpcDeviceInfo (Z)V
public fun setDebugImagesLoader (Lio/sentry/android/core/IDebugImagesLoader;)V
public fun setEnableActivityLifecycleBreadcrumbs (Z)V
public fun setEnableActivityLifecycleTracingAutoFinish (Z)V
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,8 @@ static void init(

readDefaultOptionValues(options, context);

options.addEventProcessor(new DefaultAndroidEventProcessor(context, logger, buildInfoProvider));
options.addEventProcessor(
new DefaultAndroidEventProcessor(context, logger, buildInfoProvider, options));
options.addEventProcessor(new PerformanceAndroidEventProcessor(options, activityFramesTracker));

options.setTransportGate(new AndroidTransportGate(context, options.getLogger()));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,26 +69,35 @@ final class DefaultAndroidEventProcessor implements EventProcessor {

private final @NotNull BuildInfoProvider buildInfoProvider;
private final @NotNull RootChecker rootChecker;
private final @NotNull SentryAndroidOptions options;

private final @NotNull ILogger logger;

public DefaultAndroidEventProcessor(
final @NotNull Context context,
final @NotNull ILogger logger,
final @NotNull BuildInfoProvider buildInfoProvider) {
this(context, logger, buildInfoProvider, new RootChecker(context, buildInfoProvider, logger));
final @NotNull BuildInfoProvider buildInfoProvider,
final @NotNull SentryAndroidOptions options) {
this(
context,
logger,
bruno-garcia marked this conversation as resolved.
Show resolved Hide resolved
buildInfoProvider,
new RootChecker(context, buildInfoProvider, logger),
options);
}

DefaultAndroidEventProcessor(
final @NotNull Context context,
final @NotNull ILogger logger,
final @NotNull BuildInfoProvider buildInfoProvider,
final @NotNull RootChecker rootChecker) {
final @NotNull RootChecker rootChecker,
final @NotNull SentryAndroidOptions options) {
this.context = Objects.requireNonNull(context, "The application context is required.");
this.logger = Objects.requireNonNull(logger, "The Logger is required.");
this.buildInfoProvider =
Objects.requireNonNull(buildInfoProvider, "The BuildInfoProvider is required.");
this.rootChecker = Objects.requireNonNull(rootChecker, "The RootChecker is required.");
this.options = Objects.requireNonNull(options, "The options object is required.");

ExecutorService executorService = Executors.newSingleThreadExecutor();
// dont ref. to method reference, theres a bug on it
Expand Down Expand Up @@ -328,36 +337,42 @@ private void setArchitectures(final @NotNull Device device) {
}

private void setDeviceIO(final @NotNull Device device, final boolean applyScopeData) {
final Intent batteryIntent = getBatteryIntent();
if (batteryIntent != null) {
device.setBatteryLevel(getBatteryLevel(batteryIntent));
device.setCharging(isCharging(batteryIntent));
device.setBatteryTemperature(getBatteryTemperature(batteryIntent));
}

Boolean connected;
switch (ConnectivityChecker.getConnectionStatus(context, logger)) {
case NOT_CONNECTED:
connected = false;
break;
case CONNECTED:
connected = true;
break;
default:
connected = null;
}
device.setOnline(connected);

final ActivityManager.MemoryInfo memInfo = getMemInfo();
if (memInfo != null) {
// in bytes
device.setMemorySize(getMemorySize(memInfo));
if (applyScopeData) {
device.setFreeMemory(memInfo.availMem);
device.setLowMemory(memInfo.lowMemory);
if (options.isCollectIpcDeviceInfo()) {
final Intent batteryIntent = getBatteryIntent();
if (batteryIntent != null) {
device.setBatteryLevel(getBatteryLevel(batteryIntent));
device.setCharging(isCharging(batteryIntent));
device.setBatteryTemperature(getBatteryTemperature(batteryIntent));
}
}

if (options.isCollectIpcDeviceInfo()) {
Boolean connected;
switch (ConnectivityChecker.getConnectionStatus(context, logger)) {
case NOT_CONNECTED:
connected = false;
break;
case CONNECTED:
connected = true;
break;
default:
connected = null;
}
device.setOnline(connected);
}

if (options.isCollectIpcDeviceInfo()) {
bruno-garcia marked this conversation as resolved.
Show resolved Hide resolved
final ActivityManager.MemoryInfo memInfo = getMemInfo();
if (memInfo != null) {
// in bytes
device.setMemorySize(getMemorySize(memInfo));
if (applyScopeData) {
device.setFreeMemory(memInfo.availMem);
device.setLowMemory(memInfo.lowMemory);
}
// there are runtime.totalMemory() and runtime.freeMemory(), but I kept the same for
// compatibility
}
// there are runtime.totalMemory() and runtime.freeMemory(), but I kept the same for
// compatibility
}

// this way of getting the size of storage might be problematic for storages bigger than 2GB
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ final class ManifestMetadataReader {

static final String ATTACH_SCREENSHOT = "io.sentry.attach-screenshot";
static final String CLIENT_REPORTS_ENABLE = "io.sentry.send-client-reports";
static final String COLLECT_IPC_DEVICE_INFO = "io.sentry.collect-ipc-device-info";
adinauer marked this conversation as resolved.
Show resolved Hide resolved

/** ManifestMetadataReader ctor */
private ManifestMetadataReader() {}
Expand Down Expand Up @@ -210,6 +211,9 @@ static void applyMetadata(
options.setSendClientReports(
readBool(metadata, logger, CLIENT_REPORTS_ENABLE, options.isSendClientReports()));

options.setCollectIpcDeviceInfo(
readBool(metadata, logger, COLLECT_IPC_DEVICE_INFO, options.isCollectIpcDeviceInfo()));

if (options.getTracesSampleRate() == null) {
final Double tracesSampleRate = readDouble(metadata, logger, TRACES_SAMPLE_RATE);
if (tracesSampleRate != -1) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,12 @@ public final class SentryAndroidOptions extends SentryOptions {
/** Enables or disables the attach screenshot feature when an error happened. */
private boolean attachScreenshot;

/**
* Enables or disables collecting of device information which requires Inter-Process Communication
* (IPC)
*/
private boolean collectIpcDeviceInfo = true;

public SentryAndroidOptions() {
setSentryClientName(BuildConfig.SENTRY_ANDROID_SDK_NAME + "/" + BuildConfig.VERSION_NAME);
setSdkVersion(createSdkVersion());
Expand Down Expand Up @@ -294,4 +300,12 @@ public boolean isEnableUserInteractionTracing() {
public void setEnableUserInteractionTracing(boolean enableUserInteractionTracing) {
this.enableUserInteractionTracing = enableUserInteractionTracing;
}

public boolean isCollectIpcDeviceInfo() {
return collectIpcDeviceInfo;
}

public void setCollectIpcDeviceInfo(boolean collectIpcDeviceInfo) {
this.collectIpcDeviceInfo = collectIpcDeviceInfo;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ import io.sentry.Hint
import io.sentry.ILogger
import io.sentry.SentryEvent
import io.sentry.SentryLevel
import io.sentry.SentryOptions
import io.sentry.SentryTracer
import io.sentry.TransactionContext
import io.sentry.android.core.DefaultAndroidEventProcessor.EMULATOR
Expand Down Expand Up @@ -48,23 +47,23 @@ class DefaultAndroidEventProcessorTest {

private val className = "io.sentry.android.core.DefaultAndroidEventProcessor"
private val ctorTypes =
arrayOf(Context::class.java, ILogger::class.java, BuildInfoProvider::class.java)
arrayOf(Context::class.java, ILogger::class.java, BuildInfoProvider::class.java, SentryAndroidOptions::class.java)

init {
Locale.setDefault(Locale.US)
}

private class Fixture {
val buildInfo = mock<BuildInfoProvider>()
val options = SentryOptions().apply {
val options = SentryAndroidOptions().apply {
setDebug(true)
setLogger(mock())
sdkVersion = SdkVersion("test", "1.2.3")
}
val sentryTracer = SentryTracer(TransactionContext("", ""), mock())

fun getSut(context: Context): DefaultAndroidEventProcessor {
return DefaultAndroidEventProcessor(context, options.logger, buildInfo)
return DefaultAndroidEventProcessor(context, options.logger, buildInfo, options)
}
}

Expand All @@ -86,23 +85,31 @@ class DefaultAndroidEventProcessorTest {
fun `when null context is provided, invalid argument is thrown`() {
val ctor = className.getCtor(ctorTypes)

val params = arrayOf(null, mock<SentryOptions>(), null)
val params = arrayOf(null, mock<ILogger>(), null, mock<SentryAndroidOptions>())
assertFailsWith<IllegalArgumentException> { ctor.newInstance(params) }
}

@Test
fun `when null logger is provided, invalid argument is thrown`() {
val ctor = className.getCtor(ctorTypes)

val params = arrayOf(mock<Context>(), null, null, mock<SentryAndroidOptions>())
assertFailsWith<IllegalArgumentException> { ctor.newInstance(params) }
}

@Test
fun `when null options is provided, invalid argument is thrown`() {
val ctor = className.getCtor(ctorTypes)

val params = arrayOf(mock<Context>(), null, null)
val params = arrayOf(mock<Context>(), mock<ILogger>(), mock<BuildInfoProvider>(), null)
assertFailsWith<IllegalArgumentException> { ctor.newInstance(params) }
}

@Test
fun `when null buildInfo is provided, invalid argument is thrown`() {
val ctor = className.getCtor(ctorTypes)

val params = arrayOf(null, null, mock<BuildInfoProvider>())
val params = arrayOf(null, null, mock<BuildInfoProvider>(), mock<SentryAndroidOptions>())
assertFailsWith<IllegalArgumentException> { ctor.newInstance(params) }
}

Expand Down Expand Up @@ -456,6 +463,25 @@ class DefaultAndroidEventProcessorTest {
}
}

@Test
fun `Does not collect device info that requires IPC if disabled`() {
Copy link
Member

Choose a reason for hiding this comment

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

Would be good to check that by default (no change in SentryOptions) we do get the additional context

Copy link
Member Author

Choose a reason for hiding this comment

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

fixture.options.isCollectIpcDeviceInfo = false
val sut = fixture.getSut(context)

assertNotNull(sut.process(SentryEvent(), Hint())) {
val device = it.contexts.device!!
assertNull(device.freeMemory)
assertNull(device.isLowMemory)

// commented values are not mocked by robolectric
// assertNotNull(device.batteryLevel)
// assertNotNull(device.isCharging)
// assertNotNull(device.batteryTemperature)
// assertNotNull(device.isOnline)
// assertNotNull(device.connectionType)
}
}

@Test
fun `Event sets language and locale`() {
val sut = fixture.getSut(context)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -919,4 +919,29 @@ class ManifestMetadataReaderTest {
// Assert
assertEquals(3000L, fixture.options.idleTimeout)
}

@Test
fun `applyMetadata reads collect ipc device info to options`() {
// Arrange
val bundle = bundleOf(ManifestMetadataReader.COLLECT_IPC_DEVICE_INFO to false)
val context = fixture.getContext(metaData = bundle)

// Act
ManifestMetadataReader.applyMetadata(context, fixture.options)

// Assert
assertFalse(fixture.options.isCollectIpcDeviceInfo)
}

@Test
fun `applyMetadata reads collect ipc device info and keep default value if not found`() {
// Arrange
val context = fixture.getContext()

// Act
ManifestMetadataReader.applyMetadata(context, fixture.options)

// Assert
assertTrue(fixture.options.isCollectIpcDeviceInfo)
}
}