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

importing opentelemetry-collector clobbers previously registered gRPC compressors #7920

Closed
mostynb opened this issue Jun 16, 2023 · 2 comments
Labels
bug Something isn't working

Comments

@mostynb
Copy link
Contributor

mostynb commented Jun 16, 2023

Describe the bug

Some consumers of this library have registered grpc compressors, and the init() order can cause their compressors to be clobbered by opentelemetry-collector's use of github.com/mostynb/go-grpc-compression, which follows the pattern of https://github.com/grpc/grpc-go/blob/master/encoding/gzip/gzip.go (overwrite any previously registered encoders with the same name).

Steps to reproduce

  1. Write a program that uses a specific zstd grpc compressor implementation (that isn't github.com/mostynb/go-grpc-compression/zstd).
  2. Add opentelemetry-collector to that program.
  3. Note that the zstd grpc compressor is now github.com/mostynb/go-grpc-compression/zstd, depending on init() ordering

What did you expect to see?
The programs original grpc compressor should be used.

What did you see instead?
github.com/mostynb/go-grpc-compression/zstd is used instead of the preferred compressor.

What version did you use?
Any version of opentelemetry-collector that uses github.com/mostynb/go-grpc-compression/* should be affected.

Proposed solution
mostynb/go-grpc-compression#19 adds "nonclobbering" versions of each compressor. If I were to land that and make a new release, we could change all opentelemetry-collector imports of github.com/mostynb/go-grpc-compression/* to the corresponding github.com/mostynb/go-grpc-compression/nonclobbering/* package.

If you would accept such a change, I will land that PR in github.com/mostynb/go-grpc-compression (reviews of the PR above are welcome too).

@mx-psi
Copy link
Member

mx-psi commented Jun 26, 2023

Hi @mostynb thanks for taking the time to open the issue. I think this is a reasonable solution, cc @open-telemetry/collector-approvers

codeboten pushed a commit that referenced this issue Jun 29, 2023
…ering imports (#7966)

**Description:** Switch to grpc encoders that won't override the
consumer's previously registered codecs.

Some consumers of this library have registered grpc compressors, and the
init() order can cause their compressors to be clobbered by
opentelemetry-collector's use of github.com/mostynb/go-grpc-compression,
which follows the pattern from the grpc-go library (overwrite any
previously registered encoders with the same name).

Version 1.2.0 of github.com/mostynb/go-grpc-compression added so-called
"nonclobbering" imports, which only register the codec if there is not
already a codec registered with that name. This PR switches to use
those.

**Link to tracking Issue:** #7920

**Testing:** tests have been added to the upstream library.
@codeboten
Copy link
Contributor

Fixed by #7966

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants