From 8a4fd45befe4c17c0bb3f5a17a0c8c4b4faf9cac Mon Sep 17 00:00:00 2001 From: stefanosiano Date: Mon, 8 Apr 2024 12:22:02 +0200 Subject: [PATCH 1/2] add rate limiting to metrics added metric_bucket data category --- sentry/api/sentry.api | 3 + .../src/main/java/io/sentry/DataCategory.java | 1 + .../clientreport/ClientReportRecorder.java | 3 + .../java/io/sentry/transport/RateLimiter.java | 35 +++++++-- .../java/io/sentry/util/CollectionUtils.java | 16 ++++ .../main/java/io/sentry/util/StringUtils.java | 21 +++++ .../sentry/clientreport/ClientReportTest.kt | 7 +- .../io/sentry/transport/RateLimiterTest.kt | 77 ++++++++++++++++++- .../io/sentry/util/CollectionUtilsTest.kt | 17 ++++ .../java/io/sentry/util/StringUtilsTest.kt | 25 ++++++ 10 files changed, 192 insertions(+), 13 deletions(-) diff --git a/sentry/api/sentry.api b/sentry/api/sentry.api index 6d30b9cca7a..8fb64b887af 100644 --- a/sentry/api/sentry.api +++ b/sentry/api/sentry.api @@ -224,6 +224,7 @@ public final class io/sentry/DataCategory : java/lang/Enum { public static final field Attachment Lio/sentry/DataCategory; public static final field Default Lio/sentry/DataCategory; public static final field Error Lio/sentry/DataCategory; + public static final field MetricBucket Lio/sentry/DataCategory; public static final field Monitor Lio/sentry/DataCategory; public static final field Profile Lio/sentry/DataCategory; public static final field Security Lio/sentry/DataCategory; @@ -4925,6 +4926,7 @@ public final class io/sentry/util/ClassLoaderUtils { } public final class io/sentry/util/CollectionUtils { + public static fun contains ([Ljava/lang/Object;Ljava/lang/Object;)Z public static fun filterListEntries (Ljava/util/List;Lio/sentry/util/CollectionUtils$Predicate;)Ljava/util/List; public static fun filterMapEntries (Ljava/util/Map;Lio/sentry/util/CollectionUtils$Predicate;)Ljava/util/Map; public static fun map (Ljava/util/List;Lio/sentry/util/CollectionUtils$Mapper;)Ljava/util/List; @@ -5092,6 +5094,7 @@ public final class io/sentry/util/SampleRateUtils { public final class io/sentry/util/StringUtils { public static fun byteCountToString (J)Ljava/lang/String; public static fun calculateStringHash (Ljava/lang/String;Lio/sentry/ILogger;)Ljava/lang/String; + public static fun camelCase (Ljava/lang/String;)Ljava/lang/String; public static fun capitalize (Ljava/lang/String;)Ljava/lang/String; public static fun countOf (Ljava/lang/String;C)I public static fun getStringAfterDot (Ljava/lang/String;)Ljava/lang/String; diff --git a/sentry/src/main/java/io/sentry/DataCategory.java b/sentry/src/main/java/io/sentry/DataCategory.java index 6b119b43e4c..c3d6520987b 100644 --- a/sentry/src/main/java/io/sentry/DataCategory.java +++ b/sentry/src/main/java/io/sentry/DataCategory.java @@ -12,6 +12,7 @@ public enum DataCategory { Attachment("attachment"), Monitor("monitor"), Profile("profile"), + MetricBucket("metric_bucket"), Transaction("transaction"), Security("security"), UserReport("user_report"), diff --git a/sentry/src/main/java/io/sentry/clientreport/ClientReportRecorder.java b/sentry/src/main/java/io/sentry/clientreport/ClientReportRecorder.java index ed25c007f3c..f51287d7d9f 100644 --- a/sentry/src/main/java/io/sentry/clientreport/ClientReportRecorder.java +++ b/sentry/src/main/java/io/sentry/clientreport/ClientReportRecorder.java @@ -146,6 +146,9 @@ private DataCategory categoryFromItemType(SentryItemType itemType) { if (SentryItemType.Profile.equals(itemType)) { return DataCategory.Profile; } + if (SentryItemType.Statsd.equals(itemType)) { + return DataCategory.MetricBucket; + } if (SentryItemType.Attachment.equals(itemType)) { return DataCategory.Attachment; } diff --git a/sentry/src/main/java/io/sentry/transport/RateLimiter.java b/sentry/src/main/java/io/sentry/transport/RateLimiter.java index 4e9fde8a954..c4598820abf 100644 --- a/sentry/src/main/java/io/sentry/transport/RateLimiter.java +++ b/sentry/src/main/java/io/sentry/transport/RateLimiter.java @@ -12,6 +12,7 @@ import io.sentry.clientreport.DiscardReason; import io.sentry.hints.Retryable; import io.sentry.hints.SubmissionResult; +import io.sentry.util.CollectionUtils; import io.sentry.util.HintUtils; import io.sentry.util.StringUtils; import java.util.ArrayList; @@ -168,6 +169,10 @@ private boolean isRetryAfter(final @NotNull String itemType) { return DataCategory.Attachment; case "profile": return DataCategory.Profile; + // The envelope item type used for metrics is statsd, whereas the client report category is + // metric_bucket + case "statsd": + return DataCategory.MetricBucket; case "transaction": return DataCategory.Transaction; case "check_in": @@ -189,21 +194,25 @@ public void updateRetryAfterLimits( final @Nullable String sentryRateLimitHeader, final @Nullable String retryAfterHeader, final int errorCode) { + // example: 2700:metric_bucket:organization:quota_exceeded:custom,... if (sentryRateLimitHeader != null) { for (String limit : sentryRateLimitHeader.split(",", -1)) { // Java 11 or so has strip() :( limit = limit.replace(" ", ""); - final String[] retryAfterAndCategories = - limit.split(":", -1); // we only need for 1st and 2nd item though. + final String[] rateLimit = limit.split(":", -1); + // These can be ignored by the SDK. + // final String scope = rateLimit.length > 2 ? rateLimit[2] : null; + // final String reasonCode = rateLimit.length > 3 ? rateLimit[3] : null; + final @Nullable String limitNamespaces = rateLimit.length > 4 ? rateLimit[4] : null; - if (retryAfterAndCategories.length > 0) { - final String retryAfter = retryAfterAndCategories[0]; + if (rateLimit.length > 0) { + final String retryAfter = rateLimit[0]; long retryAfterMillis = parseRetryAfterOrDefault(retryAfter); - if (retryAfterAndCategories.length > 1) { - final String allCategories = retryAfterAndCategories[1]; + if (rateLimit.length > 1) { + final String allCategories = rateLimit[1]; // we dont care if Date is UTC as we just add the relative seconds final Date date = @@ -215,7 +224,7 @@ public void updateRetryAfterLimits( for (final String catItem : categories) { DataCategory dataCategory = DataCategory.Unknown; try { - final String catItemCapitalized = StringUtils.capitalize(catItem); + final String catItemCapitalized = StringUtils.camelCase(catItem); if (catItemCapitalized != null) { dataCategory = DataCategory.valueOf(catItemCapitalized); } else { @@ -228,6 +237,18 @@ public void updateRetryAfterLimits( if (DataCategory.Unknown.equals(dataCategory)) { continue; } + // SDK doesn't support namespaces, yet. Namespaces can be returned by relay in case + // of metric_bucket items. If the namespaces are empty or contain "custom" we apply + // the rate limit to all metrics, otherwise to none. + if (DataCategory.MetricBucket.equals(dataCategory) + && limitNamespaces != null + && !limitNamespaces.equals("")) { + final String[] namespaces = limitNamespaces.split(";", -1); + if (namespaces.length > 0 && !CollectionUtils.contains(namespaces, "custom")) { + continue; + } + } + applyRetryAfterOnlyIfLonger(dataCategory, date); } } else { diff --git a/sentry/src/main/java/io/sentry/util/CollectionUtils.java b/sentry/src/main/java/io/sentry/util/CollectionUtils.java index 7dd54ef2bfa..40f91de7d84 100644 --- a/sentry/src/main/java/io/sentry/util/CollectionUtils.java +++ b/sentry/src/main/java/io/sentry/util/CollectionUtils.java @@ -145,6 +145,22 @@ public static int size(final @NotNull Iterable data) { return filteredList; } + /** + * Returns true if the element is present in the array, false otherwise. + * + * @param array - the array + * @param element - the element + * @return true if the element is present in the array, false otherwise. + */ + public static boolean contains(final @NotNull T[] array, final @NotNull T element) { + for (T t : array) { + if (t.equals(element)) { + return true; + } + } + return false; + } + /** * A simplified copy of Java 8 Predicate. * diff --git a/sentry/src/main/java/io/sentry/util/StringUtils.java b/sentry/src/main/java/io/sentry/util/StringUtils.java index ac95f9ad70c..11e5ae67cdb 100644 --- a/sentry/src/main/java/io/sentry/util/StringUtils.java +++ b/sentry/src/main/java/io/sentry/util/StringUtils.java @@ -10,6 +10,7 @@ import java.text.StringCharacterIterator; import java.util.Iterator; import java.util.Locale; +import java.util.regex.Pattern; import org.jetbrains.annotations.ApiStatus; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; @@ -21,6 +22,7 @@ public final class StringUtils { private static final String CORRUPTED_NIL_UUID = "0000-0000"; private static final String PROPER_NIL_UUID = "00000000-0000-0000-0000-000000000000"; + private static final @NotNull Pattern wordsPattern = Pattern.compile("[\\W_]+"); private StringUtils() {} @@ -50,6 +52,25 @@ private StringUtils() {} return str.substring(0, 1).toUpperCase(Locale.ROOT) + str.substring(1).toLowerCase(Locale.ROOT); } + /** + * Converts a String to CamelCase format. E.g. metric_bucket => MetricBucket; + * + * @param str the String to convert + * @return the camel case converted String or itself if empty or null + */ + public static @Nullable String camelCase(final @Nullable String str) { + if (str == null || str.isEmpty()) { + return str; + } + + String[] words = wordsPattern.split(str, -1); + StringBuilder builder = new StringBuilder(); + for (String w : words) { + builder.append(capitalize(w)); + } + return builder.toString(); + } + /** * Removes character specified by the delimiter parameter from the beginning and the end of the * string. diff --git a/sentry/src/test/java/io/sentry/clientreport/ClientReportTest.kt b/sentry/src/test/java/io/sentry/clientreport/ClientReportTest.kt index b4615cac76f..527e1b55310 100644 --- a/sentry/src/test/java/io/sentry/clientreport/ClientReportTest.kt +++ b/sentry/src/test/java/io/sentry/clientreport/ClientReportTest.kt @@ -23,6 +23,7 @@ import io.sentry.UncaughtExceptionHandlerIntegration.UncaughtExceptionHint import io.sentry.UserFeedback import io.sentry.dsnString import io.sentry.hints.Retryable +import io.sentry.metrics.EncodedMetrics import io.sentry.protocol.SentryId import io.sentry.protocol.SentryTransaction import io.sentry.protocol.User @@ -67,13 +68,14 @@ class ClientReportTest { SentryEnvelopeItem.fromUserFeedback(opts.serializer, UserFeedback(SentryId(UUID.randomUUID()))), SentryEnvelopeItem.fromAttachment(opts.serializer, NoOpLogger.getInstance(), Attachment("{ \"number\": 10 }".toByteArray(), "log.json"), 1000), SentryEnvelopeItem.fromProfilingTrace(ProfilingTraceData(File(""), transaction), 1000, opts.serializer), - SentryEnvelopeItem.fromCheckIn(opts.serializer, CheckIn("monitor-slug-1", CheckInStatus.ERROR)) + SentryEnvelopeItem.fromCheckIn(opts.serializer, CheckIn("monitor-slug-1", CheckInStatus.ERROR)), + SentryEnvelopeItem.fromMetrics(EncodedMetrics(emptyMap())) ) clientReportRecorder.recordLostEnvelope(DiscardReason.NETWORK_ERROR, envelope) val clientReportAtEnd = clientReportRecorder.resetCountsAndGenerateClientReport() - testHelper.assertTotalCount(12, clientReportAtEnd) + testHelper.assertTotalCount(13, clientReportAtEnd) testHelper.assertCountFor(DiscardReason.SAMPLE_RATE, DataCategory.Error, 3, clientReportAtEnd) testHelper.assertCountFor(DiscardReason.BEFORE_SEND, DataCategory.Error, 2, clientReportAtEnd) testHelper.assertCountFor(DiscardReason.QUEUE_OVERFLOW, DataCategory.Transaction, 1, clientReportAtEnd) @@ -83,6 +85,7 @@ class ClientReportTest { testHelper.assertCountFor(DiscardReason.NETWORK_ERROR, DataCategory.Attachment, 1, clientReportAtEnd) testHelper.assertCountFor(DiscardReason.NETWORK_ERROR, DataCategory.Profile, 1, clientReportAtEnd) testHelper.assertCountFor(DiscardReason.NETWORK_ERROR, DataCategory.Monitor, 1, clientReportAtEnd) + testHelper.assertCountFor(DiscardReason.NETWORK_ERROR, DataCategory.MetricBucket, 1, clientReportAtEnd) } @Test diff --git a/sentry/src/test/java/io/sentry/transport/RateLimiterTest.kt b/sentry/src/test/java/io/sentry/transport/RateLimiterTest.kt index d9e217b0ac0..8346ddf1023 100644 --- a/sentry/src/test/java/io/sentry/transport/RateLimiterTest.kt +++ b/sentry/src/test/java/io/sentry/transport/RateLimiterTest.kt @@ -20,6 +20,7 @@ import io.sentry.TransactionContext import io.sentry.UserFeedback import io.sentry.clientreport.DiscardReason import io.sentry.clientreport.IClientReportRecorder +import io.sentry.metrics.EncodedMetrics import io.sentry.protocol.SentryId import io.sentry.protocol.SentryTransaction import io.sentry.protocol.User @@ -102,13 +103,14 @@ class RateLimiterTest { val eventItem = SentryEnvelopeItem.fromEvent(fixture.serializer, SentryEvent()) val transaction = SentryTransaction(SentryTracer(TransactionContext("name", "op"), hub)) val transactionItem = SentryEnvelopeItem.fromEvent(fixture.serializer, transaction) - val envelope = SentryEnvelope(SentryEnvelopeHeader(), arrayListOf(eventItem, transactionItem)) + val statsdItem = SentryEnvelopeItem.fromMetrics(EncodedMetrics(emptyMap())) + val envelope = SentryEnvelope(SentryEnvelopeHeader(), arrayListOf(eventItem, transactionItem, statsdItem)) - rateLimiter.updateRetryAfterLimits("1:transaction:key, 1:default;error;security:organization", null, 1) + rateLimiter.updateRetryAfterLimits("1:transaction:key, 1:default;error;metric_bucket;security:organization", null, 1) val result = rateLimiter.filter(envelope, Hint()) assertNotNull(result) - assertEquals(2, result.items.count()) + assertEquals(3, result.items.count()) } @Test @@ -199,8 +201,9 @@ class RateLimiterTest { val attachmentItem = SentryEnvelopeItem.fromAttachment(fixture.serializer, NoOpLogger.getInstance(), Attachment("{ \"number\": 10 }".toByteArray(), "log.json"), 1000) val profileItem = SentryEnvelopeItem.fromProfilingTrace(ProfilingTraceData(File(""), transaction), 1000, fixture.serializer) val checkInItem = SentryEnvelopeItem.fromCheckIn(fixture.serializer, CheckIn("monitor-slug-1", CheckInStatus.ERROR)) + val statsdItem = SentryEnvelopeItem.fromMetrics(EncodedMetrics(emptyMap())) - val envelope = SentryEnvelope(SentryEnvelopeHeader(), arrayListOf(eventItem, userFeedbackItem, sessionItem, attachmentItem, profileItem, checkInItem)) + val envelope = SentryEnvelope(SentryEnvelopeHeader(), arrayListOf(eventItem, userFeedbackItem, sessionItem, attachmentItem, profileItem, checkInItem, statsdItem)) rateLimiter.updateRetryAfterLimits(null, null, 429) val result = rateLimiter.filter(envelope, Hint()) @@ -213,6 +216,7 @@ class RateLimiterTest { verify(fixture.clientReportRecorder, times(1)).recordLostEnvelopeItem(eq(DiscardReason.RATELIMIT_BACKOFF), same(attachmentItem)) verify(fixture.clientReportRecorder, times(1)).recordLostEnvelopeItem(eq(DiscardReason.RATELIMIT_BACKOFF), same(profileItem)) verify(fixture.clientReportRecorder, times(1)).recordLostEnvelopeItem(eq(DiscardReason.RATELIMIT_BACKOFF), same(checkInItem)) + verify(fixture.clientReportRecorder, times(1)).recordLostEnvelopeItem(eq(DiscardReason.RATELIMIT_BACKOFF), same(statsdItem)) verifyNoMoreInteractions(fixture.clientReportRecorder) } @@ -272,6 +276,71 @@ class RateLimiterTest { verifyNoMoreInteractions(fixture.clientReportRecorder) } + @Test + fun `drop metrics items as lost`() { + val rateLimiter = fixture.getSUT() + val hub = mock() + whenever(hub.options).thenReturn(SentryOptions()) + + val eventItem = SentryEnvelopeItem.fromEvent(fixture.serializer, SentryEvent()) + val f = File.createTempFile("test", "trace") + val transaction = SentryTracer(TransactionContext("name", "op"), hub) + val profileItem = SentryEnvelopeItem.fromProfilingTrace(ProfilingTraceData(f, transaction), 1000, fixture.serializer) + val statsdItem = SentryEnvelopeItem.fromMetrics(EncodedMetrics(emptyMap())) + val envelope = SentryEnvelope(SentryEnvelopeHeader(), arrayListOf(eventItem, profileItem, statsdItem)) + + rateLimiter.updateRetryAfterLimits("60:metric_bucket:key", null, 1) + val result = rateLimiter.filter(envelope, Hint()) + + assertNotNull(result) + assertEquals(2, result.items.toList().size) + + verify(fixture.clientReportRecorder, times(1)).recordLostEnvelopeItem(eq(DiscardReason.RATELIMIT_BACKOFF), same(statsdItem)) + verifyNoMoreInteractions(fixture.clientReportRecorder) + } + + @Test + fun `drop metrics items if namespace is custom`() { + val rateLimiter = fixture.getSUT() + val statsdItem = SentryEnvelopeItem.fromMetrics(EncodedMetrics(emptyMap())) + val envelope = SentryEnvelope(SentryEnvelopeHeader(), arrayListOf(statsdItem)) + + rateLimiter.updateRetryAfterLimits("60:metric_bucket:key:quota_exceeded:custom", null, 1) + val result = rateLimiter.filter(envelope, Hint()) + assertNull(result) + + verify(fixture.clientReportRecorder, times(1)).recordLostEnvelopeItem(eq(DiscardReason.RATELIMIT_BACKOFF), same(statsdItem)) + verifyNoMoreInteractions(fixture.clientReportRecorder) + } + + @Test + fun `drop metrics items if namespaces is empty`() { + val rateLimiter = fixture.getSUT() + val statsdItem = SentryEnvelopeItem.fromMetrics(EncodedMetrics(emptyMap())) + val envelope = SentryEnvelope(SentryEnvelopeHeader(), arrayListOf(statsdItem)) + + rateLimiter.updateRetryAfterLimits("60:metric_bucket:key:quota_exceeded::", null, 1) + val result = rateLimiter.filter(envelope, Hint()) + assertNull(result) + + verify(fixture.clientReportRecorder, times(1)).recordLostEnvelopeItem(eq(DiscardReason.RATELIMIT_BACKOFF), same(statsdItem)) + verifyNoMoreInteractions(fixture.clientReportRecorder) + } + + @Test + fun `drop metrics items if namespaces is not present`() { + val rateLimiter = fixture.getSUT() + val statsdItem = SentryEnvelopeItem.fromMetrics(EncodedMetrics(emptyMap())) + val envelope = SentryEnvelope(SentryEnvelopeHeader(), arrayListOf(statsdItem)) + + rateLimiter.updateRetryAfterLimits("60:metric_bucket:key:quota_exceeded", null, 1) + val result = rateLimiter.filter(envelope, Hint()) + assertNull(result) + + verify(fixture.clientReportRecorder, times(1)).recordLostEnvelopeItem(eq(DiscardReason.RATELIMIT_BACKOFF), same(statsdItem)) + verifyNoMoreInteractions(fixture.clientReportRecorder) + } + @Test fun `any limit can be checked`() { val rateLimiter = fixture.getSUT() diff --git a/sentry/src/test/java/io/sentry/util/CollectionUtilsTest.kt b/sentry/src/test/java/io/sentry/util/CollectionUtilsTest.kt index 266d188b4db..ebdaff477b1 100644 --- a/sentry/src/test/java/io/sentry/util/CollectionUtilsTest.kt +++ b/sentry/src/test/java/io/sentry/util/CollectionUtilsTest.kt @@ -4,6 +4,8 @@ import io.sentry.JsonObjectReader import java.io.StringReader import kotlin.test.Test import kotlin.test.assertEquals +import kotlin.test.assertFalse +import kotlin.test.assertTrue class CollectionUtilsTest { @@ -62,4 +64,19 @@ class CollectionUtilsTest { assertEquals("value1", result?.get("key1")) assertEquals("value3", result?.get("key3")) } + + @Test + fun `contains returns false for empty arrays`() { + assertFalse(CollectionUtils.contains(emptyArray(), "")) + } + + @Test + fun `contains returns true if element is present`() { + assertTrue(CollectionUtils.contains(arrayOf("one", "two", "three"), "two")) + } + + @Test + fun `contains returns false if element is not present`() { + assertFalse(CollectionUtils.contains(arrayOf("one", "two", "three"), "four")) + } } diff --git a/sentry/src/test/java/io/sentry/util/StringUtilsTest.kt b/sentry/src/test/java/io/sentry/util/StringUtilsTest.kt index d06f3859ae7..fe2d026dd28 100644 --- a/sentry/src/test/java/io/sentry/util/StringUtilsTest.kt +++ b/sentry/src/test/java/io/sentry/util/StringUtilsTest.kt @@ -57,6 +57,31 @@ class StringUtilsTest { assertEquals("", StringUtils.capitalize("")) } + @Test + fun `camelCase string`() { + assertEquals("TestCase", StringUtils.camelCase("test_case")) + } + + @Test + fun `camelCase string even if its uppercase`() { + assertEquals("TestCase", StringUtils.camelCase("TEST CASE")) + } + + @Test + fun `camelCase do not throw if only 1 char`() { + assertEquals("T", StringUtils.camelCase("t")) + } + + @Test + fun `camelCase returns itself if null`() { + assertNull(StringUtils.camelCase(null)) + } + + @Test + fun `camelCase returns itself if empty`() { + assertEquals("", StringUtils.camelCase("")) + } + @Test fun `removeSurrounding returns null if argument is null`() { assertNull(StringUtils.removeSurrounding(null, "\"")) From 0bed2c6c2dc4586e71311b292d2d4ac4de486acb Mon Sep 17 00:00:00 2001 From: stefanosiano Date: Mon, 8 Apr 2024 12:24:13 +0200 Subject: [PATCH 2/2] updated changelog --- CHANGELOG.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7019755c21d..6b6da2766bf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,11 @@ # Changelog +## Unreleased + +### Fixes + +- Add rate limit to Metrics ([#3334](https://github.com/getsentry/sentry-java/pull/3334)) + ## 7.7.0 ### Features