-
Notifications
You must be signed in to change notification settings - Fork 554
feat(OTLP): add zstd and gzip compression support to HTTP OTLP exporter #3092
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
base: main
Are you sure you want to change the base?
feat(OTLP): add zstd and gzip compression support to HTTP OTLP exporter #3092
Conversation
9b9cfed
to
c948ac8
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3092 +/- ##
=======================================
+ Coverage 80.1% 80.5% +0.4%
=======================================
Files 126 126
Lines 21957 22198 +241
=======================================
+ Hits 17603 17890 +287
+ Misses 4354 4308 -46 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
a174890
to
57ed869
Compare
95488f2
to
52938a4
Compare
} | ||
#[cfg(not(feature = "gzip-http"))] | ||
Some(crate::Compression::Gzip) => { | ||
Err("gzip compression requested but gzip-http feature not enabled".to_string()) |
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.
could this be detected and fail-fast at exporter build time itself?
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.
Pull Request Overview
This PR adds HTTP compression support (gzip and zstd) to the OTLP HTTP exporter, aligning it with the existing gRPC exporter capabilities. It implements compression for all signal types (traces, metrics, logs) and includes comprehensive test coverage.
- Adds optional gzip and zstd compression support via feature flags (
gzip-http
,zstd-http
) - Refactors compression resolution logic to be shared between HTTP and gRPC exporters
- Updates all HTTP export methods to handle compressed payloads with appropriate headers
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
opentelemetry-otlp/src/lib.rs | Updates imports to conditionally include WithTonicConfig |
opentelemetry-otlp/src/exporter/tonic/mod.rs | Refactors to use shared compression resolution logic |
opentelemetry-otlp/src/exporter/mod.rs | Adds shared compression resolution function |
opentelemetry-otlp/src/exporter/http/trace.rs | Updates trace exporter to handle compression headers |
opentelemetry-otlp/src/exporter/http/mod.rs | Core HTTP compression implementation and test coverage |
opentelemetry-otlp/src/exporter/http/metrics.rs | Updates metrics exporter for compression support |
opentelemetry-otlp/src/exporter/http/logs.rs | Updates logs exporter for compression support |
opentelemetry-otlp/examples/basic-otlp-http/src/main.rs | Example demonstrating compression usage |
opentelemetry-otlp/examples/basic-otlp-http/Cargo.toml | Adds compression feature flags to example |
opentelemetry-otlp/Cargo.toml | Adds compression dependencies and feature flags |
Comments suppressed due to low confidence (1)
opentelemetry-otlp/src/exporter/http/mod.rs:295
- The variable name
Compression
conflicts with the typecrate::Compression
. Consider usingflate2::Compression
or aliasing to avoid confusion.
let mut encoder = GzEncoder::new(Vec::new(), Compression::default());
//! use opentelemetry_otlp::{Protocol, WithExportConfig, WithTonicConfig}; | ||
//! use opentelemetry_otlp::{Protocol, WithExportConfig}; | ||
//! # #[cfg(feature = "grpc-tonic")] | ||
//! use opentelemetry_otlp::WithTonicConfig; |
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.
good catch! Wondering how our CI didn't catch this before!
.header(CONTENT_TYPE, content_type); | ||
|
||
if let Some(encoding) = content_encoding { | ||
request_builder = request_builder.header("Content-Encoding", encoding); |
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.
nit: Move Content-Encoding to const string, or is it available from http crate already?
@@ -50,6 +49,9 @@ pub struct HttpConfig { | |||
|
|||
/// Additional headers to send to the collector. | |||
headers: Option<HashMap<String, String>>, | |||
|
|||
/// The compression algorithm to use when communicating with the collector. |
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.
/// The compression algorithm to use when communicating with the collector. | |
/// The compression algorithm to use when communicating with the OTLP endpoint. |
}; | ||
|
||
let (compressed_body, content_encoding) = self.compress_body(body)?; | ||
Ok((compressed_body, content_type, content_encoding)) |
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.
nit: The variable gives the wrong impression that compressed_body is always compressed. But if Compression is None, then its just returning existing body as-is.
May be:
if (self.compression is None)
{
Ok(body,content_type,None)
}
else
{
call self.compress_body(...),
}
@@ -15,6 +15,8 @@ bench = false | |||
[features] | |||
default = ["reqwest-blocking"] | |||
reqwest-blocking = ["opentelemetry-otlp/reqwest-blocking-client"] | |||
gzip = ["opentelemetry-otlp/gzip-http"] |
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.
examples are currently kept basic.-showing absolute minimum required to work. I suggest to keep it that way in this PR. If we make a separate decision about improving these examples to show more scenarios, we can do it for this+ the gRPC examples too, in a separate PR.
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.
https://github.com/open-telemetry/opentelemetry-rust/blob/main/opentelemetry-otlp/src/lib.rs#L269-L271 The kitchensink would be a good place to showcase this.
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.
Looks good! Requesting changes just for the below:
- See if we can detect invalid/incorrect compression setting at build_exporter time itself and not in actual export. Eg; user enabled compression via env_variable, but the feature-flag is not enabled.
- Add a changelog as this is a new capability
- Lets keep example as-is in this PR.
Fixes #
#3083
Changes
Straightforward enough! None of our clients support this natively, so we have to do the compressing ourselves. Have copied the pattern used by the gRPC exporter in terms of features and API, and moved a bit of code around so we can share it.
Test coverage added, and have also fired up the OTLP example and pointed it at a collector with both compression formats.
Merge requirement checklist
CHANGELOG.md
files updated for non-trivial, user-facing changes