Make ITransport top-level abstraction for sending events to Sentry. #1118
Conversation
Flip the relation between `AsyncConnection` and `ITransport` and make `BlockingHttpTransport` use `HttpConnection`, making it possible to provide alternative transport implementations not bound to use thread pool.
maciejwalkowiak
requested review from
bruno-garcia and
marandaneto
as code owners
|
@marandaneto, as a next step - I believe we should create an interface |
Codecov Report
@@ Coverage Diff @@
## gh-1097-rate-limiter #1118 +/- ##
==========================================================
- Coverage 74.09% 73.66% -0.44%
+ Complexity 1591 1569 -22
==========================================================
Files 164 164
Lines 5633 5609 -24
Branches 576 572 -4
==========================================================
- Hits 4174 4132 -42
- Misses 1178 1185 +7
- Partials 281 292 +11
Continue to review full report at Codecov.
|
| envelopeCache.discard(envelope); | ||
| } | ||
| } else { | ||
| executor.submit(new EnvelopeSender(filteredEnvelope, hint, currentEnvelopeCache)); |
There was a problem hiding this comment.
I think the name BlockingHttpTransport is misleading since it gives the impression the interface is going to block until for the event submission, which isnt' the case.
We're scheduling the event through the executor here.
Maybe we can use another name?
There was a problem hiding this comment.
yeah I kinda agree, not sure about a suitable name though.
SingleThreadHttpTransport?
There was a problem hiding this comment.
Can be also AsyncHttpTransport. It is async, but blocking.
| @ApiStatus.NonExtendable // only not final because of testing | ||
| @ApiStatus.Internal | ||
| public class HttpTransport implements ITransport { | ||
| class HttpConnection { |
There was a problem hiding this comment.
is the HttpConnection reusable for the NIO implementation as well? just trying to understand why we have the BlockingHttpTransport + HttpConnection, I do get their difference, but what would be the advantage of it right now?
There was a problem hiding this comment.
Just to split the responsibilities - and I agree that the split is not perfect. I tried to create decorators like EnvelopeCachingHttpTransport and RateLimitingHttpTransport but since send returns void it's also not possible.
HttpConnection is package private, to give us flexibility of refactoring later without breaking users' code.
|
do we need a changelog? how do we communicate the breaking changes later on? |
|
@marandaneto @bruno-garcia I need this PR reviewed. |
| final class AsyncHttpTransportFactory implements TransportFactory { | ||
|
|
||
| @Override | ||
| public ITransport create(final @NotNull SentryOptions options) { |
There was a problem hiding this comment.
should we add throws IllegalArgumentException in the method signature?
There was a problem hiding this comment.
I think we should not, what's the reasonable way to handle this exception here anyways?
There was a problem hiding this comment.
if we handle it down its fine, I was just curious cus the TransportFactory now is the way to go for custom transports right
There was a problem hiding this comment.
We don't handle it, the exception is propagated to the very top and in the end Sentry init will fail.
This PR is a PR to another branch, so once whole package is there we can take a look at it again.
There was a problem hiding this comment.
few missing jetbrains annotations but other than that LGTM.
I didn't find a plugin that fails the build if annotations are missing, but it'd be nice to write a custom linter for that, so I don't need to be the human linter here :D
I am only picky about that cus it's nicer when writing Kotlin code.
| import org.jetbrains.annotations.NotNull; | ||
|
|
||
| /** Creates instances of {@link ITransport}. */ | ||
| public interface TransportFactory { |
There was a problem hiding this comment.
should we do ITransportFactory?
There was a problem hiding this comment.
I have mixed feelings about putting I in the front of names for interfaces, but I will just follow your preference. Tell me :-)
There was a problem hiding this comment.
no feelings or preferences here, but see ITransport, so ideally, we do a single way, either all of them or none of them.
chose which ones to rename :)
There was a problem hiding this comment.
Let it be ITransportFactory.
📢 Type of change
📜 Description
Flip the relation between
AsyncConnectionandITransportand makeBlockingHttpTransportuseHttpConnection, making it possible to provide alternative transport implementations not bound to use thread pool.💡 Motivation and Context
In order to provide non blocking transport, transport itself has to be responsible for managing threads (not the AsyncConnection).
💚 How did you test it?
Unit & integration tests.
📝 Checklist
🔮 Next steps