From 2af8d1ae1af717314bb479e07d163f6461205387 Mon Sep 17 00:00:00 2001 From: LucasZF Date: Mon, 4 Nov 2024 14:54:06 +0000 Subject: [PATCH] Fix: Allow MaxBreadcrumb 0 / Expose MaxBreadcrumb metadata. (#3836) * expose max-breadcrumbs on meta data and implement disabled queue when maxbreadcrumbs sets to 0 * missing queue class and test * update changelog --------- Co-authored-by: Lucas Co-authored-by: Stefano --- CHANGELOG.md | 2 + .../android/core/ManifestMetadataReader.java | 5 + .../core/ManifestMetadataReaderTest.kt | 25 ++++ .../src/main/AndroidManifest.xml | 3 + .../main/java/io/sentry/DisabledQueue.java | 115 ++++++++++++++++++ sentry/src/main/java/io/sentry/Scope.java | 4 +- .../test/java/io/sentry/DisabledQueueTest.kt | 93 ++++++++++++++ sentry/src/test/java/io/sentry/ScopeTest.kt | 11 ++ 8 files changed, 257 insertions(+), 1 deletion(-) create mode 100644 sentry/src/main/java/io/sentry/DisabledQueue.java create mode 100644 sentry/src/test/java/io/sentry/DisabledQueueTest.kt diff --git a/CHANGELOG.md b/CHANGELOG.md index cd68c3158e..da49a05efd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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 diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/ManifestMetadataReader.java b/sentry-android-core/src/main/java/io/sentry/android/core/ManifestMetadataReader.java index adfd4f22ad..eb60c5d9c4 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/ManifestMetadataReader.java +++ b/sentry-android-core/src/main/java/io/sentry/android/core/ManifestMetadataReader.java @@ -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"; @@ -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, diff --git a/sentry-android-core/src/test/java/io/sentry/android/core/ManifestMetadataReaderTest.kt b/sentry-android-core/src/test/java/io/sentry/android/core/ManifestMetadataReaderTest.kt index 17f0f3950b..ee4b4ae39a 100644 --- a/sentry-android-core/src/test/java/io/sentry/android/core/ManifestMetadataReaderTest.kt +++ b/sentry-android-core/src/test/java/io/sentry/android/core/ManifestMetadataReaderTest.kt @@ -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 diff --git a/sentry-samples/sentry-samples-android/src/main/AndroidManifest.xml b/sentry-samples/sentry-samples-android/src/main/AndroidManifest.xml index 2327573a43..d8ae6c709d 100644 --- a/sentry-samples/sentry-samples-android/src/main/AndroidManifest.xml +++ b/sentry-samples/sentry-samples-android/src/main/AndroidManifest.xml @@ -152,6 +152,9 @@ + + + diff --git a/sentry/src/main/java/io/sentry/DisabledQueue.java b/sentry/src/main/java/io/sentry/DisabledQueue.java new file mode 100644 index 0000000000..afef111af4 --- /dev/null +++ b/sentry/src/main/java/io/sentry/DisabledQueue.java @@ -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 extends AbstractCollection implements Queue, 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; + } + + /** 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 iterator() { + return new Iterator() { + + @Override + public boolean hasNext() { + return false; + } + + @Override + public E next() { + throw new NoSuchElementException(); + } + + @Override + public void remove() { + throw new IllegalStateException(); + } + }; + } +} diff --git a/sentry/src/main/java/io/sentry/Scope.java b/sentry/src/main/java/io/sentry/Scope.java index f071cb8c5e..c74fabce25 100644 --- a/sentry/src/main/java/io/sentry/Scope.java +++ b/sentry/src/main/java/io/sentry/Scope.java @@ -754,7 +754,9 @@ public void clearAttachments() { * @return the breadcrumbs queue */ private @NotNull Queue createBreadcrumbsList(final int maxBreadcrumb) { - return SynchronizedQueue.synchronizedQueue(new CircularFifoQueue<>(maxBreadcrumb)); + return maxBreadcrumb > 0 + ? SynchronizedQueue.synchronizedQueue(new CircularFifoQueue<>(maxBreadcrumb)) + : SynchronizedQueue.synchronizedQueue(new DisabledQueue<>()); } /** diff --git a/sentry/src/test/java/io/sentry/DisabledQueueTest.kt b/sentry/src/test/java/io/sentry/DisabledQueueTest.kt new file mode 100644 index 0000000000..351f87eaff --- /dev/null +++ b/sentry/src/test/java/io/sentry/DisabledQueueTest.kt @@ -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() + assertEquals(0, queue.size, "Size should always be zero.") + } + + @Test + fun `add does not add elements`() { + val queue = DisabledQueue() + 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() + assertFalse(queue.isEmpty(), "isEmpty should always return false.") + } + + @Test + fun `isEmpty always returns false if add function was called`() { + val queue = DisabledQueue() + queue.add(1) + + assertFalse(queue.isEmpty(), "isEmpty should always return false.") + } + + @Test + fun `offer does not add elements`() { + val queue = DisabledQueue() + 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() + queue.add(1) + assertNull(queue.poll(), "poll should always return null.") + } + + @Test + fun `peek returns null`() { + val queue = DisabledQueue() + queue.add(1) + + assertNull(queue.peek(), "peek should always return null.") + } + + @Test + fun `element returns null`() { + val queue = DisabledQueue() + assertNull(queue.element(), "element should always return null.") + } + + @Test + fun `remove throws NoSuchElementException`() { + val queue = DisabledQueue() + assertThrows(NoSuchElementException::class.java) { queue.remove() } + } + + @Test + fun `clear does nothing`() { + val queue = DisabledQueue() + 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() + 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() + val iterator = queue.iterator() + assertThrows(IllegalStateException::class.java) { iterator.remove() } + } +} diff --git a/sentry/src/test/java/io/sentry/ScopeTest.kt b/sentry/src/test/java/io/sentry/ScopeTest.kt index 07b9176de7..a0f54d5205 100644 --- a/sentry/src/test/java/io/sentry/ScopeTest.kt +++ b/sentry/src/test/java/io/sentry/ScopeTest.kt @@ -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? {