diff --git a/CHANGELOG.md b/CHANGELOG.md index a35cc1194e..9b1d6b2cb8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,6 +25,7 @@ * Fix: Set release and environment on Transactions (#1152) * Fix: Do not set transaction on the scope automatically * Enhancement: Automatically assign span context to captured events (#1156) +* Feat: OutboxSender supports all envelope item types #1158 * Enhancement: Improve ITransaction and ISpan null-safety compatibility (#1161) # 4.0.0-alpha.2 diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/EnvelopeFileObserver.java b/sentry-android-core/src/main/java/io/sentry/android/core/EnvelopeFileObserver.java index b7b2d2abee..cd27f074ac 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/EnvelopeFileObserver.java +++ b/sentry-android-core/src/main/java/io/sentry/android/core/EnvelopeFileObserver.java @@ -9,6 +9,7 @@ import io.sentry.hints.ApplyScopeData; import io.sentry.hints.Cached; import io.sentry.hints.Flushable; +import io.sentry.hints.Resettable; import io.sentry.hints.Retryable; import io.sentry.hints.SubmissionResult; import io.sentry.util.Objects; @@ -59,17 +60,17 @@ public void onEvent(int eventType, @Nullable String relativePath) { } private static final class CachedEnvelopeHint - implements Cached, Retryable, SubmissionResult, Flushable, ApplyScopeData { - boolean retry = false; - boolean succeeded = false; + implements Cached, Retryable, SubmissionResult, Flushable, ApplyScopeData, Resettable { + boolean retry; + boolean succeeded; - private @NotNull final CountDownLatch latch; + private @NotNull CountDownLatch latch; private final long flushTimeoutMillis; private final @NotNull ILogger logger; public CachedEnvelopeHint(final long flushTimeoutMillis, final @NotNull ILogger logger) { + reset(); this.flushTimeoutMillis = flushTimeoutMillis; - this.latch = new CountDownLatch(1); this.logger = Objects.requireNonNull(logger, "ILogger is required."); } @@ -104,5 +105,12 @@ public void setResult(boolean succeeded) { public boolean isSuccess() { return succeeded; } + + @Override + public void reset() { + latch = new CountDownLatch(1); + retry = false; + succeeded = false; + } } } diff --git a/sentry-android-core/src/test/java/io/sentry/android/core/EnvelopeFileObserverTest.kt b/sentry-android-core/src/test/java/io/sentry/android/core/EnvelopeFileObserverTest.kt index d0ecd315b3..b2d46c4107 100644 --- a/sentry-android-core/src/test/java/io/sentry/android/core/EnvelopeFileObserverTest.kt +++ b/sentry-android-core/src/test/java/io/sentry/android/core/EnvelopeFileObserverTest.kt @@ -4,7 +4,7 @@ import android.os.FileObserver import androidx.test.ext.junit.runners.AndroidJUnit4 import com.nhaarman.mockitokotlin2.any import com.nhaarman.mockitokotlin2.anyOrNull -import com.nhaarman.mockitokotlin2.argWhere +import com.nhaarman.mockitokotlin2.check import com.nhaarman.mockitokotlin2.eq import com.nhaarman.mockitokotlin2.mock import com.nhaarman.mockitokotlin2.never @@ -14,28 +14,31 @@ import io.sentry.IEnvelopeSender import io.sentry.ILogger import io.sentry.SentryOptions import io.sentry.hints.ApplyScopeData +import io.sentry.hints.Resettable +import io.sentry.hints.Retryable +import io.sentry.hints.SubmissionResult import java.io.File import kotlin.test.Test import kotlin.test.assertEquals import kotlin.test.assertFailsWith +import kotlin.test.assertFalse import org.junit.runner.RunWith @RunWith(AndroidJUnit4::class) class EnvelopeFileObserverTest { private class Fixture { + val fileName = "file-name.txt" var path: String? = "." - var envelopeSender: IEnvelopeSender = mock() - var logger: ILogger = mock() - var options: SentryOptions = SentryOptions() - - init { - options.isDebug = true - options.setLogger(logger) + val envelopeSender = mock() + val logger = mock() + val options = SentryOptions().apply { + isDebug = true + setLogger(logger) } - fun getSut(): EnvelopeFileObserver { - return EnvelopeFileObserver(path, envelopeSender, logger, options.flushTimeoutMillis) + fun getSut(flushTimeoutMillis: Long): EnvelopeFileObserver { + return EnvelopeFileObserver(path, envelopeSender, logger, flushTimeoutMillis) } } @@ -43,38 +46,73 @@ class EnvelopeFileObserverTest { @Test fun `envelope sender is called with fully qualified path`() { - val sut = fixture.getSut() - val param = "file-name.txt" - sut.onEvent(FileObserver.CLOSE_WRITE, param) - verify(fixture.envelopeSender).processEnvelopeFile(eq(fixture.path + File.separator + param), any()) + triggerEvent() + + verify(fixture.envelopeSender).processEnvelopeFile(eq(fixture.path + File.separator + fixture.fileName), any()) } @Test fun `when event type is not close write, envelope sender is not called`() { - val sut = fixture.getSut() - sut.onEvent(FileObserver.CLOSE_WRITE.inv(), "file-name.txt") + triggerEvent(eventType = FileObserver.CLOSE_WRITE.inv()) + verifyZeroInteractions(fixture.envelopeSender) } @Test fun `when event is fired with null path, envelope reader is not called`() { - val sut = fixture.getSut() - sut.onEvent(0, null) + triggerEvent(relativePath = null) + verify(fixture.envelopeSender, never()).processEnvelopeFile(anyOrNull(), any()) } @Test fun `when null is passed as a path, ctor throws`() { fixture.path = null - val exception = assertFailsWith { fixture.getSut() } + val exception = assertFailsWith { fixture.getSut(0) } assertEquals("File path is required.", exception.message) } @Test fun `envelope sender is called with fully qualified path and ApplyScopeData hint`() { - val sut = fixture.getSut() - val param = "file-name.txt" - sut.onEvent(FileObserver.CLOSE_WRITE, param) - verify(fixture.envelopeSender).processEnvelopeFile(eq(fixture.path + File.separator + param), argWhere { it is ApplyScopeData }) + triggerEvent() + + verify(fixture.envelopeSender).processEnvelopeFile( + eq(fixture.path + File.separator + fixture.fileName), + check { it is ApplyScopeData }) + } + + @Test + fun `envelope sender Hint is Resettable`() { + triggerEvent() + + verify(fixture.envelopeSender).processEnvelopeFile( + eq(fixture.path + File.separator + fixture.fileName), + check { it is Resettable }) + } + + @Test + fun `Hint resets its state`() { + triggerEvent(flushTimeoutMillis = 0) + + verify(fixture.envelopeSender).processEnvelopeFile( + eq(fixture.path + File.separator + fixture.fileName), + check { + (it as SubmissionResult).setResult(true) + (it as Retryable).isRetry = true + + (it as Resettable).reset() + + assertFalse(it.isRetry) + assertFalse(it.isSuccess) + }) + } + + private fun triggerEvent( + flushTimeoutMillis: Long = 15_000, + eventType: Int = FileObserver.CLOSE_WRITE, + relativePath: String? = fixture.fileName + ) { + val sut = fixture.getSut(flushTimeoutMillis) + sut.onEvent(eventType, relativePath) } } diff --git a/sentry/api/sentry.api b/sentry/api/sentry.api index b06aba9841..00e9bb5c90 100644 --- a/sentry/api/sentry.api +++ b/sentry/api/sentry.api @@ -1144,6 +1144,10 @@ public abstract interface class io/sentry/hints/Flushable { public abstract fun waitFlush ()Z } +public abstract interface class io/sentry/hints/Resettable { + public abstract fun reset ()V +} + public abstract interface class io/sentry/hints/Retryable { public abstract fun isRetry ()Z public abstract fun setRetry (Z)V diff --git a/sentry/src/main/java/io/sentry/OutboxSender.java b/sentry/src/main/java/io/sentry/OutboxSender.java index b715b55740..c3394bfa43 100644 --- a/sentry/src/main/java/io/sentry/OutboxSender.java +++ b/sentry/src/main/java/io/sentry/OutboxSender.java @@ -4,6 +4,7 @@ import static io.sentry.cache.EnvelopeCache.PREFIX_CURRENT_SESSION_FILE; import io.sentry.hints.Flushable; +import io.sentry.hints.Resettable; import io.sentry.hints.Retryable; import io.sentry.hints.SubmissionResult; import io.sentry.util.CollectionUtils; @@ -107,6 +108,7 @@ private void processEnvelope(final @NotNull SentryEnvelope envelope, final @Null "Processing Envelope with %d item(s)", CollectionUtils.size(envelope.getItems())); int items = 0; + for (final SentryEnvelopeItem item : envelope.getItems()) { items++; @@ -138,26 +140,37 @@ private void processEnvelope(final @NotNull SentryEnvelope envelope, final @Null } hub.captureEvent(event, hint); logger.log(SentryLevel.DEBUG, "Item %d is being captured.", items); - if (hint instanceof Flushable) { - if (!((Flushable) hint).waitFlush()) { - logger.log( - SentryLevel.WARNING, - "Timed out waiting for event submission: %s", - event.getEventId()); - - break; - } - } else { - LogUtils.logIfNotFlushable(logger, hint); + + if (!waitFlush(hint)) { + logger.log( + SentryLevel.WARNING, + "Timed out waiting for event submission: %s", + event.getEventId()); + break; } } } catch (Exception e) { logger.log(ERROR, "Item failed to process.", e); } } else { - // TODO: Handle attachments and other types + // send unknown item types over the wire + final SentryEnvelope newEnvelope = + new SentryEnvelope( + envelope.getHeader().getEventId(), envelope.getHeader().getSdkVersion(), item); + hub.captureEnvelope(newEnvelope, hint); logger.log( - SentryLevel.WARNING, "Item %d of type: %s ignored.", items, item.getHeader().getType()); + SentryLevel.DEBUG, + "%s item %d is being captured.", + item.getHeader().getType().getItemType(), + items); + + if (!waitFlush(hint)) { + logger.log( + SentryLevel.WARNING, + "Timed out waiting for item type submission: %s", + item.getHeader().getType().getItemType()); + break; + } } if (hint instanceof SubmissionResult) { @@ -171,6 +184,20 @@ private void processEnvelope(final @NotNull SentryEnvelope envelope, final @Null break; } } + + // reset the Hint to its initial state as we use it multiple times. + if (hint instanceof Resettable) { + ((Resettable) hint).reset(); + } + } + } + + private boolean waitFlush(final @Nullable Object hint) { + if (hint instanceof Flushable) { + return ((Flushable) hint).waitFlush(); + } else { + LogUtils.logIfNotFlushable(logger, hint); } + return true; } } diff --git a/sentry/src/main/java/io/sentry/hints/Resettable.java b/sentry/src/main/java/io/sentry/hints/Resettable.java new file mode 100644 index 0000000000..67de2a9cc5 --- /dev/null +++ b/sentry/src/main/java/io/sentry/hints/Resettable.java @@ -0,0 +1,7 @@ +package io.sentry.hints; + +/** Marker interface for a reusable Hint */ +public interface Resettable { + /** Reset the Hint to its initial state */ + void reset(); +} diff --git a/sentry/src/test/java/io/sentry/OutboxSenderTest.kt b/sentry/src/test/java/io/sentry/OutboxSenderTest.kt index d30569f91b..bd32de92c9 100644 --- a/sentry/src/test/java/io/sentry/OutboxSenderTest.kt +++ b/sentry/src/test/java/io/sentry/OutboxSenderTest.kt @@ -24,17 +24,10 @@ import kotlin.test.assertTrue class OutboxSenderTest { private class Fixture { - var hub: IHub = mock() - var envelopeReader: IEnvelopeReader = mock() - var serializer: ISerializer = mock() - var logger: ILogger = mock() - var options: SentryOptions - - init { - options = SentryOptions() - options.isDebug = true - options.setLogger(logger) - } + val hub = mock() + var envelopeReader = mock() + val serializer = mock() + val logger = mock() fun getSut(): OutboxSender { return OutboxSender(hub, envelopeReader, serializer, logger, 15000) @@ -43,7 +36,7 @@ class OutboxSenderTest { private val fixture = Fixture() - private fun getTempEnvelope(fileName: String): String { + private fun getTempEnvelope(fileName: String = "envelope-event-attachment.txt"): String { val testFile = this::class.java.classLoader.getResource(fileName) val testFileBytes = testFile!!.readBytes() val targetFile = File.createTempFile("temp-envelope", ".tmp") @@ -55,7 +48,7 @@ class OutboxSenderTest { fun `when envelopeReader returns null, file is deleted `() { whenever(fixture.envelopeReader.read(any())).thenReturn(null) val sut = fixture.getSut() - val path = getTempEnvelope("envelope-event-attachment.txt") + val path = getTempEnvelope() assertTrue(File(path).exists()) // sanity check sut.processEnvelopeFile(path, mock()) assertFalse(File(path).exists()) @@ -69,7 +62,7 @@ class OutboxSenderTest { val expected = SentryEvent(SentryId(UUID.fromString("9ec79c33-ec99-42ab-8353-589fcb2e04dc")), Date()) whenever(fixture.serializer.deserialize(any(), eq(SentryEvent::class.java))).thenReturn(expected) val sut = fixture.getSut() - val path = getTempEnvelope("envelope-event-attachment.txt") + val path = getTempEnvelope() assertTrue(File(path).exists()) // sanity check sut.processEnvelopeFile(path, mock()) @@ -89,7 +82,7 @@ class OutboxSenderTest { whenever(fixture.serializer.deserializeEnvelope(any())).thenReturn(expected) whenever(fixture.serializer.deserialize(any(), eq(SentryEvent::class.java))).thenReturn(event) val sut = fixture.getSut() - val path = getTempEnvelope("envelope-event-attachment.txt") + val path = getTempEnvelope() assertTrue(File(path).exists()) // sanity check sut.processEnvelopeFile(path, mock()) @@ -100,12 +93,28 @@ class OutboxSenderTest { verify(fixture.logger, never()).log(eq(SentryLevel.ERROR), any(), any()) } + @Test + fun `when envelope has unknown item type, create and capture an envelope`() { + fixture.envelopeReader = EnvelopeReader() + + val sut = fixture.getSut() + val path = getTempEnvelope(fileName = "envelope_attachment.txt") + assertTrue(File(path).exists()) // sanity check + sut.processEnvelopeFile(path, mock()) + + verify(fixture.hub).captureEnvelope(any(), any()) + assertFalse(File(path).exists()) + // Additionally make sure we have no errors logged + verify(fixture.logger, never()).log(eq(SentryLevel.ERROR), any(), any()) + verify(fixture.logger, never()).log(eq(SentryLevel.ERROR), any(), any()) + } + @Test fun `when parser is EnvelopeReader and serializer returns a null event, file error logged, no event captured `() { fixture.envelopeReader = EnvelopeReader() whenever(fixture.serializer.deserialize(any(), eq(SentryEvent::class.java))).thenReturn(null) val sut = fixture.getSut() - val path = getTempEnvelope("envelope-event-attachment.txt") + val path = getTempEnvelope() assertTrue(File(path).exists()) // sanity check sut.processEnvelopeFile(path, mock()) @@ -121,7 +130,7 @@ class OutboxSenderTest { whenever(fixture.serializer.deserializeEnvelope(any())).thenReturn(null) whenever(fixture.serializer.deserialize(any(), eq(SentryEvent::class.java))).thenReturn(null) val sut = fixture.getSut() - val path = getTempEnvelope("envelope-event-attachment.txt") + val path = getTempEnvelope() assertTrue(File(path).exists()) // sanity check sut.processEnvelopeFile(path, mock()) diff --git a/sentry/src/test/resources/envelope_attachment.txt b/sentry/src/test/resources/envelope_attachment.txt new file mode 100644 index 0000000000..04a6e32325 --- /dev/null +++ b/sentry/src/test/resources/envelope_attachment.txt @@ -0,0 +1,3 @@ +{} +{"type":"attachment","length":61,"filename":"attachment.txt","content_type":"text/plain"} +some plain text attachment file which include two line breaks