Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(compression): Add the option of configuring compression levels #11084

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions .chloggen/configure-compression-levels.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
# Use this changelog template to create an entry for release notes.

# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
change_type: 'enhancement'

# The name of the component, or a single word describing the area of concern, (e.g. otlpreceiver)
component: confighttp

# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
note: Added support for configuring compression levels.

# One or more tracking issues or pull requests related to the change
issues: [10467]

# (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:

# Optional: The change log or logs in which this entry should be included.
# e.g. '[user]' or '[user, api]'
# Include 'user' if the change is relevant to end users.
# Include 'api' if there is a change to a library API.
# Default: '[user]'
change_logs: [user, api]
58 changes: 47 additions & 11 deletions config/configcompression/compressiontype.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,20 +3,31 @@

package configcompression // import "go.opentelemetry.io/collector/config/configcompression"

import "fmt"
import (
"fmt"

"github.com/klauspost/compress/zlib"
)

// Type represents a compression method
type Type string
Copy link
Member

@dmitryax dmitryax Sep 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we can reuse this type and keep the level encoded in the string. API users may rely on configcompression.Type == configcompression.TypeGzip which won't work anymore...

I think we need new additional types like these

type Config struct {
	Type Type
	Level Level
}

type Level int

Config may be also called TypeWithLevel.

Given that this module is 1.x, it should be okay to add. However, we would need to make a breaking change to the Co API (not an end-user breaking change) in config HTTP by changing the confighttp.ClientConfig.Compression field from compressionconfig.Type to compressionconfig.Config, which should be okay given that the confighttp module is still 0.x.

Then we can have all validation in Config.UnmarshalText and assume that Config is always valid

cc @open-telemetry/collector-approvers

Copy link
Contributor Author

@rnishtala-sumo rnishtala-sumo Sep 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

confighttp.ClientConfig.Compression field from compressionconfig.Type to compressionconfig.Config

won't this be a breaking change for the user as well?, i.e with the above suggested change we would need two fields to specify compression like below

compression:
  type: "gzip"
  level: 1

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dmitryax I've made changes based on the above comment. Please let me know if this is what you had in mind.

Copy link
Contributor Author

@rnishtala-sumo rnishtala-sumo Sep 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like the tests are failing because of errors like below, since this is now a breaking change

Error: config/config.go:147:24: invalid operation: cfg.Compression != "" (mismatched types configcompression.TypeWithLevel and untyped string)

There are tests in multiple contrib components that are failing because they assume the Compression is a string.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

won't this be a breaking change for the user as well?,

No, we should keep the string encoding <type>/<level> where the /<level> part is optional

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JFYI, I tested that with the above change both of the following configuration are valid

compression struct

exporters:
  otlphttp:
    endpoint: otelcol:4317
    compression:
      type: gzip
      level: 1

compression string

exporters:
  otlphttp:
    endpoint: otelcol:4317
    compression: gzip

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @rnishtala-sumo. To be explicit, I (obviously) think this approach looks good :)

I suppose we'll need to wait for a couple more opinions from approvers to make a final decision

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dmitryax @andrzej-stencel do you think we could proceed with @axw's approach?

Copy link
Member

@dmitryax dmitryax Dec 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update: We discussed this during the Collector SIG call on Dec 4. Most of the Collector leads present there (@mx-psi and @jpkrohling) favored introducing another field (compression_params) to avoid breaking the existing user interface even in a backward compatible way. @rnishtala-sumo is going to submit another PR to implement that approach.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's the new PR with option2 - #11805

type Level int

type TypeWithLevel struct {
Type Type `mapstructure:"type"`
Level Level `mapstructure:"level"`
}

const (
TypeGzip Type = "gzip"
TypeZlib Type = "zlib"
TypeDeflate Type = "deflate"
TypeSnappy Type = "snappy"
TypeZstd Type = "zstd"
TypeLz4 Type = "lz4"
typeNone Type = "none"
typeEmpty Type = ""
TypeGzip Type = "gzip"
TypeZlib Type = "zlib"
TypeDeflate Type = "deflate"
TypeSnappy Type = "snappy"
TypeZstd Type = "zstd"
TypeLz4 Type = "lz4"
typeNone Type = "none"
typeEmpty Type = ""
LevelNone Level = 0
)

// IsCompressed returns false if CompressionType is nil, none, or empty.
Expand All @@ -25,7 +36,7 @@
return *ct != typeEmpty && *ct != typeNone
}

func (ct *Type) UnmarshalText(in []byte) error {
func (ct *TypeWithLevel) UnmarshalText(in []byte) error {
typ := Type(in)
if typ == TypeGzip ||
typ == TypeZlib ||
Expand All @@ -35,8 +46,33 @@
typ == TypeLz4 ||
typ == typeNone ||
typ == typeEmpty {
*ct = typ
*&ct.Type = typ
return nil
}
return fmt.Errorf("unsupported compression type %q", typ)
}

func (ct *TypeWithLevel) Validate() error {
if (ct.Type == TypeGzip && isValidLevel(int(ct.Level))) ||
(ct.Type == TypeZlib && isValidLevel(int(ct.Level))) ||
(ct.Type == TypeDeflate && isValidLevel(int(ct.Level))) ||
ct.Type == TypeSnappy ||
ct.Type == TypeLz4 ||
ct.Type == TypeZstd ||
ct.Type == typeNone ||
ct.Type == typeEmpty {
return nil
}

Check warning on line 65 in config/configcompression/compressiontype.go

View check run for this annotation

Codecov / codecov/patch

config/configcompression/compressiontype.go#L55-L65

Added lines #L55 - L65 were not covered by tests

return fmt.Errorf("unsupported compression type and level %s - %d", ct.Type, ct.Level)

Check warning on line 67 in config/configcompression/compressiontype.go

View check run for this annotation

Codecov / codecov/patch

config/configcompression/compressiontype.go#L67

Added line #L67 was not covered by tests
}

// Checks the validity of zlib/gzip/flate compression levels
func isValidLevel(level int) bool {
return level == zlib.DefaultCompression ||
level == int(LevelNone) ||
level == zlib.HuffmanOnly ||
level == zlib.NoCompression ||
level == zlib.BestSpeed ||
(level >= zlib.BestSpeed && level <= zlib.BestCompression)

Check warning on line 77 in config/configcompression/compressiontype.go

View check run for this annotation

Codecov / codecov/patch

config/configcompression/compressiontype.go#L71-L77

Added lines #L71 - L77 were not covered by tests
}
4 changes: 2 additions & 2 deletions config/configcompression/compressiontype_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -71,15 +71,15 @@ func TestUnmarshalText(t *testing.T) {
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
temp := typeNone
var temp TypeWithLevel
err := temp.UnmarshalText(tt.compressionName)
if tt.shouldError {
assert.Error(t, err)
return
}
require.NoError(t, err)
ct := Type(tt.compressionName)
assert.Equal(t, temp, ct)
assert.Equal(t, temp.Type, ct)
assert.Equal(t, tt.isCompressed, ct.IsCompressed())
})
}
Expand Down
1 change: 1 addition & 0 deletions config/configcompression/go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ module go.opentelemetry.io/collector/config/configcompression
go 1.22.0

require (
github.com/klauspost/compress v1.17.9
github.com/stretchr/testify v1.10.0
go.uber.org/goleak v1.3.0
)
Expand Down
2 changes: 2 additions & 0 deletions config/configcompression/go.sum

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

23 changes: 23 additions & 0 deletions config/confighttp/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,29 @@ README](../configtls/README.md).
- `compression`: Compression type to use among `gzip`, `zstd`, `snappy`, `zlib`, `deflate`, and `lz4`.
- look at the documentation for the server-side of the communication.
- `none` will be treated as uncompressed, and any other inputs will cause an error.
- Compression levels can now be configured as part of the compression type like below. Not specifying any compression level will result in the default.
- `gzip`
- NoCompression: `gzip/0`
- BestSpeed: `gzip/1`
- BestCompression: `gzip/9`
- DefaultCompression: `gzip`
- `zlib`
- NoCompression: `zlib/0`
- BestSpeed: `zlib/1`
- BestCompression: `zlib/9`
- DefaultCompression: `zlib`
- `deflate`
- NoCompression: `deflate/0`
- BestSpeed: `deflate/1`
- BestCompression: `deflate/9`
- DefaultCompression: `deflate`
- `zstd`
- SpeedFastest: `zstd/1`
- SpeedDefault: `zstd/3`
- SpeedBetterCompression: `zstd/6`
- SpeedBestCompression: `zstd/11`
- `snappy`
No compression levels supported yet
- [`max_idle_conns`](https://golang.org/pkg/net/http/#Transport)
- [`max_idle_conns_per_host`](https://golang.org/pkg/net/http/#Transport)
- [`max_conns_per_host`](https://golang.org/pkg/net/http/#Transport)
Expand Down
6 changes: 3 additions & 3 deletions config/confighttp/compression.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import (

type compressRoundTripper struct {
rt http.RoundTripper
compressionType configcompression.Type
compressionType configcompression.TypeWithLevel
compressor *compressor
}

Expand Down Expand Up @@ -76,7 +76,7 @@ var availableDecoders = map[string]func(body io.ReadCloser) (io.ReadCloser, erro
},
}

func newCompressRoundTripper(rt http.RoundTripper, compressionType configcompression.Type) (*compressRoundTripper, error) {
func newCompressRoundTripper(rt http.RoundTripper, compressionType configcompression.TypeWithLevel) (*compressRoundTripper, error) {
encoder, err := newCompressor(compressionType)
if err != nil {
return nil, err
Expand Down Expand Up @@ -112,7 +112,7 @@ func (r *compressRoundTripper) RoundTrip(req *http.Request) (*http.Response, err

// Clone the headers and add the encoding header.
cReq.Header = req.Header.Clone()
cReq.Header.Add(headerContentEncoding, string(r.compressionType))
cReq.Header.Add(headerContentEncoding, string(r.compressionType.Type))

return r.rt.RoundTrip(cReq)
}
Expand Down
Loading
Loading