feat: Add support for configuring compression levels #11805
+222
−64
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rnishtala-sumo
force-pushed
the
compression-config-option2
branch
3 times, most recently
from
7bbcdeb to
ad9e1ee
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #11805 +/- ##
==========================================
+ Coverage 91.44% 91.53% +0.08%
==========================================
Files 446 445 -1
Lines 23742 23760 +18
==========================================
+ Hits 21712 21749 +37
+ Misses 1654 1633 -21
- Partials 376 378 +2 ☔ View full report in Codecov by Sentry. |
dmitryax
reviewed
Dec 5, 2024
rnishtala-sumo
force-pushed
the
compression-config-option2
branch
3 times, most recently
from
b9aedbf to
853d79f
Compare
rnishtala-sumo
force-pushed
the
compression-config-option2
branch
from
853d79f to
ad9a8a3
Compare
dmitryax
reviewed
Dec 11, 2024
| # Use this changelog template to create an entry for release notes. | ||
|
|
||
| # One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' | ||
| change_type: 'breaking' |
| # (Optional) One or more lines of additional information to render under the primary note. | ||
| # These lines will be padded with 2 spaces and then inserted directly into the document. | ||
| # Use pipe (|) for multiline entries. | ||
| subtext: |
There was a problem hiding this comment.
Please add more details here describing a new field
| import "fmt" | ||
| import ( | ||
| "fmt" | ||
| ) |
There was a problem hiding this comment.
Nit please avoid unrelated changes
| @@ -36,7 +36,7 @@ require ( | |||
| github.com/golang/snappy v0.0.4 // indirect | |||
| github.com/google/uuid v1.6.0 // indirect | |||
| github.com/json-iterator/go v1.1.12 // indirect | |||
| github.com/klauspost/compress v1.17.9 // indirect | |||
| github.com/klauspost/compress v1.17.11 // indirect | |||
There was a problem hiding this comment.
Why is this change? It doesn't require new version right?
| compressor *compressor | ||
| rt http.RoundTripper | ||
| compressionType configcompression.Type | ||
| CompressionParams configcompression.CompressionParams |
There was a problem hiding this comment.
to be consistent
Suggested change
| CompressionParams configcompression.CompressionParams | |
| compressionParams configcompression.CompressionParams |
| tests := []struct { | ||
| name string | ||
| encoding configcompression.Type | ||
| enclevel configcompression.Level |
| } | ||
| client, err := clientSettings.ToClient(context.Background(), componenttest.NewNopHost(), componenttest.NewNopTelemetrySettings()) | ||
| require.NoError(t, err) | ||
| res, err := client.Do(req) | ||
| if tt.shouldError { | ||
| assert.Error(t, err) | ||
| require.Error(t, err) |
| @@ -104,16 +119,26 @@ func TestHTTPClientCompression(t *testing.T) { | |||
|
|
|||
| req, err := http.NewRequest(http.MethodGet, srv.URL, reqBody) | |||
| require.NoError(t, err, "failed to create request to test handler") | |||
|
|
|||
| compressionType := tt.encoding | |||
| err = compressionType.UnmarshalText([]byte(tt.encoding)) | |||
dmitryax
reviewed
Dec 11, 2024
| - SpeedFastest: `1` | ||
| - SpeedDefault: `3` | ||
| - SpeedBetterCompression: `6` | ||
| - SpeedBestCompression: `11` |
There was a problem hiding this comment.
Does it mean that zstd has more levels? I see we don't validate that
| // Checks the validity of zlib/gzip/flate compression levels | ||
| func isValidLevel(level configcompression.Level) bool { | ||
| return level == zlib.DefaultCompression || | ||
| level == configcompression.LevelNone || |
| level == configcompression.LevelNone || | ||
| level == zlib.HuffmanOnly || | ||
| level == zlib.NoCompression || | ||
| level == zlib.BestSpeed || |
There was a problem hiding this comment.
This seems redundant as we check it in the next line
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Add the option of configuring compression levels. This is not intended to be a breaking change for the user.
Link to tracking issue
Issue - #10467
Testing
Unit tests were added/changed