diff --git a/CHANGELOG.md b/CHANGELOG.md index cfd2e8d0f78..4d67fb265ee 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,8 @@ ### Fixes +- Avoid a CPU busy-loop when recording discarded log or metric envelopes under rate limiting ([#5835](https://github.com/getsentry/sentry-java/pull/5835)) + - `ClientReportRecorder` now reads the item count from the envelope item header instead of deserializing the payload, which under sustained rate limiting could pin CPU cores while repeatedly throwing exceptions - Release `MediaMuxer` when the replay video encoder fails to start to avoid a resource leak ([#5607](https://github.com/getsentry/sentry-java/pull/5607)) ### Performance diff --git a/sentry/api/sentry.api b/sentry/api/sentry.api index c623e71d08f..8f9e9a1b01c 100644 --- a/sentry/api/sentry.api +++ b/sentry/api/sentry.api @@ -3112,6 +3112,7 @@ public final class io/sentry/SentryEnvelopeItemHeader : io/sentry/JsonSerializab public fun getAttachmentType ()Ljava/lang/String; public fun getContentType ()Ljava/lang/String; public fun getFileName ()Ljava/lang/String; + public fun getItemCount ()Ljava/lang/Integer; public fun getLength ()I public fun getPlatform ()Ljava/lang/String; public fun getType ()Lio/sentry/SentryItemType; diff --git a/sentry/src/main/java/io/sentry/SentryEnvelopeItemHeader.java b/sentry/src/main/java/io/sentry/SentryEnvelopeItemHeader.java index 25f1e893fc3..23b3afc2a27 100644 --- a/sentry/src/main/java/io/sentry/SentryEnvelopeItemHeader.java +++ b/sentry/src/main/java/io/sentry/SentryEnvelopeItemHeader.java @@ -52,6 +52,10 @@ public int getLength() { return platform; } + public @Nullable Integer getItemCount() { + return itemCount; + } + @ApiStatus.Internal public SentryEnvelopeItemHeader( final @NotNull SentryItemType type, diff --git a/sentry/src/main/java/io/sentry/clientreport/ClientReportRecorder.java b/sentry/src/main/java/io/sentry/clientreport/ClientReportRecorder.java index b3756ed32e4..ccd7c5dc402 100644 --- a/sentry/src/main/java/io/sentry/clientreport/ClientReportRecorder.java +++ b/sentry/src/main/java/io/sentry/clientreport/ClientReportRecorder.java @@ -6,10 +6,6 @@ import io.sentry.SentryEnvelopeItem; import io.sentry.SentryItemType; import io.sentry.SentryLevel; -import io.sentry.SentryLogEvent; -import io.sentry.SentryLogEvents; -import io.sentry.SentryMetricsEvent; -import io.sentry.SentryMetricsEvents; import io.sentry.SentryOptions; import io.sentry.protocol.SentrySpan; import io.sentry.protocol.SentryTransaction; @@ -105,34 +101,18 @@ public void recordLostEnvelopeItem( recordLostEventInternal(reason.getReason(), itemCategory.getCategory(), 1L); executeOnDiscard(reason, itemCategory, 1L); } else if (itemCategory.equals(DataCategory.LogItem)) { - final @Nullable SentryLogEvents logs = envelopeItem.getLogs(options.getSerializer()); - if (logs != null) { - final @NotNull List items = logs.getItems(); - final long count = items.size(); - recordLostEventInternal(reason.getReason(), itemCategory.getCategory(), count); - final long logBytes = envelopeItem.getData().length; - recordLostEventInternal( - reason.getReason(), DataCategory.LogByte.getCategory(), logBytes); - executeOnDiscard(reason, itemCategory, count); - } else { - options.getLogger().log(SentryLevel.ERROR, "Unable to parse lost logs envelope item."); - } + final long count = itemCountFromHeader(envelopeItem); + recordLostEventInternal(reason.getReason(), itemCategory.getCategory(), count); + final long logBytes = envelopeItem.getData().length; + recordLostEventInternal(reason.getReason(), DataCategory.LogByte.getCategory(), logBytes); + executeOnDiscard(reason, itemCategory, count); } else if (itemCategory.equals(DataCategory.TraceMetric)) { - final @Nullable SentryMetricsEvents metrics = - envelopeItem.getMetrics(options.getSerializer()); - if (metrics != null) { - final @NotNull List items = metrics.getItems(); - final long count = items.size(); - recordLostEventInternal(reason.getReason(), itemCategory.getCategory(), count); - final long metricBytes = envelopeItem.getData().length; - recordLostEventInternal( - reason.getReason(), DataCategory.TraceMetricByte.getCategory(), metricBytes); - executeOnDiscard(reason, itemCategory, count); - } else { - options - .getLogger() - .log(SentryLevel.ERROR, "Unable to parse lost metrics envelope item."); - } + final long count = itemCountFromHeader(envelopeItem); + recordLostEventInternal(reason.getReason(), itemCategory.getCategory(), count); + final long metricBytes = envelopeItem.getData().length; + recordLostEventInternal( + reason.getReason(), DataCategory.TraceMetricByte.getCategory(), metricBytes); + executeOnDiscard(reason, itemCategory, count); } else { recordLostEventInternal(reason.getReason(), itemCategory.getCategory(), 1L); executeOnDiscard(reason, itemCategory, 1L); @@ -176,6 +156,14 @@ private void recordLostEventInternal( storage.addCount(key, countToAdd); } + // The number of items batched into a log or metric envelope item is stored in its header, so we + // read it from there instead of deserializing the payload. Deserializing on the discard path is + // expensive and, under sustained rate limiting, caused a CPU busy-loop (JAVA-662). + private long itemCountFromHeader(final @NotNull SentryEnvelopeItem envelopeItem) { + final @Nullable Integer itemCount = envelopeItem.getHeader().getItemCount(); + return itemCount != null ? itemCount : 1L; + } + @Nullable ClientReport resetCountsAndGenerateClientReport() { final Date currentDate = DateUtils.getCurrentDateTime(); diff --git a/sentry/src/test/java/io/sentry/clientreport/ClientReportTest.kt b/sentry/src/test/java/io/sentry/clientreport/ClientReportTest.kt index ceb254213cb..2700e4899ed 100644 --- a/sentry/src/test/java/io/sentry/clientreport/ClientReportTest.kt +++ b/sentry/src/test/java/io/sentry/clientreport/ClientReportTest.kt @@ -15,7 +15,9 @@ import io.sentry.Sentry import io.sentry.SentryEnvelope import io.sentry.SentryEnvelopeHeader import io.sentry.SentryEnvelopeItem +import io.sentry.SentryEnvelopeItemHeader import io.sentry.SentryEvent +import io.sentry.SentryItemType import io.sentry.SentryLogEvent import io.sentry.SentryLogEvents import io.sentry.SentryLogLevel @@ -46,7 +48,10 @@ import java.util.UUID import kotlin.test.Test import kotlin.test.assertEquals import kotlin.test.assertTrue +import org.mockito.kotlin.any +import org.mockito.kotlin.doReturn import org.mockito.kotlin.mock +import org.mockito.kotlin.never import org.mockito.kotlin.times import org.mockito.kotlin.verify import org.mockito.kotlin.whenever @@ -417,6 +422,82 @@ class ClientReportTest { assertEquals(envelope.items.first().data.size.toLong(), metricByteItem.quantity) } + @Test + fun `recording lost log item reads count from the header without deserializing the payload`() { + val onDiscardMock = mock() + givenClientReportRecorder { options -> options.onDiscard = onDiscardMock } + + // A payload that would throw if deserialized; the count must come from the header instead. + val brokenPayload = "not valid json".toByteArray() + val itemHeader = + SentryEnvelopeItemHeader( + SentryItemType.Log, + 0, + "application/vnd.sentry.items.log+json", + null, + null, + null, + 5, + ) + val item = + mock { + on { it.header } doReturn itemHeader + on { it.data } doReturn brokenPayload + } + + clientReportRecorder.recordLostEnvelopeItem(DiscardReason.NETWORK_ERROR, item) + + verify(item, never()).getLogs(any()) + verify(onDiscardMock, times(1)).execute(DiscardReason.NETWORK_ERROR, DataCategory.LogItem, 5) + + val clientReport = clientReportRecorder.resetCountsAndGenerateClientReport() + assertEquals( + 5, + clientReport!! + .discardedEvents!! + .first { it.category == DataCategory.LogItem.category } + .quantity, + ) + assertEquals( + brokenPayload.size.toLong(), + clientReport.discardedEvents!! + .first { it.category == DataCategory.LogByte.category } + .quantity, + ) + } + + @Test + fun `recording lost log item without item count in header falls back to one`() { + givenClientReportRecorder() + + val itemHeader = + SentryEnvelopeItemHeader( + SentryItemType.Log, + 0, + "application/vnd.sentry.items.log+json", + null, + null, + null, + null, + ) + val item = + mock { + on { it.header } doReturn itemHeader + on { it.data } doReturn ByteArray(0) + } + + clientReportRecorder.recordLostEnvelopeItem(DiscardReason.NETWORK_ERROR, item) + + val clientReport = clientReportRecorder.resetCountsAndGenerateClientReport() + assertEquals( + 1, + clientReport!! + .discardedEvents!! + .first { it.category == DataCategory.LogItem.category } + .quantity, + ) + } + private fun givenClientReportRecorder( callback: Sentry.OptionsConfiguration? = null ) {