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

[exporter/awss3] Add compression option (27872) #31622

Merged
merged 5 commits into from
Mar 9, 2024

Conversation

pepperkick
Copy link
Contributor

Description:
Add compression option to compress files using compress/gzip library before uploading to S3.

Link to tracking Issue:
Fixes #27872

Testing:

Sent n number of traces through the S3 exporter using k6 to compare sizes. Used Minio as the S3 backend.

Marshaler Compression k6 Requests k6 Data Sent S3 Objects S3 Total Size
otlp_json No 101 118 KB 101 36 KB
otlp_proto No 101 118 KB 101 11 KB
otlp_json Yes 101 118 KB 101 21 KB
otlp_proto Yes 101 118 KB 101 9.9 KB

Additionally, new unit test to check file name.

Documentation:

  • Updated README.md file

@pepperkick pepperkick requested a review from atoulme as a code owner March 6, 2024 11:30
@pepperkick pepperkick requested a review from a team March 6, 2024 11:30
Copy link

linux-foundation-easycla bot commented Mar 6, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@github-actions github-actions bot requested a review from pdelewski March 6, 2024 11:30
@pepperkick pepperkick changed the title [exporter/awss3exporter] Add compression option (27872) [exporter/awss3] Add compression option (27872) Mar 6, 2024
@atoulme
Copy link
Contributor

atoulme commented Mar 6, 2024

Please take a look at the failing build.

Do you expect the only compression will be .gz only? Could we have multiple types of compression?

@pepperkick
Copy link
Contributor Author

Please take a look at the failing build.

=== Failed
make[2]: *** [../../Makefile.Common:131: test-with-cover] Error 3
=== FAIL: . TestComponentLifecycle/metrics-shutdown (0.00s)
    generated_component_test.go:50: 
        	Error Trace:	/home/runner/work/opentelemetry-collector-contrib/opentelemetry-collector-contrib/receiver/opencensusreceiver/generated_component_test.go:50
        	Error:      	Received unexpected error:
make[1]: *** [Makefile:165: receiver/opencensusreceiver] Error 2
make: *** [Makefile:117: gotest-with-cover] Error 2
        	            	failed to bind to address "0.0.0.0:55678": listen tcp 0.0.0.0:55678: bind: address already in use
        	Test:       	TestComponentLifecycle/metrics-shutdown

=== FAIL: . TestComponentLifecycle/traces-shutdown (0.00s)

=== FAIL: . TestComponentLifecycle (0.00s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x129e5fc]

Seems like there is some port conflict for opencensusreceiver in CI. Not sure but maybe parallel builds?

Do you expect the only compression will be .gz only? Could we have multiple types of compression?

Good question, the issue only mentioned gzip and it is most used. Only other I can think of is zip compression via archive/zip library.
I can make the compression enum type similar to the marshaler field so new options can be added in future. My concern in this case would be appending the file extension for various types.

@atoulme
Copy link
Contributor

atoulme commented Mar 7, 2024

sorry for the leading question, I meant we can use the configcompression.Type type: https://github.com/open-telemetry/opentelemetry-collector/blob/main/config/configcompression/compressiontype.go

I will look at the build and file an issue.

@pepperkick
Copy link
Contributor Author

pepperkick commented Mar 7, 2024

sorry for the leading question, I meant we can use the configcompression.Type type: https://github.com/open-telemetry/opentelemetry-collector/blob/main/config/configcompression/compressiontype.go

Updated code to reuse the configcompression.Type. Was not aware of this!

I will look at the build and file an issue.

Thanks! 😄

@pepperkick pepperkick force-pushed the awss3-exporter-compression branch 2 times, most recently from 5e82ce6 to ba202ad Compare March 8, 2024 22:03
@atoulme atoulme added the ready to merge Code review completed; ready to merge by maintainers label Mar 9, 2024
@dmitryax dmitryax merged commit 7ecee0e into open-telemetry:main Mar 9, 2024
147 checks passed
@github-actions github-actions bot added this to the next release milestone Mar 9, 2024
DougManton pushed a commit to DougManton/opentelemetry-collector-contrib that referenced this pull request Mar 13, 2024
**Description:**
Add `compression` option to compress files using `compress/gzip` library
before uploading to S3.

**Link to tracking Issue:**
Fixes
open-telemetry#27872

**Testing:** 

Sent n number of traces through the S3 exporter using k6 to compare
sizes. Used Minio as the S3 backend.
| Marshaler | Compression | k6 Requests | k6 Data Sent | S3 Objects | S3
Total Size |
| --- | --- | --- | --- | --- | --- |
| otlp_json | No | 101 | 118 KB | 101 | 36 KB | 
| otlp_proto | No | 101 | 118 KB | 101 | 11 KB | 
| otlp_json | Yes | 101 | 118 KB | 101 | 21 KB | 
| otlp_proto | Yes | 101 | 118 KB | 101 | 9.9 KB | 

Additionally, new unit test to check file name.

**Documentation:**
- Updated README.md file
XinRanZhAWS pushed a commit to XinRanZhAWS/opentelemetry-collector-contrib that referenced this pull request Mar 13, 2024
**Description:**
Add `compression` option to compress files using `compress/gzip` library
before uploading to S3.

**Link to tracking Issue:**
Fixes
open-telemetry#27872

**Testing:** 

Sent n number of traces through the S3 exporter using k6 to compare
sizes. Used Minio as the S3 backend.
| Marshaler | Compression | k6 Requests | k6 Data Sent | S3 Objects | S3
Total Size |
| --- | --- | --- | --- | --- | --- |
| otlp_json | No | 101 | 118 KB | 101 | 36 KB | 
| otlp_proto | No | 101 | 118 KB | 101 | 11 KB | 
| otlp_json | Yes | 101 | 118 KB | 101 | 21 KB | 
| otlp_proto | Yes | 101 | 118 KB | 101 | 9.9 KB | 

Additionally, new unit test to check file name.

**Documentation:**
- Updated README.md file
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
exporter/awss3 ready to merge Code review completed; ready to merge by maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

awss3export to support gzip
4 participants