Add non blocking Apache HttpClient 5 based Transport. #1136
Conversation
maciejwalkowiak
requested review from
bruno-garcia and
marandaneto
as code owners
|
@marandaneto can you take a look what's the problem with Android sample here? There cannot be optional dependencies for the sentry module? |
sentry/src/main/java/io/sentry/ApacheHttpClientTransportFactory.java
Outdated
Show resolved
Hide resolved
see my comments, I bet they are not Android compatible. |
|
Thanks @marandaneto. Merry Christmas and see you on the 4th! |
Codecov Report
@@ Coverage Diff @@
## gh-1097-rate-limiter #1136 +/- ##
==========================================================
- Coverage 74.91% 74.88% -0.03%
- Complexity 1613 1624 +11
==========================================================
Files 169 170 +1
Lines 5617 5706 +89
Branches 548 557 +9
==========================================================
+ Hits 4208 4273 +65
- Misses 1153 1172 +19
- Partials 256 261 +5
Continue to review full report at Codecov.
|
|
@marandaneto @bruno-garcia please find some time to review this PR. |
...apache-http-client-5/src/main/java/io/sentry/transport/apache/ApacheHttpClientTransport.java
Outdated
Show resolved
Hide resolved
|
@maciejwalkowiak Manoel is back next week. It LGTM but I have a note on the stream->byte array and the json type of the body, lets discuss that before merging |
| @@ -13,6 +13,8 @@ | |||
| * Ref: `ITransport` implementations are now responsible for executing request in asynchronous or synchronous way (#1118) | |||
| * Ref: Add option to set `TransportFactory` instead of `ITransport` on `SentryOptions` (#1124) | |||
| * Ref: Simplify ITransport creation in ITransportFactory (#1135) | |||
| * Feat: Add non blocking Apache HttpClient 5 based Transport (#1136) | |||
| * Enhancement: Autoconfigure Apache HttpClient 5 based Transport in Spring Boot integration (#1143) | |||
There was a problem hiding this comment.
this is part of the PR template next steps, but it's in the changelog as well, which one would be not up to date?
| @@ -57,6 +57,8 @@ object Config { | |||
| val springAop = "org.springframework:spring-aop" | |||
| val aspectj = "org.aspectj:aspectjweaver" | |||
| val servletApi = "javax.servlet:javax.servlet-api" | |||
|
|
|||
| val apacheHttpClient = "org.apache.httpcomponents.client5:httpclient5:5.0.3" | |||
There was a problem hiding this comment.
1st stable version of this lib v5 appeared in Feb, 2020, do you think that the majority of Apps have been upgraded already?
There was a problem hiding this comment.
There is no package collision between version 5 and 4 so even if they did not, it means just an extra dependency. We still can add transport based on 4.x if there will be such request.
| new FutureCallback<SimpleHttpResponse>() { | ||
| @Override | ||
| public void completed(SimpleHttpResponse response) { | ||
| currentlyRunning.decrementAndGet(); |
There was a problem hiding this comment.
should this be the last line to be called? theoretically there's still something happening.
There was a problem hiding this comment.
There is but it means that the connection is not occupied anymore. I don't see any risk here.
| request.setHeader("Content-Encoding", "gzip"); | ||
| request.setHeader("Content-Type", "application/x-sentry-envelope"); | ||
| request.setHeader("Accept", "application/json"); |
There was a problem hiding this comment.
Should this be part of RequestDetailsResolver or even extracting to a new class? it's a duplicated code right now.
There was a problem hiding this comment.
If it's just 3 lines of code I wouldn't bother
There was a problem hiding this comment.
if RequestDetailsResolver is used in both transports and they are meant to configure the connection with its headers, I don't see why duplicating it though.
https://github.com/getsentry/sentry-java/blob/gh-1097-apache-non-blocking-transport/sentry/src/main/java/io/sentry/transport/HttpConnection.java#L124-L126
There was a problem hiding this comment.
It's up to the transport to decide if it sends gzip or not, I also don't think that we should set fixed accept header. RequestDetailsResolver returns headers that users MUST send to Sentry.
|
@bruno-garcia @marandaneto take another look please. |
📜 Description
Add non blocking Apache HttpClient 5 based Transport suitable for high traffic backend applications.
💡 Motivation and Context
Current AsyncTransport can perform up to 3 requests per second which is too little for performance feature running in backend applications.
💚 How did you test it?
Unit tests plus sample app for running performance tests.
📝 Checklist
🔮 Next steps
Auto-configure new transport in Spring Boot auto-configuration when apache http client is on the classpath.