fix: rate limiting categories #381
Conversation
marandaneto
changed the title
Codecov Report
@@ Coverage Diff @@
## master #381 +/- ##
============================================
+ Coverage 59.23% 59.68% +0.45%
- Complexity 772 785 +13
============================================
Files 88 88
Lines 3562 3612 +50
Branches 346 350 +4
============================================
+ Hits 2110 2156 +46
- Misses 1299 1303 +4
Partials 153 153
Continue to review full report at Codecov.
|
sentry-core/src/main/java/io/sentry/core/transport/HttpTransport.java
Outdated
Show resolved
Hide resolved
sentry-core/src/main/java/io/sentry/core/transport/HttpTransport.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Code looks good, a few nitpicks, but the implementation in Cocoa is again different. We either have it wrong here or in Cocoa. We should clarify with @jan-auer before merging.
sentry-core/src/main/java/io/sentry/core/transport/HttpTransport.java
Outdated
Show resolved
Hide resolved
sentry-core/src/main/java/io/sentry/core/transport/HttpTransport.java
Outdated
Show resolved
Hide resolved
sentry-core/src/main/java/io/sentry/core/transport/HttpTransport.java
Outdated
Show resolved
Hide resolved
sentry-core/src/main/java/io/sentry/core/transport/HttpTransport.java
Outdated
Show resolved
Hide resolved
sentry-core/src/main/java/io/sentry/core/transport/HttpTransport.java
Outdated
Show resolved
Hide resolved
… also created enums
sentry-core/src/main/java/io/sentry/core/SentryEnvelopeItemType.java
Outdated
Show resolved
Hide resolved
sentry-core/src/main/java/io/sentry/core/transport/HttpTransport.java
Outdated
Show resolved
Hide resolved
sentry-core/src/test/java/io/sentry/core/transport/HttpTransportTest.kt
Outdated
Show resolved
Hide resolved
|
|
||
| transport.send(event) | ||
| Thread.sleep(2000) | ||
| assertFalse(transport.isRetryAfter("wtf")) |
There was a problem hiding this comment.
m: Maybe it makes sense to assert that the other categories have a retry after active.
Furthermore, we are going away from Retry-After to Rate-Limits, maybe it makes sense to rename this method at some point, it's a little bit confusing for me.
There was a problem hiding this comment.
renaming would be a breaking change as Android is released like this since 2.0 GA.
but still it's a retry after approach. why would it not make sense?
There was a problem hiding this comment.
Because it's our custom rate limit approach and I think we just used the retry after header to implement rate-limiting in the first place. But it's not a big deal.
| val event = SentryEvent() | ||
|
|
||
| transport.send(event) | ||
| Thread.sleep(2000) |
There was a problem hiding this comment.
l: As already pointed out testing with Thread.sleep is not the best idea. Imagine we want to test a maximum timeout of one day. How would we do that? Furthermore, this slows down the test suite a lot if we do it in multiple tests. We don't have to improve this in this PR, but maybe we do it at some point.
There was a problem hiding this comment.
| } | ||
|
|
||
| @Test | ||
| fun `When default category is present, apply for error category`() { |
There was a problem hiding this comment.
I think the upper test is already testing if default applies to error. I think we could remove this test.
There was a problem hiding this comment.
indirectly, yes, but I prefer an explicitly test for it.
a change can cause this test to be broken but not the above one. this guarantees that.
|
|
||
| whenever(fixture.connection.inputStream).thenThrow(IOException()) | ||
| whenever(fixture.connection.getHeaderField(eq("X-Sentry-Rate-Limits"))) | ||
| .thenReturn("60:error:key, 1:error:organization") |
There was a problem hiding this comment.
h: I think it would be nice to also test if a new timestamp is applied if the category has a longer rate-limiting. Something like this "2:error:organization 3:error:key, 1:error:organization" and then test that the rate limit is actually 3 seconds. Here a DateProvider with whom we could move the date 3 seconds would already be handy.
There was a problem hiding this comment.
true, but this test is being done indirectly by all of the other tests.
I don't mind adding an extra one, but we don't have the DateProvider right now, can do next.
📢 Type of change
📜 Description
rate limiting categories
💡 Motivation and Context
We were using envelope item types and it should be its own category.
💚 How did you test it?
📝 Checklist
🔮 Next steps