From d282d06f5b75971bdc3ee3e4c5d01ff5438d7b71 Mon Sep 17 00:00:00 2001 From: Lucas Date: Wed, 30 Oct 2024 20:31:56 +0000 Subject: [PATCH 1/6] expose max-breadcrumbs on meta data and implement disabled queue when maxbreadcrumbs sets to 0 --- CHANGELOG.md | 11 ++++++++ .../android/core/ManifestMetadataReader.java | 9 +++++++ .../core/ManifestMetadataReaderTest.kt | 26 +++++++++++++++++++ .../src/main/AndroidManifest.xml | 3 +++ sentry/src/main/java/io/sentry/Scope.java | 4 ++- sentry/src/test/java/io/sentry/ScopeTest.kt | 11 ++++++++ 6 files changed, 63 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c6d8bdfc83..a1e2da54ec 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,16 @@ # Changelog +## Unreleased + +### Features + +- Add meta option to set the maximum amount of breadcrumbs to be logged. ([#?](https://github.com/getsentry/sentry-java/pull/?)) + + +### Fixes + +- using MaxBreadcrumb with value 0 no longer crashes. ([#?](https://github.com/getsentry/sentry-java/pull/?)) + ## 7.16.0 ### Features 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 babcfdfc98..47d8629b07 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,13 @@ 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 25b2e0191c..235c5d75df 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 @@ -1515,4 +1515,30 @@ class ManifestMetadataReaderTest { assertTrue(fixture.options.experimental.sessionReplay.maskViewClasses.contains(SentryReplayOptions.IMAGE_VIEW_CLASS_NAME)) assertTrue(fixture.options.experimental.sessionReplay.maskViewClasses.contains(SentryReplayOptions.TEXT_VIEW_CLASS_NAME)) } + + @Test + fun `applyMetadata reads maxbreadcrumb mask flags 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 max breadcrumb mask flags 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) + } + } diff --git a/sentry-samples/sentry-samples-android/src/main/AndroidManifest.xml b/sentry-samples/sentry-samples-android/src/main/AndroidManifest.xml index 058ad3710c..668f877670 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/Scope.java b/sentry/src/main/java/io/sentry/Scope.java index f071cb8c5e..344c4b48fe 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/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? { From 4371e957d27e75f840718264ccf4cc6792c8a6b7 Mon Sep 17 00:00:00 2001 From: Lucas Date: Wed, 30 Oct 2024 20:32:18 +0000 Subject: [PATCH 2/6] missing queue class and test --- .../main/java/io/sentry/DisabledQueue.java | 100 ++++++++++++++++++ .../test/java/io/sentry/DisabledQueueTest.kt | 93 ++++++++++++++++ 2 files changed, 193 insertions(+) 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/sentry/src/main/java/io/sentry/DisabledQueue.java b/sentry/src/main/java/io/sentry/DisabledQueue.java new file mode 100644 index 0000000000..83f0f50e1a --- /dev/null +++ b/sentry/src/main/java/io/sentry/DisabledQueue.java @@ -0,0 +1,100 @@ +package io.sentry; +import org.jetbrains.annotations.NotNull; +import org.jetbrains.annotations.Nullable; +import java.io.Serializable; +import java.util.AbstractCollection; +import java.util.Iterator; +import java.util.NoSuchElementException; +import java.util.Queue; + +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/test/java/io/sentry/DisabledQueueTest.kt b/sentry/src/test/java/io/sentry/DisabledQueueTest.kt new file mode 100644 index 0000000000..408a8baaaf --- /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() } + } +} From 3e51f6cb185719486d982eadb28600737566a982 Mon Sep 17 00:00:00 2001 From: Lucas Date: Wed, 30 Oct 2024 20:45:08 +0000 Subject: [PATCH 3/6] update changelog --- CHANGELOG.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cbd76f0038..30e6869603 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,11 +4,11 @@ ### Features -- Add meta option to set the maximum amount of breadcrumbs to be logged. ([#?](https://github.com/getsentry/sentry-java/pull/?)) +- Add meta option to set the maximum amount of breadcrumbs to be logged. ([#3836](https://github.com/getsentry/sentry-java/pull/3836)) ### Fixes -- using MaxBreadcrumb with value 0 no longer crashes. ([#?](https://github.com/getsentry/sentry-java/pull/?)) +- 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 From 5f9ef0fbfbaafefa0d5fe40aafd0ec2b2e266c59 Mon Sep 17 00:00:00 2001 From: Lucas Date: Wed, 30 Oct 2024 20:50:39 +0000 Subject: [PATCH 4/6] ./gradlew :spotlessApply --- .../android/core/ManifestMetadataReader.java | 6 +-- .../main/java/io/sentry/DisabledQueue.java | 43 +++++++++++++------ sentry/src/main/java/io/sentry/Scope.java | 6 +-- .../test/java/io/sentry/DisabledQueueTest.kt | 6 +-- 4 files changed, 36 insertions(+), 25 deletions(-) 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 43ff665816..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 @@ -216,11 +216,7 @@ static void applyMetadata( options.getSessionTrackingIntervalMillis())); options.setMaxBreadcrumbs( - (int)readLong( - metadata, - logger, - MAX_BREADCRUMBS, - options.getMaxBreadcrumbs())); + (int) readLong(metadata, logger, MAX_BREADCRUMBS, options.getMaxBreadcrumbs())); options.setEnableActivityLifecycleBreadcrumbs( readBool( diff --git a/sentry/src/main/java/io/sentry/DisabledQueue.java b/sentry/src/main/java/io/sentry/DisabledQueue.java index 83f0f50e1a..afef111af4 100644 --- a/sentry/src/main/java/io/sentry/DisabledQueue.java +++ b/sentry/src/main/java/io/sentry/DisabledQueue.java @@ -1,19 +1,20 @@ package io.sentry; -import org.jetbrains.annotations.NotNull; -import org.jetbrains.annotations.Nullable; + 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 { +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() { } + public DisabledQueue() {} // ----------------------------------------------------------------------- /** @@ -22,7 +23,9 @@ public DisabledQueue() { } * @return this queue's size */ @Override - public int size() { return 0; } + public int size() { + return 0; + } /** * Returns true if this queue is empty; false otherwise. @@ -30,11 +33,13 @@ public DisabledQueue() { } * @return false */ @Override - public boolean isEmpty() { return false; } + public boolean isEmpty() { + return false; + } /** Does nothing. */ @Override - public void clear() { } + public void clear() {} /** * Since the queue is disabled, the element will not be added. @@ -43,7 +48,9 @@ public void clear() { } * @return false, always */ @Override - public boolean add(final @NotNull E element) { return false; } + public boolean add(final @NotNull E element) { + return false; + } // ----------------------------------------------------------------------- @@ -59,16 +66,24 @@ public boolean offer(@NotNull E element) { } @Override - public @Nullable E poll() { return null; } + public @Nullable E poll() { + return null; + } @Override - public @Nullable E element() { return null; } + public @Nullable E element() { + return null; + } @Override - public @Nullable E peek() { return null; } + public @Nullable E peek() { + return null; + } @Override - public @NotNull E remove() { throw new NoSuchElementException("queue is disabled"); } + public @NotNull E remove() { + throw new NoSuchElementException("queue is disabled"); + } // ----------------------------------------------------------------------- @@ -93,8 +108,8 @@ public E next() { @Override public void remove() { - throw new IllegalStateException(); - } + throw new IllegalStateException(); + } }; } } diff --git a/sentry/src/main/java/io/sentry/Scope.java b/sentry/src/main/java/io/sentry/Scope.java index 344c4b48fe..c74fabce25 100644 --- a/sentry/src/main/java/io/sentry/Scope.java +++ b/sentry/src/main/java/io/sentry/Scope.java @@ -754,9 +754,9 @@ public void clearAttachments() { * @return the breadcrumbs queue */ private @NotNull Queue createBreadcrumbsList(final int maxBreadcrumb) { - return maxBreadcrumb > 0 ? - SynchronizedQueue.synchronizedQueue(new CircularFifoQueue<>(maxBreadcrumb)) : - SynchronizedQueue.synchronizedQueue(new DisabledQueue<>()); + 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 index 408a8baaaf..351f87eaff 100644 --- a/sentry/src/test/java/io/sentry/DisabledQueueTest.kt +++ b/sentry/src/test/java/io/sentry/DisabledQueueTest.kt @@ -30,7 +30,7 @@ class DisabledQueueTest { @Test fun `isEmpty always returns false if add function was called`() { val queue = DisabledQueue() - queue.add(1); + queue.add(1) assertFalse(queue.isEmpty(), "isEmpty should always return false.") } @@ -45,14 +45,14 @@ class DisabledQueueTest { @Test fun `poll returns null`() { val queue = DisabledQueue() - queue.add(1); + queue.add(1) assertNull(queue.poll(), "poll should always return null.") } @Test fun `peek returns null`() { val queue = DisabledQueue() - queue.add(1); + queue.add(1) assertNull(queue.peek(), "peek should always return null.") } From d581e91738d4cbe1867f779117f908143a6000f8 Mon Sep 17 00:00:00 2001 From: LucasZF Date: Fri, 1 Nov 2024 11:20:17 +0000 Subject: [PATCH 5/6] Update sentry-android-core/src/test/java/io/sentry/android/core/ManifestMetadataReaderTest.kt Co-authored-by: Stefano --- .../java/io/sentry/android/core/ManifestMetadataReaderTest.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 83b2c3cf6d..d4bd7ad4a2 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 @@ -1530,7 +1530,7 @@ class ManifestMetadataReaderTest { } @Test - fun `applyMetadata reads max breadcrumb mask flags to options and keeps default if not found`() { + fun `applyMetadata reads maxBreadcrumbs to options and keeps default if not found`() { // Arrange val context = fixture.getContext() From d0fa3b8f830ec1220203c6168dbc2c6f7a81dc5b Mon Sep 17 00:00:00 2001 From: LucasZF Date: Fri, 1 Nov 2024 11:20:23 +0000 Subject: [PATCH 6/6] Update sentry-android-core/src/test/java/io/sentry/android/core/ManifestMetadataReaderTest.kt Co-authored-by: Stefano --- .../java/io/sentry/android/core/ManifestMetadataReaderTest.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 d4bd7ad4a2..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 @@ -1517,7 +1517,7 @@ class ManifestMetadataReaderTest { } @Test - fun `applyMetadata reads maxbreadcrumb mask flags to options and sets the value if found`() { + 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)