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

[rnishtala-sumo]

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

Documentation

https://github.com/rnishtala-sumo/opentelemetry-collector/blob/configure-compression-levels/config/confighttp/README.md#client-configuration

[codecov]

Codecov Report

Attention: Patch coverage is 54.83871% with 28 lines in your changes missing coverage. Please review.

Project coverage is 91.33%. Comparing base (bc7af62) to head (78d83fd).
Report is 113 commits behind head on main.

Files with missing lines	Patch %	Lines
config/configcompression/compressiontype.go	9.52%	19 Missing ⚠️
config/confighttp/compressor.go	75.67%	7 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #11084      +/-   ##
==========================================
- Coverage   91.43%   91.33%   -0.10%     
==========================================
  Files         447      447              
  Lines       23745    23783      +38     
==========================================
+ Hits        21712    21723      +11     
- Misses       1657     1682      +25     
- Partials      376      378       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[rnishtala-sumo]

@dmitrii Anoshin @alex Boten @mx-psi would appreciate some attention to this PR. It doesn't have any breaking changes.

[dmitryax]

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

[rnishtala-sumo]

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

[rnishtala-sumo]

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

[rnishtala-sumo]

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.

[dmitryax]

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

[rnishtala-sumo]

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

[axw]

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

[rnishtala-sumo]

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

[dmitryax]

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.

[rnishtala-sumo]

Here's the new PR with option2 - #11805

[andrzej-stencel]

I like this approach. It's additive and doesn't break stable components.

[dmitryax]

Nits

[github-actions]

This PR was marked stale due to lack of activity. It will be closed in 14 days.

[github-actions]

This PR was marked stale due to lack of activity. It will be closed in 14 days.