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

Fix: Allow MaxBreadcrumb 0 / Expose MaxBreadcrumb metadata. #3836

Merged
merged 8 commits into from
Nov 4, 2024
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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,12 @@

### Features

- Add meta option to set the maximum amount of breadcrumbs to be logged. ([#3836](https://github.com/getsentry/sentry-java/pull/3836))
- Use a separate `Random` instance per thread to improve SDK performance ([#3835](https://github.com/getsentry/sentry-java/pull/3835))

### Fixes

- Using MaxBreadcrumb with value 0 no longer crashes. ([#3836](https://github.com/getsentry/sentry-java/pull/3836))
- Accept manifest integer values when requiring floating values ([#3823](https://github.com/getsentry/sentry-java/pull/3823))

## 7.16.0
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,8 @@ final class ManifestMetadataReader {

static final String ENABLE_METRICS = "io.sentry.enable-metrics";

static final String MAX_BREADCRUMBS = "io.sentry.max-breadcrumbs";

static final String REPLAYS_SESSION_SAMPLE_RATE = "io.sentry.session-replay.session-sample-rate";

static final String REPLAYS_ERROR_SAMPLE_RATE = "io.sentry.session-replay.on-error-sample-rate";
Expand Down Expand Up @@ -213,6 +215,9 @@ static void applyMetadata(
SESSION_TRACKING_TIMEOUT_INTERVAL_MILLIS,
options.getSessionTrackingIntervalMillis()));

options.setMaxBreadcrumbs(
(int) readLong(metadata, logger, MAX_BREADCRUMBS, options.getMaxBreadcrumbs()));

options.setEnableActivityLifecycleBreadcrumbs(
readBool(
metadata,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1516,6 +1516,31 @@ class ManifestMetadataReaderTest {
assertTrue(fixture.options.experimental.sessionReplay.maskViewClasses.contains(SentryReplayOptions.TEXT_VIEW_CLASS_NAME))
}

@Test
fun `applyMetadata reads maxBreadcrumbs to options and sets the value if found`() {
// Arrange
val bundle = bundleOf(ManifestMetadataReader.MAX_BREADCRUMBS to 1)
val context = fixture.getContext(metaData = bundle)

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

// Assert
assertEquals(1, fixture.options.maxBreadcrumbs)
}

@Test
fun `applyMetadata reads maxBreadcrumbs to options and keeps default if not found`() {
// Arrange
val context = fixture.getContext()

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

// Assert
assertEquals(100, fixture.options.maxBreadcrumbs)
}

@Test
fun `applyMetadata reads integers even when expecting floats`() {
// Arrange
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,9 @@
<!-- how to enable the attach screenshot feature-->
<meta-data android:name="io.sentry.attach-screenshot" android:value="true" />

<!-- how many breadcrumbs will be stored-->
<meta-data android:name="io.sentry.max-breadcrumbs" android:value="100"/>

<!-- how to enable the attach view hierarchy feature-->
<meta-data android:name="io.sentry.attach-view-hierarchy" android:value="true" />

Expand Down
115 changes: 115 additions & 0 deletions sentry/src/main/java/io/sentry/DisabledQueue.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,115 @@
package io.sentry;

import java.io.Serializable;
import java.util.AbstractCollection;
import java.util.Iterator;
import java.util.NoSuchElementException;
import java.util.Queue;
import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;

final class DisabledQueue<E> extends AbstractCollection<E> implements Queue<E>, Serializable {

/** Serialization version. */
private static final long serialVersionUID = -8423413834657610417L;

/** Constructor that creates a queue that does not accept any element. */
public DisabledQueue() {}

// -----------------------------------------------------------------------
/**
* Returns the number of elements stored in the queue.
*
* @return this queue's size
*/
@Override
public int size() {
return 0;
}

/**
* Returns true if this queue is empty; false otherwise.
*
* @return false
*/
@Override
public boolean isEmpty() {
return false;
Copy link
Member

Choose a reason for hiding this comment

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

@lucas-zimerman @stefanosiano should this return true instead? If no, can we document why?

}

/** Does nothing. */
@Override
public void clear() {}

/**
* Since the queue is disabled, the element will not be added.
*
* @param element the element to add
* @return false, always
*/
@Override
public boolean add(final @NotNull E element) {
return false;
}

// -----------------------------------------------------------------------

/**
* Receives an element but do nothing with it.
*
* @param element the element to add
* @return false, always
*/
@Override
public boolean offer(@NotNull E element) {
return false;
}

@Override
public @Nullable E poll() {
return null;
}

@Override
public @Nullable E element() {
return null;
}

@Override
public @Nullable E peek() {
return null;
}

@Override
public @NotNull E remove() {
throw new NoSuchElementException("queue is disabled");
}

// -----------------------------------------------------------------------

/**
* Returns an iterator over this queue's elements.
*
* @return an iterator over this queue's elements
*/
@Override
public @NotNull Iterator<E> iterator() {
return new Iterator<E>() {

@Override
public boolean hasNext() {
return false;
}

@Override
public E next() {
throw new NoSuchElementException();
}

@Override
public void remove() {
throw new IllegalStateException();
}
};
}
}
4 changes: 3 additions & 1 deletion sentry/src/main/java/io/sentry/Scope.java
Original file line number Diff line number Diff line change
Expand Up @@ -754,7 +754,9 @@ public void clearAttachments() {
* @return the breadcrumbs queue
*/
private @NotNull Queue<Breadcrumb> createBreadcrumbsList(final int maxBreadcrumb) {
return SynchronizedQueue.synchronizedQueue(new CircularFifoQueue<>(maxBreadcrumb));
return maxBreadcrumb > 0
? SynchronizedQueue.synchronizedQueue(new CircularFifoQueue<>(maxBreadcrumb))
: SynchronizedQueue.synchronizedQueue(new DisabledQueue<>());
}

/**
Expand Down
93 changes: 93 additions & 0 deletions sentry/src/test/java/io/sentry/DisabledQueueTest.kt
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
package io.sentry
import org.junit.Assert.assertThrows
import java.util.NoSuchElementException
import kotlin.test.Test
import kotlin.test.assertEquals
import kotlin.test.assertFalse
import kotlin.test.assertNull

class DisabledQueueTest {

@Test
fun `size starts empty`() {
val queue = DisabledQueue<Int>()
assertEquals(0, queue.size, "Size should always be zero.")
}

@Test
fun `add does not add elements`() {
val queue = DisabledQueue<Int>()
assertFalse(queue.add(1), "add should always return false.")
assertEquals(0, queue.size, "Size should still be zero after attempting to add an element.")
}

@Test
fun `isEmpty returns false when created`() {
val queue = DisabledQueue<Int>()
assertFalse(queue.isEmpty(), "isEmpty should always return false.")
}

@Test
fun `isEmpty always returns false if add function was called`() {
val queue = DisabledQueue<Int>()
queue.add(1)

assertFalse(queue.isEmpty(), "isEmpty should always return false.")
}

@Test
fun `offer does not add elements`() {
val queue = DisabledQueue<Int>()
assertFalse(queue.offer(1), "offer should always return false.")
assertEquals(0, queue.size, "Size should still be zero after attempting to offer an element.")
}

@Test
fun `poll returns null`() {
val queue = DisabledQueue<Int>()
queue.add(1)
assertNull(queue.poll(), "poll should always return null.")
}

@Test
fun `peek returns null`() {
val queue = DisabledQueue<Int>()
queue.add(1)

assertNull(queue.peek(), "peek should always return null.")
}

@Test
fun `element returns null`() {
val queue = DisabledQueue<Int>()
assertNull(queue.element(), "element should always return null.")
}

@Test
fun `remove throws NoSuchElementException`() {
val queue = DisabledQueue<Int>()
assertThrows(NoSuchElementException::class.java) { queue.remove() }
}

@Test
fun `clear does nothing`() {
val queue = DisabledQueue<Int>()
queue.clear() // Should not throw an exception
assertEquals(0, queue.size, "Size should remain zero after clear.")
}

@Test
fun `iterator has no elements`() {
val queue = DisabledQueue<Int>()
val iterator = queue.iterator()
assertFalse(iterator.hasNext(), "Iterator should have no elements.")
assertThrows(NoSuchElementException::class.java) { iterator.next() }
}

@Test
fun `iterator remove throws IllegalStateException`() {
val queue = DisabledQueue<Int>()
val iterator = queue.iterator()
assertThrows(IllegalStateException::class.java) { iterator.remove() }
}
}
11 changes: 11 additions & 0 deletions sentry/src/test/java/io/sentry/ScopeTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -1029,6 +1029,17 @@ class ScopeTest {
)
}

@Test
fun `creating a new scope won't crash if max breadcrumbs is set to zero`() {
val options = SentryOptions().apply {
maxBreadcrumbs = 0
}
val scope = Scope(options)

// expect no exception to be thrown
// previously was crashing, see https://github.com/getsentry/sentry-java/issues/3313
}

private fun eventProcessor(): EventProcessor {
return object : EventProcessor {
override fun process(event: SentryEvent, hint: Hint): SentryEvent? {
Expand Down
Loading