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

rnishtala-sumo · 2024-09-09T16:58:19Z

Description

Add the option of configuring compression levels. This is not intended to be a breaking change for the user.

Link to tracking issue

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 · 2024-09-09T17:16:41Z

Codecov Report

Attention: Patch coverage is 79.01235% with 17 lines in your changes missing coverage. Please review.

Project coverage is 92.08%. Comparing base (4f79e0d) to head (427a8ed).

Files with missing lines	Patch %	Lines
config/confighttp/compressor.go	62.22%	14 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #11084      +/-   ##
==========================================
- Coverage   92.14%   92.08%   -0.06%     
==========================================
  Files         433      433              
  Lines       20389    20437      +48     
==========================================
+ Hits        18787    18819      +32     
- Misses       1238     1251      +13     
- Partials      364      367       +3

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

rnishtala-sumo · 2024-09-24T21:06:00Z

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

config/confighttp/compressor.go

config/confighttp/README.md

config/configcompression/compressiontype.go

config/confighttp/confighttp.go

config/configcompression/compressiontype.go

config/confighttp/compressor.go

dmitryax · 2024-10-01T21:22:05Z

config/confighttp/confighttp.go

@@ -216,8 +216,13 @@ func (hcs *ClientConfig) ToClient(ctx context.Context, host component.Host, sett

 	// Compress the body using specified compression methods if non-empty string is provided.
 	// Supporting gzip, zlib, deflate, snappy, and zstd; none is treated as uncompressed.
+	var compressionTypeWithLevel configcompression.TypeWithLevel
+	err = compressionTypeWithLevel.UnmarshalText([]byte(hcs.Compression))


We don't need to call UnmarshalText explicitly. Changing ClientConfig.Compression field from configcompression.Type to configcompression.TypeWithLevel should be enough

Doing that seemed to be a breaking change. There are errors like this seen in multiple contrib components

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

Yes, it is a Go API breaking change, but not an end-user breaking change. I think this is the cleanest solution. @open-telemetry/collector-approvers let me know if you have other ideas

if we make the API breaking change, are going to mute all the failing tests (in this repo) and have the contrib component owners fix that later. Sorry, I'm not clear on this process.

@dmitryax would appreciate some clarity on this. The suggested API breaking change is going to break collector tests that reference components from the contrib repo. Are we ok fixing those later in a separate PR?

How about keeping this as is (non-breaking) for this PR? and handle all breaking changes (core and contrib) in 2 other PRs. The only reason is, it seems to affect multiple contrib components (almost all)

@dmitryax I have pushed the API breaking changes and the contrib tests(contrib-tests / contrib-tests-matrix) seem to be failing.

If you have an idea how to make it in several steps without breaking contrib and get this result, that would be great. I'm not aware of any

the only thing I can think of is keeping Compression as it original type in this PR i.e configcompression.Type as suggested in a previous comment, and handling the breaking changes together.

#11084 (comment)

I think we can stick to the current plan. These are the only changes that need to go into the contrib repository

https://github.com/open-telemetry/opentelemetry-collector-contrib/pull/35843/files

config/confighttp/README.md

config/configcompression/compressiontype.go

dmitryax · 2024-10-14T22:27:24Z

config/configcompression/compressiontype.go


+	return fmt.Errorf("unsupported compression type/level %s/%d", compressionTyp, ct.Level)


This would be confusing output if user didn't provide level

this should print the default level, which I think is fine?

I made a change to the error message, please let me know if you think this is clearer

c7f36ad

dmitryax · 2024-10-14T22:33:14Z

config/confighttp/compressor.go

+		if gZipCompressor.pool.Get() == nil {
+			gZipCompressor.pool = sync.Pool{New: func() any { w, _ := gzip.NewWriterLevel(nil, int(compressionType.Level)); return w }}


This seems incorrect. We are taking compressor with any level from the pool. If user picks zlib/1 first and it goes back to the pool, the second call with any other level like zlib/2 would still give the same zlib/1 instance from the pool. We cannot reuse them compressors with different levels.

We might need a map of pools built dynamically here

@dmitryax it doesn't look like the user can change the compression level dynamically? Because they would need to restart the collector to change this configuration. Wouldn't the sync.pool be cleared when this happens?

I created a map of compressor pools for compressors that support levels, all the tests pass with this change
c7f36ad

…t coverage

… levels

…el for compression

rnishtala-sumo force-pushed the configure-compression-levels branch from 0500415 to b13f7fc Compare September 9, 2024 17:05

rnishtala-sumo force-pushed the configure-compression-levels branch 3 times, most recently from 2ff904d to e39d5e7 Compare September 10, 2024 15:25

rnishtala-sumo marked this pull request as ready for review September 10, 2024 15:26

rnishtala-sumo requested review from a team and codeboten September 10, 2024 15:26

rnishtala-sumo force-pushed the configure-compression-levels branch 2 times, most recently from 2165697 to f478e7d Compare September 10, 2024 19:02

rnishtala-sumo force-pushed the configure-compression-levels branch from f478e7d to 83cf4e5 Compare September 20, 2024 22:02

rnishtala-sumo requested a review from a team as a code owner September 20, 2024 22:02

rnishtala-sumo force-pushed the configure-compression-levels branch from 83cf4e5 to e58acbc Compare September 20, 2024 23:29

dmitryax reviewed Sep 25, 2024

View reviewed changes

config/confighttp/compressor.go Outdated Show resolved Hide resolved

config/confighttp/README.md Show resolved Hide resolved

config/configcompression/compressiontype.go Show resolved Hide resolved

rnishtala-sumo force-pushed the configure-compression-levels branch from e58acbc to 0b2b372 Compare September 30, 2024 21:07

rnishtala-sumo commented Sep 30, 2024

View reviewed changes

config/confighttp/confighttp.go Show resolved Hide resolved

rnishtala-sumo requested a review from dmitryax September 30, 2024 21:21

rnishtala-sumo force-pushed the configure-compression-levels branch from 0b2b372 to 9fe4614 Compare September 30, 2024 22:11

dmitryax reviewed Sep 30, 2024

View reviewed changes

config/configcompression/compressiontype.go Outdated Show resolved Hide resolved

rnishtala-sumo force-pushed the configure-compression-levels branch from 60c6f00 to 800a4c2 Compare October 1, 2024 15:21

rnishtala-sumo requested a review from dmitryax October 1, 2024 15:26

rnishtala-sumo force-pushed the configure-compression-levels branch from 800a4c2 to 18d5c7b Compare October 1, 2024 15:32

dmitryax reviewed Oct 1, 2024

View reviewed changes

config/configcompression/compressiontype.go Outdated Show resolved Hide resolved

config/configcompression/compressiontype.go Outdated Show resolved Hide resolved

config/confighttp/compressor.go Outdated Show resolved Hide resolved

dmitryax reviewed Oct 1, 2024

View reviewed changes

rnishtala-sumo requested a review from dmitryax October 2, 2024 16:20

rnishtala-sumo force-pushed the configure-compression-levels branch from bfe8fb4 to 9ed1c64 Compare October 3, 2024 15:41

dmitryax reviewed Oct 4, 2024

View reviewed changes

config/confighttp/README.md Outdated Show resolved Hide resolved

config/confighttp/README.md Outdated Show resolved Hide resolved

config/configcompression/compressiontype.go Outdated Show resolved Hide resolved

rnishtala-sumo requested a review from dmitryax October 8, 2024 14:42

rnishtala-sumo force-pushed the configure-compression-levels branch from bc6ea67 to 62be46f Compare October 9, 2024 03:17

rnishtala-sumo force-pushed the configure-compression-levels branch from 62be46f to eaf3eea Compare October 11, 2024 16:16

dmitryax reviewed Oct 14, 2024

View reviewed changes

rnishtala-sumo added 8 commits October 16, 2024 12:51

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

44ddbee

Extending test coverage for compression types

f06ab76

Breaking changes that add support to configure compression levels

ce18148

Unmarshal compression type/level from configuration and extending tes…

2ad9719

…t coverage

Keep encoder pools reusable for compression

72d4f71

Changed the compression type in client configuration to be TypeWithLevel

cdbf393

Adding support for lz4 compression

6e2752b

Create a map of compressor pools for compressors that support varying…

c7f36ad

… levels

rnishtala-sumo force-pushed the configure-compression-levels branch from eaf3eea to c7f36ad Compare October 16, 2024 16:51

rnishtala-sumo requested a review from dmitryax October 16, 2024 16:55

Change the default config for the otlphttpexporter to use TypeWithLev…

427a8ed

…el for compression

rnishtala-sumo force-pushed the configure-compression-levels branch from ebb77fa to 427a8ed Compare October 16, 2024 17:50

Remove duplicate initialization of the compressor pools

70a79fd

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

rnishtala-sumo commented Sep 9, 2024 •

edited

Loading

codecov bot commented Sep 9, 2024 •

edited

Loading

rnishtala-sumo commented Sep 24, 2024

dmitryax Oct 1, 2024

rnishtala-sumo Oct 1, 2024

dmitryax Oct 1, 2024

rnishtala-sumo Oct 1, 2024

rnishtala-sumo Oct 3, 2024

rnishtala-sumo Oct 4, 2024 •

edited

Loading

rnishtala-sumo Oct 8, 2024

dmitryax Oct 14, 2024

rnishtala-sumo Oct 15, 2024

rnishtala-sumo Oct 16, 2024

dmitryax Oct 14, 2024

rnishtala-sumo Oct 15, 2024 •

edited

Loading

rnishtala-sumo Oct 16, 2024

dmitryax Oct 14, 2024

rnishtala-sumo Oct 15, 2024

rnishtala-sumo Oct 16, 2024


		return fmt.Errorf("unsupported compression type/level %s/%d", compressionTyp, ct.Level)

		if gZipCompressor.pool.Get() == nil {
		gZipCompressor.pool = sync.Pool{New: func() any { w, _ := gzip.NewWriterLevel(nil, int(compressionType.Level)); return w }}

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

Are you sure you want to change the base?

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

Conversation

rnishtala-sumo commented Sep 9, 2024 • edited Loading

Description

Link to tracking issue

Testing

Documentation

codecov bot commented Sep 9, 2024 • edited Loading

Codecov Report

rnishtala-sumo commented Sep 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rnishtala-sumo Oct 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rnishtala-sumo Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rnishtala-sumo commented Sep 9, 2024 •

edited

Loading

codecov bot commented Sep 9, 2024 •

edited

Loading

rnishtala-sumo Oct 4, 2024 •

edited

Loading

rnishtala-sumo Oct 15, 2024 •

edited

Loading