Add support for compression options in Kafka exporter #63

Saber-W · 2021-05-24T20:04:26Z

Description:
The Kafka exporter currently doesn't support any compression method, which would cause distress to some users. This PR adds support for four compression options(gzip, snappy, lz4 and zstd) to the Kafka exporter, along with the compression level as requested in the issue.

Link to tracking Issue:
#3223

Testing:
The related exsiting unit tests in Kafka expoter have been updated. A new unit test compression_test.go is added.

exporter/kafkaexporter/compression_test.go

exporter/kafkaexporter/README.md

Aneurysm9 · 2021-05-24T20:25:46Z

exporter/kafkaexporter/README.md

@@ -61,6 +61,8 @@ The following settings can be optionally configured:
  User should calculate this as `num_seconds * requests_per_second` where:
    - `num_seconds` is the number of seconds to buffer in case of a backend outage
    - `requests_per_second` is the average number of requests per seconds.
+- `compression` (default = none): The compression codec to compress the messages sent to Kafka. Support four compression methods: `gzip`, `snappy`, `lz4` and `zstd`.
+- `compression_level` (default = -1000): The corresponding compression level.


What does the compression_level value mean? Is there a scale? Is it the same for all supported compression methods? Does a lower value indicate more compression, or a higher value? What is the range of this scale?

What does the compression_level value mean? Is there a scale?

Yes, it is a scale. It measures the compression degree to the files.

Is it the same for all supported compression methods?

No, each compression method has different range. For example, the valid range for gzip is 1 - 9.

Does a lower value indicate more compression, or a higher value?

Higher value indicates more compression.

What is the range of this scale?

The valid range for each method is as below:
gzip: 1 - 9
snappy: do not support
lz4: 1 - 17
zstd: 1 - 22

I would put all these info into README. Thanks!

exporter/kafkaexporter/compression.go

Aneurysm9 · 2021-05-24T20:28:43Z

exporter/kafkaexporter/compression.go

+	"github.com/Shopify/sarama"
+)
+
+func ConfigureCompression(compression string, saramaConfig *sarama.Config) error {


Does this need to be exported? If so, it needs godoc describing its use. It probably doesn't, though, and thus should be unexported.

I also don't see why this would be exported, but I saw that ConfigureAuthentication is also an exported method. Not sure why that would be, and if the same reasons would apply to ConfigureCompression?

Yeah I agree it doesn't need to - will do!

I also don't see why this would be exported, but I saw that ConfigureAuthentication is also an exported method. Not sure why that would be, and if the same reasons would apply to ConfigureCompression?

ConfigureAuthentication is exported because it is called by the Kafka receiver. I think configureCompression doesn't need to at this point.

exporter/kafkaexporter/config.go

eddyleelin

Nice job, this is really, really cool!! One question: why did you decide to make compression and compression_level separate settings? Wouldn't it have been better to have it be nested, like

exporters:
  kafka:
    compression:
      codec: zstd
      level: 6

rather than

exporters:
  kafka:
    compression: zstd
    compression_level: 6

Good work again! 👍

exporter/kafkaexporter/config.go

eddyleelin · 2021-05-24T20:27:42Z

exporter/kafkaexporter/config_test.go

+		CompressionCodec: defaultCompression,
+		CompressionLevel: defaultCompressionLevel,


Maybe it would be good to try some non-default values for these in the testdata config?

Thanks!

why did you decide to make compression and compression_level separate settings?

The Kafka exporter basically uses sarama as the producer to produce messages to Kafka. So I think it would be the best to follow the sarama's specification about the compression options.

Maybe it would be good to try some non-default values for these in the testdata config?

Yes, I should add more tests.

eddyleelin · 2021-05-24T20:54:41Z

exporter/kafkaexporter/kafka_exporter_test.go

+		CompressionCodec: defaultCompression,
+		CompressionLevel: defaultCompressionLevel,


Are these needed in this test? I couldn't find where it would change anything being tested here?

Yes, this test will fail if not.

exporter/kafkaexporter/README.md

eddyleelin · 2021-05-25T18:26:21Z

exporter/kafkaexporter/README.md

+- `compression_level` (default = -1000): The corresponding compression level which measures the compression degree.
+  Lower compression level would lead to fast compression as well as relatively big file size. The valid ranges for each compression method are:
+  - `gzip`: `1 - 9`
+  - `snappy`: Snappy does not support compression level.


Would this be more clear?

Suggested change

- `snappy`: Snappy does not support compression level.

- `snappy`: Snappy does not support custom compression level.

Signed-off-by: Anthony J Mirabella <a9@aneurysm9.com> Co-authored-by: Juraci Paixão Kröhling <juraci@kroehling.de>

* Initial commit * Add CODEOWNERS file (#2) * Add CODEOWNERS file * Update CODEOWNERS * Moved from github.com/observatorium/opentelemetry-collector-builder (#3) Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de> * fixed panics (#6) Signed-off-by: Joe Elliott <number101010@gmail.com> * Replace master with main in CI and mergify files (#8) Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de> * Bump to OpenTelemetry Collector 0.20.0 (#10) Closes #9 Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de> * Explicitly enable Go modules in quickstart instructions (#13) * Update to collector v0.21.0 (#17) Fixes #16 Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de> * Update to collector v0.22.0 (#19) * Download go modules before building (#20) Fixes #14 * Add version command (#25) Signed-off-by: Ashmita Bohara <ashmita.bohara152@gmail.com> * Pass errors from cobra Execute back to main for correct exit code (#28) * pass errors from cobra execute back to main * print the error * Update to collector v0.23.0 (#27) * Generate a warning if the builder and collector base version mismatch (#30) * Generate a warning if the builder and collector base version mismatch * Show current default version in the warning message * Update to OpenTelemetry Collector 0.24.0 * Don't use %w formatting with log.Fatal (#35) * Update to OpenTelemetry Collector 0.25.0 (#36) Signed-off-by: Serge Catudal <serge.catudal@gmail.com> * Update to 0.26.0 and update BuildInfo (#39) * Sync build and CI Go versions at latest 1.16 (#34) * Sync build and CI Go versions at latest 1.16 * Run go mod tidy * Set go binary to use in the compilation phase in tests Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de> Co-authored-by: Juraci Paixão Kröhling <juraci@kroehling.de> * Add option to generate go code only (no compile) (#40) * Issue#24 Add option to generate go code only (no compile) * Update cmd/root.go logging Suggested by @jpkkrohling Co-authored-by: Juraci Paixão Kröhling <juraci.github@kroehling.de> * remove verbose help .. created by corba * suggestion by jpkrohling to keep generateandcompile * lint error: remove unused var * reword cmd option and add back help message for default Co-authored-by: Juraci Paixão Kröhling <juraci.github@kroehling.de> * Don't reuse exec.Cmd (#42) * Update to OpenTelemetry Collector 0.27.0 (#43) * Add CI Badge (#47) * Update to Collector v0.28.0 (#49) * Update to Collector v0.28.0 Closes #48 Addresses the breaking API change in open-telemetry#3163, besides the usual version number changes. Signed-off-by: Fangyi Zhou <me@fangyi.io> * Use `go mod tidy` instead of `go mod download` It appears that this magically resolves the go.mod file issue. https://stackoverflow.com/questions/67203641/missing-go-sum-entry-for-module-providing-package-package-name Signed-off-by: Fangyi Zhou <me@fangyi.io> * Account for go mod download in go1.17 not updating go.sum (#50) * Update to collector v0.29.0 (#54) * Update replaces.builder.yaml * Update nocore.builder.yaml * Update config.go * Update README.md * Update main.go * Update to collector v0.30.0 (#57) * cmd: fix module flag default value to github.com/open-telemetry (#58) Signed-off-by: Koichi Shiraishi <zchee.io@gmail.com> * Update to collector v0.31.0 (#60) * Update to v0.33.0 (#62) Signed-off-by: Anthony J Mirabella <a9@aneurysm9.com> * Add excludes support to generated go.mod (#63) Signed-off-by: Anthony J Mirabella <a9@aneurysm9.com> Co-authored-by: Juraci Paixão Kröhling <juraci@kroehling.de> * Small cleanup for the builder files (#64) Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com> * Support building with Go 1.17 (#66) * Support building with Go 1.17 Fixes #65 Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de> * Update workflows to use Go 1.17 Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de> * Add gosec exceptions for exec.Command Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de> * Update to OpenTelemetry core 0.34.0 (#68) Fixes #67 Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de> * Upgrade to OpenTelemetry Collector 0.35.0 (#70) Signed-off-by: Fangyi Zhou <me@fangyi.io> * Upgrade to OpenTelemetry Collector 0.36.0 (#76) * Generate custom service code for Windows (#75) * update main to include windows service code * use main version from tag 0.35.0 * update main function * align with upstream v0.36.0 tag * dummy change to trigger build * Revert "dummy change to trigger build" This reverts commit 629d499461da2d2c240bf1e495b5fe0558e3547f. * Remove Core from Module type (#77) Fixes #15 Signed-off-by: yugo-horie <u5.horie@gmail.com> * release 0.37.0 (#78) * release 0.37.0 * update use of NewCommand * Move builder to subdirectory Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de> Co-authored-by: Bogdan Drutu <lazy@splunk.com> Co-authored-by: Bogdan Drutu <bogdandrutu@gmail.com> Co-authored-by: Joe Elliott <joe.elliott@grafana.com> Co-authored-by: Eric Yang <jiwen624@gmail.com> Co-authored-by: Brian Gibbins <eroteme@supernought.co.uk> Co-authored-by: Ashmita <ashmita.bohara152@gmail.com> Co-authored-by: Fangyi Zhou <me@fangyi.io> Co-authored-by: Shaun Creary <65406540+crearys@users.noreply.github.com> Co-authored-by: Patryk Małek <69143962+pmalek-sumo@users.noreply.github.com> Co-authored-by: Serge Catudal <serge.catudal@gmail.com> Co-authored-by: Aaron Stone <aaron@serendipity.cx> Co-authored-by: Patryk Małek <pmalek@sumologic.com> Co-authored-by: Aaron Stone <aaron.stone@udacity.com> Co-authored-by: Kelvin Lo <kello@live.ca> Co-authored-by: Himanshu <addyjeridiq@gmail.com> Co-authored-by: Y.Horie <u5.horie@gmail.com> Co-authored-by: Koichi Shiraishi <zchee.io@gmail.com> Co-authored-by: Anthony Mirabella <a9@aneurysm9.com> Co-authored-by: Cal Loomis <68860480+loomis-relativity@users.noreply.github.com> Co-authored-by: alrex <aboten@lightstep.com>

Shibo Wang added 2 commits May 24, 2021 11:37

Add support for compression options in Kafka exporter

7243ee5

Fix bugs to pass all tests

648c741

Saber-W requested review from alolita and Aneurysm9 May 24, 2021 20:04

Saber-W changed the title ~~Compression kafka exporter~~ Add support for compression options in Kafka exporter May 24, 2021

eddyleelin reviewed May 24, 2021

View reviewed changes

exporter/kafkaexporter/compression_test.go Show resolved Hide resolved

Aneurysm9 reviewed May 24, 2021

View reviewed changes

eddyleelin reviewed May 24, 2021

View reviewed changes

Fix grammer bugs, add unit tests and usage specification in README

7d1d6ec

Aneurysm9 approved these changes May 24, 2021

View reviewed changes

exporter/kafkaexporter/README.md Outdated Show resolved Hide resolved

alolita approved these changes May 25, 2021

View reviewed changes

Fix wrong grammar usage

4a8ae60

Saber-W closed this May 25, 2021

eddyleelin reviewed May 25, 2021

View reviewed changes

JamesJHPark pushed a commit that referenced this pull request Nov 3, 2021

Add excludes support to generated go.mod (#63)

6689c36

Signed-off-by: Anthony J Mirabella <a9@aneurysm9.com> Co-authored-by: Juraci Paixão Kröhling <juraci@kroehling.de>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for compression options in Kafka exporter #63

Add support for compression options in Kafka exporter #63

Saber-W commented May 24, 2021

Aneurysm9 May 24, 2021

Saber-W May 24, 2021

Aneurysm9 May 24, 2021

eddyleelin May 24, 2021

Saber-W May 24, 2021

Saber-W May 24, 2021

eddyleelin left a comment •

edited

Loading

eddyleelin May 24, 2021

Saber-W May 24, 2021

eddyleelin May 24, 2021

Saber-W May 24, 2021

eddyleelin May 25, 2021 •

edited

Loading

		CompressionCodec: defaultCompression,
		CompressionLevel: defaultCompressionLevel,

	- `snappy`: Snappy does not support compression level.
	- `snappy`: Snappy does not support custom compression level.

Add support for compression options in Kafka exporter #63

Add support for compression options in Kafka exporter #63

Conversation

Saber-W commented May 24, 2021

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

eddyleelin left a comment • 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

eddyleelin May 25, 2021 • edited Loading

Choose a reason for hiding this comment

eddyleelin left a comment •

edited

Loading

eddyleelin May 25, 2021 •

edited

Loading