Add non blocking Apache HttpClient 5 based Transport. by maciejwalkowiak · Pull Request #1136 · getsentry/sentry-java

maciejwalkowiak · 2020-12-23T14:49:30Z

📜 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

I reviewed submitted code
I added tests to verify changes
All tests passing
No breaking changes

🔮 Next steps

Auto-configure new transport in Spring Boot auto-configuration when apache http client is on the classpath.

maciejwalkowiak · 2020-12-23T15:30:05Z

@marandaneto can you take a look what's the problem with Android sample here? There cannot be optional dependencies for the sentry module?

sentry/build.gradle.kts

sentry/src/main/java/io/sentry/ApacheHttpClientTransportFactory.java

marandaneto · 2020-12-23T17:23:52Z

@marandaneto can you take a look what's the problem with Android sample here? There cannot be optional dependencies for the sentry module?

see my comments, I bet they are not Android compatible.
I'm on vacation till the 4th so can't look into it in detail right now.

maciejwalkowiak · 2020-12-23T18:53:26Z

Thanks @marandaneto. Merry Christmas and see you on the 4th!

codecov-io · 2020-12-29T16:21:07Z

Codecov Report

Merging #1136 (60f95c1) into gh-1097-rate-limiter (28a0068) will decrease coverage by 0.02%.
The diff coverage is 79.77%.

@@                    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

Impacted Files	Coverage Δ	Complexity Δ
...ry/transport/apache/ApacheHttpClientTransport.java	`72.30% <72.30%> (ø)`	`8.00 <8.00> (?)`
...sport/apache/ApacheHttpClientTransportFactory.java	`100.00% <100.00%> (ø)`	`3.00 <3.00> (?)`
...io/sentry/spring/boot/SentryAutoConfiguration.java	`93.87% <100.00%> (+0.26%)`	`1.00 <0.00> (ø)`
...main/java/io/sentry/AsyncHttpTransportFactory.java	`100.00% <100.00%> (ø)`	`2.00 <1.00> (ø)`
...ava/io/sentry/SentryEnvelopeItemHeaderAdapter.java	`64.28% <0.00%> (-7.15%)`	`7.00% <0.00%> (-1.00%)`
...in/java/io/sentry/SentryEnvelopeHeaderAdapter.java	`75.58% <0.00%> (-3.49%)`	`17.00% <0.00%> (-1.00%)`
sentry/src/main/java/io/sentry/GsonSerializer.java	`96.77% <0.00%> (-0.20%)`	`9.00% <0.00%> (ø%)`
...a/io/sentry/exception/SentryEnvelopeException.java
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 28a0068...60f95c1. Read the comment docs.

maciejwalkowiak · 2021-01-06T10:47:48Z

@marandaneto @bruno-garcia please find some time to review this PR.

...apache-http-client-5/src/main/java/io/sentry/transport/apache/ApacheHttpClientTransport.java

bruno-garcia · 2021-01-06T18:03:12Z

@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

marandaneto · 2021-01-07T12:44:05Z

CHANGELOG.md

 * 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)   


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?

marandaneto · 2021-01-07T12:45:35Z

buildSrc/src/main/java/Config.kt

        val aspectj = "org.aspectj:aspectjweaver"
        val servletApi = "javax.servlet:javax.servlet-api"
+
+        val apacheHttpClient = "org.apache.httpcomponents.client5:httpclient5:5.0.3"


1st stable version of this lib v5 appeared in Feb, 2020, do you think that the majority of Apps have been upgraded already?

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.

marandaneto · 2021-01-07T12:49:17Z

...apache-http-client-5/src/main/java/io/sentry/transport/apache/ApacheHttpClientTransport.java

+              new FutureCallback<SimpleHttpResponse>() {
+                @Override
+                public void completed(SimpleHttpResponse response) {
+                  currentlyRunning.decrementAndGet();


should this be the last line to be called? theoretically there's still something happening.

There is but it means that the connection is not occupied anymore. I don't see any risk here.

marandaneto · 2021-01-08T09:56:56Z

...apache-http-client-5/src/main/java/io/sentry/transport/apache/ApacheHttpClientTransport.java

+          request.setHeader("Content-Encoding", "gzip");
+          request.setHeader("Content-Type", "application/x-sentry-envelope");
+          request.setHeader("Accept", "application/json");


Should this be part of RequestDetailsResolver or even extracting to a new class? it's a duplicated code right now.

If it's just 3 lines of code I wouldn't bother

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

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.

maciejwalkowiak · 2021-01-11T17:44:38Z

@bruno-garcia @marandaneto take another look please.

Add non blocking Apache HttpClient 5 based Transport.

6fc166b

maciejwalkowiak requested review from bruno-garcia and marandaneto as code owners December 23, 2020 14:49

Changelog.

1c1b5f3

marandaneto reviewed Dec 23, 2020

View reviewed changes

sentry/build.gradle.kts Outdated Show resolved Hide resolved

marandaneto reviewed Dec 23, 2020

View reviewed changes

sentry/src/main/java/io/sentry/ApacheHttpClientTransportFactory.java Show resolved Hide resolved

maciejwalkowiak added 2 commits December 24, 2020 14:21

Move ApacheHttpClientTransport to a separate module.

8104feb

Rename module.

a1ad38f

ApacheHttpClientTransportFactory auto-configuration. (#1143)

ad3a7b8

This was referenced Dec 31, 2020

High throughput transport implementation #1114

Merged

WIP: Added non-blocking connection implementation. #1104

Closed

This was referenced Jan 6, 2021

4.0.0 GA - Performance Monitoring #1151

Closed

Attach HTTP headers to Sentry request #594

Closed

bruno-garcia reviewed Jan 6, 2021

View reviewed changes

...apache-http-client-5/src/main/java/io/sentry/transport/apache/ApacheHttpClientTransport.java Outdated Show resolved Hide resolved

marandaneto reviewed Jan 7, 2021

View reviewed changes

marandaneto reviewed Jan 8, 2021

View reviewed changes

Fix content type.

60f95c1

bruno-garcia approved these changes Jan 11, 2021

View reviewed changes

maciejwalkowiak merged commit 92876c8 into gh-1097-rate-limiter Jan 12, 2021

maciejwalkowiak deleted the gh-1097-apache-non-blocking-transport branch January 12, 2021 14:44

Uh oh!

Conversation

maciejwalkowiak commented Dec 23, 2020

📜 Description

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

🔮 Next steps

Uh oh!

maciejwalkowiak commented Dec 23, 2020

Uh oh!

Uh oh!

Uh oh!

marandaneto commented Dec 23, 2020

Uh oh!

maciejwalkowiak commented Dec 23, 2020

Uh oh!

codecov-io commented Dec 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

maciejwalkowiak commented Jan 6, 2021

Uh oh!

Uh oh!

bruno-garcia commented Jan 6, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maciejwalkowiak commented Jan 11, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-io commented Dec 29, 2020 •

edited

Loading