-
-
Notifications
You must be signed in to change notification settings - Fork 32
feat: make gzip as default content encoding type #433
Conversation
Codecov Report
@@ Coverage Diff @@
## master #433 +/- ##
=========================================
Coverage ? 60.26%
Complexity ? 816
=========================================
Files ? 93
Lines ? 3727
Branches ? 359
=========================================
Hits ? 2246
Misses ? 1328
Partials ? 153
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice removal of some code duplication there!
I just have a suggestion for consideration; will leave it up to another reviewer to approve.
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
…rt.java Co-authored-by: Rodolfo Carvalho <rhcarvalho@gmail.com>
sentry-core/src/main/java/io/sentry/core/transport/HttpTransport.java
Outdated
Show resolved
Hide resolved
…necessary flushes as try-resources will do it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good. Still I have a few comments :)
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/test/java/io/sentry/core/transport/HttpTransportTest.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
📢 Type of change
📜 Description
feat: make gzip as default content encoding type
💡 Motivation and Context
compress the content of the payload before sending it over the network, so saving some data.
💚 How did you test it?
running and tests passed.
serialization layer is not affected and so didn't need extra tests.
the response content is still a raw JSON, no need to change that as we only get a very small response when there's a 4xx error for example.
📝 Checklist
🔮 Next steps
CI error is due to missing NDK on LGTM analyzer.
https://discuss.lgtm.com/t/android-ndk-v21-1-6352462-is-not-available/2910