-
Notifications
You must be signed in to change notification settings - Fork 130
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
Add zstd compressor implementation for OTLP exporters #1108
Conversation
5ad5869
to
7215363
Compare
@Override | ||
public String getEncoding() { | ||
return "zstd"; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(unrelated to this PR) was there reason for this method to be in Compressor
and not CompressorProvider
(e.g. similar to ConfigurableSpanExporterProvider.getName()
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah so this is a good question which I encountered implementing. My thinking:
- We want some sort of name that we can reference when we call
setCompression(String)
on the exporters - We also want a value to use for the
Content-Encoding
header - I think it will always be sufficient to use the same name for
setCompression
andContent-Encoding
.
By the time the Compressor is resolved from CompressorProvider, the compressor wouldn't be able to access a CompressorProvider#getName
any more. So my thinking was, put the identifier down in Compressor instead of CompressorProvider, so its always accessible to the Compressor where its needed for Content-Encoding
. Count on the fact that CompressorProvider#getInstance()
should return singletons, and therefore you can call CompressorProvider.getInstance().getName()
to get the name identifier of the CompressorProvider
.
I also thought about how ContextStorageProvider also doesn't have a #getName()
method, since its identified by its FQCN for configuration. Compressor is different because we definitely want to be able to use a short name instead of FQCN, but that's a little bit of the prior art and what I was thinking about.
Implementation of the
Compressor
SPI I propose in open-telemetry/opentelemetry-java#5990This PR adds support for a zstd compression, which is one of the algorithms supported by the collector: https://github.com/open-telemetry/opentelemetry-collector/blob/73fa163951aeee538bd42f561d3a96205ee251d0/config/configgrpc/README.md
I think compressors SPIs like this belong outside the core repo because they're not explicitly specified, and there may be disagreement about how to package up the code which performs the compression. For example, this artifact proposed in this PR has a dependency on
com.github.luben:zstd-jni:1.5.5-10
, which uses JNI to improve performance. Some might prefer a pure java version, or prefer the dependency be shaded. I want to provide something people can easily use, without providing a solution for every combination of user requirements (users can always provide their own, quite easily).