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: supports more compression methods for configgrpc #4088

Merged
merged 11 commits into from
Nov 5, 2021

Conversation

hyunuk
Copy link
Contributor

@hyunuk hyunuk commented Sep 22, 2021

Description:
This PR ensures that configgrpc supports more compression methods lz4, snappy and zstd, in addition to gzip.
It uses github.com/mostynb/go-grpc-compression, which is a wrapper for compression methods that are not supported by google.golang.org/grpc.

Link to tracking Issue:
#2548

@hyunuk hyunuk closed this Sep 22, 2021
@hyunuk hyunuk reopened this Sep 23, 2021
@hyunuk hyunuk marked this pull request as ready for review September 23, 2021 14:53
@hyunuk hyunuk requested review from a team and Aneurysm9 September 23, 2021 14:53
@Aneurysm9 Aneurysm9 linked an issue Sep 24, 2021 that may be closed by this pull request
Copy link
Member

@bogdandrutu bogdandrutu left a comment

Choose a reason for hiding this comment

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

Title and changelog is wrong since this is only for client side but not for server side as well.

@Aneurysm9
Copy link
Member

Title and changelog is wrong since this is only for client side but not for server side as well.

Simply including the additional compression packages makes them available for use by the server through content negotiation. From grpc.RegisterConpression:

will be automatically accessed when receiving a message based on the content coding header. Servers also use it to send a response with the same encoding as the request.

@alolita alolita added the ready-to-merge Code review completed; ready to merge by maintainers label Sep 28, 2021
CHANGELOG.md Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Oct 12, 2021

Codecov Report

Merging #4088 (b12782a) into main (cffbecb) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #4088   +/-   ##
=======================================
  Coverage   88.13%   88.13%           
=======================================
  Files         176      176           
  Lines       10425    10425           
=======================================
  Hits         9188     9188           
  Misses       1005     1005           
  Partials      232      232           
Impacted Files Coverage Δ
config/configgrpc/configgrpc.go 93.91% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cffbecb...b12782a. Read the comment docs.

@hyunuk hyunuk requested a review from bogdandrutu October 13, 2021 11:57
@codeboten
Copy link
Contributor

@bogdandrutu any followup items here?

@bogdandrutu
Copy link
Member

@codeboten this was discussed last SIG meeting, and it was asked to investigate/create an issue to investigate if the new dependency is the right one, or we should not depend on that third-party module.

@codeboten
Copy link
Contributor

@bogdandrutu ah I see, thanks for clarifying. It seems most other projects (including collector-contrib and jaeger) use the sarama library for the different compression options, should we move this PR in that direction?

Would that be enough to move the PR forward?

@Aneurysm9
Copy link
Member

@bogdandrutu ah I see, thanks for clarifying. It seems most other projects (including collector-contrib and jaeger) use the sarama library for the different compression options, should we move this PR in that direction?

Sarama is using the same implementations of zstd and lz4 as the dependency included here. That gives me some confidence in the stability of those implementations and the gRPC compression interface wrapping them is simple and easily reviewed and understood. I don't see a reason to not use this library.

@anuraaga
Copy link
Contributor

anuraaga commented Oct 28, 2021

@Aneurysm9 I think the main advantage could be more directly controlling the versions of those compression dependencies. zstd seems to be a couple minors behind, not such a big deal probably, but lz4 seems to actually be two majors behind and it's unclear whether that version is supported anymore by the project. Actually, I noticed this useful-looking commit in the latest version from our own @lizthegrey pierrec/lz4@9f82ba4

The simpleness of the gRPC interfaces means on the flip side that implementing here isn't that hard really, but can ensure better that we're on the latest version of these libraries.

@hyunuk
Copy link
Contributor Author

hyunuk commented Oct 28, 2021

ah I see, thanks for clarifying. It seems most other projects (including collector-contrib and jaeger) use the sarama library for the different compression options, should we move this PR in that direction?

Would that be enough to move the PR forward?

Sarama is a Go client for Kafka, so all of usage in collector-contrib and jaeger is related to Kafka, not for gRPC configuration.Only kafkareceiver and kafkaexporter is using sarama library in collector-contrib.

To use different compression methods in gRPC, each of them should implement the compressor interface. That's why I'm using a wrapper library in this PR.

@lizthegrey
Copy link
Member

Honeycomb upgraded from v2 to v3 of lz4 seamlessly but v4 has breaking changes we weren't ready to take yet.

@bogdandrutu bogdandrutu merged commit 6bd94b1 into open-telemetry:main Nov 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge Code review completed; ready to merge by maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

configgrpc.go should support more encoding/compressions
8 participants