Skip to content

Commit 16767a0

Browse files
scottgerringscottgerring
committed
address review comments
1 parent 52938a4 commit 16767a0

File tree

5 files changed

+104
-85
lines changed

5 files changed

+104
-85
lines changed

‎opentelemetry-otlp/examples/basic-otlp-http/Cargo.toml‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,8 +15,6 @@ bench = false
1515
[features]
1616
default = ["reqwest-blocking"]
1717
reqwest-blocking = ["opentelemetry-otlp/reqwest-blocking-client"]
18-
gzip = ["opentelemetry-otlp/gzip-http"]
19-
zstd = ["opentelemetry-otlp/zstd-http"]
2018

2119
[dependencies]
2220
opentelemetry = { path = "../../../opentelemetry" }

‎opentelemetry-otlp/examples/basic-otlp-http/src/main.rs‎

Lines changed: 6 additions & 68 deletions
Original file line numberDiff line numberDiff line change
@@ -5,8 +5,6 @@ use opentelemetry::{
55
};
66
use opentelemetry_appender_tracing::layer::OpenTelemetryTracingBridge;
77
use opentelemetry_otlp::WithExportConfig;
8-
#[cfg(any(feature = "gzip", feature = "zstd"))]
9-
use opentelemetry_otlp::{Compression, WithHttpConfig};
108
use opentelemetry_otlp::{LogExporter, MetricExporter, Protocol, SpanExporter};
119
use opentelemetry_sdk::Resource;
1210
use opentelemetry_sdk::{
@@ -29,29 +27,9 @@ fn get_resource() -> Resource {
2927
}
3028

3129
fn init_logs() -> SdkLoggerProvider {
32-
#[cfg(any(feature = "gzip", feature = "zstd"))]
33-
let mut exporter_builder = LogExporter::builder()
30+
let exporter = LogExporter::builder()
3431
.with_http()
35-
.with_protocol(Protocol::HttpBinary);
36-
37-
#[cfg(not(any(feature = "gzip", feature = "zstd")))]
38-
let exporter_builder = LogExporter::builder()
39-
.with_http()
40-
.with_protocol(Protocol::HttpBinary);
41-
42-
#[cfg(feature = "gzip")]
43-
{
44-
exporter_builder = exporter_builder.with_compression(Compression::Gzip);
45-
println!("Using gzip compression for logs");
46-
}
47-
48-
#[cfg(all(feature = "zstd", not(feature = "gzip")))]
49-
{
50-
exporter_builder = exporter_builder.with_compression(Compression::Zstd);
51-
println!("Using zstd compression for logs");
52-
}
53-
54-
let exporter = exporter_builder
32+
.with_protocol(Protocol::HttpBinary)
5533
.build()
5634
.expect("Failed to create log exporter");
5735

@@ -62,29 +40,9 @@ fn init_logs() -> SdkLoggerProvider {
6240
}
6341

6442
fn init_traces() -> SdkTracerProvider {
65-
#[cfg(any(feature = "gzip", feature = "zstd"))]
66-
let mut exporter_builder = SpanExporter::builder()
67-
.with_http()
68-
.with_protocol(Protocol::HttpBinary); //can be changed to `Protocol::HttpJson` to export in JSON format
69-
70-
#[cfg(not(any(feature = "gzip", feature = "zstd")))]
71-
let exporter_builder = SpanExporter::builder()
43+
let exporter = SpanExporter::builder()
7244
.with_http()
73-
.with_protocol(Protocol::HttpBinary); //can be changed to `Protocol::HttpJson` to export in JSON format
74-
75-
#[cfg(feature = "gzip")]
76-
{
77-
exporter_builder = exporter_builder.with_compression(Compression::Gzip);
78-
println!("Using gzip compression for traces");
79-
}
80-
81-
#[cfg(all(feature = "zstd", not(feature = "gzip")))]
82-
{
83-
exporter_builder = exporter_builder.with_compression(Compression::Zstd);
84-
println!("Using zstd compression for traces");
85-
}
86-
87-
let exporter = exporter_builder
45+
.with_protocol(Protocol::HttpBinary) //can be changed to `Protocol::HttpJson` to export in JSON format
8846
.build()
8947
.expect("Failed to create trace exporter");
9048

@@ -95,29 +53,9 @@ fn init_traces() -> SdkTracerProvider {
9553
}
9654

9755
fn init_metrics() -> SdkMeterProvider {
98-
#[cfg(any(feature = "gzip", feature = "zstd"))]
99-
let mut exporter_builder = MetricExporter::builder()
100-
.with_http()
101-
.with_protocol(Protocol::HttpBinary); //can be changed to `Protocol::HttpJson` to export in JSON format
102-
103-
#[cfg(not(any(feature = "gzip", feature = "zstd")))]
104-
let exporter_builder = MetricExporter::builder()
56+
let exporter = MetricExporter::builder()
10557
.with_http()
106-
.with_protocol(Protocol::HttpBinary); //can be changed to `Protocol::HttpJson` to export in JSON format
107-
108-
#[cfg(feature = "gzip")]
109-
{
110-
exporter_builder = exporter_builder.with_compression(Compression::Gzip);
111-
println!("Using gzip compression for metrics");
112-
}
113-
114-
#[cfg(all(feature = "zstd", not(feature = "gzip")))]
115-
{
116-
exporter_builder = exporter_builder.with_compression(Compression::Zstd);
117-
println!("Using zstd compression for metrics");
118-
}
119-
120-
let exporter = exporter_builder
58+
.with_protocol(Protocol::HttpBinary) //can be changed to `Protocol::HttpJson` to export in JSON format
12159
.build()
12260
.expect("Failed to create metric exporter");
12361

‎opentelemetry-otlp/src/exporter/http/logs.rs‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ use opentelemetry::otel_debug;
44
use opentelemetry_sdk::error::{OTelSdkError, OTelSdkResult};
55
use opentelemetry_sdk::logs::{LogBatch, LogExporter};
66
use std::time;
7+
use http::header::CONTENT_ENCODING;
78

89
impl LogExporter for OtlpHttpClient {
910
async fn export(&self, batch: LogBatch<'_>) -> OTelSdkResult {
@@ -24,7 +25,7 @@ impl LogExporter for OtlpHttpClient {
2425
.header(CONTENT_TYPE, content_type);
2526

2627
if let Some(encoding) = content_encoding {
27-
request_builder = request_builder.header("Content-Encoding", encoding);
28+
request_builder = request_builder.header(CONTENT_ENCODING, encoding);
2829
}
2930

3031
let mut request = request_builder

‎opentelemetry-otlp/src/exporter/http/mod.rs‎

Lines changed: 65 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -47,10 +47,10 @@ pub struct HttpConfig {
4747
/// Select the HTTP client
4848
client: Option<Arc<dyn HttpClient>>,
4949

50-
/// Additional headers to send to the collector.
50+
/// Additional headers to send to the OTLP endpoint.
5151
headers: Option<HashMap<String, String>>,
5252

53-
/// The compression algorithm to use when communicating with the collector.
53+
/// The compression algorithm to use when communicating with the OTLP endpoint.
5454
compression: Option<crate::Compression>,
5555
}
5656

@@ -119,6 +119,28 @@ impl HttpExporterBuilder {
119119

120120
let compression = self.resolve_compression(signal_compression_var)?;
121121

122+
// Validate compression is supported at build time
123+
if let Some(compression_alg) = &compression {
124+
match compression_alg {
125+
crate::Compression::Gzip => {
126+
#[cfg(not(feature = "gzip-http"))]
127+
{
128+
return Err(ExporterBuildError::UnsupportedCompressionAlgorithm(
129+
"gzip compression requested but gzip-http feature not enabled".to_string(),
130+
));
131+
}
132+
}
133+
crate::Compression::Zstd => {
134+
#[cfg(not(feature = "zstd-http"))]
135+
{
136+
return Err(ExporterBuildError::UnsupportedCompressionAlgorithm(
137+
"zstd compression requested but zstd-http feature not enabled".to_string(),
138+
));
139+
}
140+
}
141+
}
142+
}
143+
122144
let timeout = resolve_timeout(signal_timeout_var, self.exporter_config.timeout.as_ref());
123145

124146
#[allow(unused_mut)] // TODO - clippy thinks mut is not needed, but it is
@@ -284,8 +306,10 @@ pub(crate) struct OtlpHttpClient {
284306
}
285307

286308
impl OtlpHttpClient {
287-
/// Compress data using gzip if compression is enabled and the feature is available
288-
fn compress_body(&self, body: Vec<u8>) -> Result<(Vec<u8>, Option<&'static str>), String> {
309+
/// Compress data using gzip or zstd if the user has requested it and the relevant feature
310+
/// has been enabled. If the user has requested it but the feature has not been enabled,
311+
/// we should catch this at exporter build time and never get here.
312+
fn process_body(&self, body: Vec<u8>) -> Result<(Vec<u8>, Option<&'static str>), String> {
289313
match self.compression {
290314
#[cfg(feature = "gzip-http")]
291315
Some(crate::Compression::Gzip) => {
@@ -352,8 +376,8 @@ impl OtlpHttpClient {
352376
_ => (req.encode_to_vec(), "application/x-protobuf"),
353377
};
354378

355-
let (compressed_body, content_encoding) = self.compress_body(body)?;
356-
Ok((compressed_body, content_type, content_encoding))
379+
let (processed_body, content_encoding) = self.process_body(body)?;
380+
Ok((processed_body, content_type, content_encoding))
357381
}
358382

359383
#[cfg(feature = "logs")]
@@ -374,7 +398,7 @@ impl OtlpHttpClient {
374398
_ => (req.encode_to_vec(), "application/x-protobuf"),
375399
};
376400

377-
let (compressed_body, content_encoding) = self.compress_body(body)?;
401+
let (compressed_body, content_encoding) = self.process_body(body)?;
378402
Ok((compressed_body, content_type, content_encoding))
379403
}
380404

@@ -399,7 +423,7 @@ impl OtlpHttpClient {
399423
_ => (req.encode_to_vec(), "application/x-protobuf"),
400424
};
401425

402-
match self.compress_body(body) {
426+
match self.process_body(body) {
403427
Ok((compressed_body, content_encoding)) => {
404428
Some((compressed_body, content_type, content_encoding))
405429
}
@@ -839,7 +863,7 @@ mod tests {
839863

840864
// Test with some sample data
841865
let test_data = b"Hello, world! This is test data for compression.";
842-
let result = client.compress_body(test_data.to_vec()).unwrap();
866+
let result = client.process_body(test_data.to_vec()).unwrap();
843867
let (compressed_body, content_encoding) = result;
844868

845869
// Verify encoding header is set
@@ -870,7 +894,7 @@ mod tests {
870894

871895
// Test with some sample data
872896
let test_data = b"Hello, world! This is test data for zstd compression.";
873-
let result = client.compress_body(test_data.to_vec()).unwrap();
897+
let result = client.process_body(test_data.to_vec()).unwrap();
874898
let (compressed_body, content_encoding) = result;
875899

876900
// Verify encoding header is set
@@ -897,7 +921,7 @@ mod tests {
897921
);
898922

899923
let body = vec![1, 2, 3, 4];
900-
let result = client.compress_body(body.clone()).unwrap();
924+
let result = client.process_body(body.clone()).unwrap();
901925
let (result_body, content_encoding) = result;
902926

903927
// Body should be unchanged and no encoding header
@@ -918,7 +942,7 @@ mod tests {
918942
);
919943

920944
let body = vec![1, 2, 3, 4];
921-
let result = client.compress_body(body);
945+
let result = client.process_body(body);
922946

923947
// Should return error when gzip requested but feature not enabled
924948
assert!(result.is_err());
@@ -940,7 +964,7 @@ mod tests {
940964
);
941965

942966
let body = vec![1, 2, 3, 4];
943-
let result = client.compress_body(body);
967+
let result = client.process_body(body);
944968

945969
// Should return error when zstd requested but feature not enabled
946970
assert!(result.is_err());
@@ -1199,5 +1223,33 @@ mod tests {
11991223
},
12001224
);
12011225
}
1226+
1227+
#[cfg(all(feature = "trace", not(feature = "gzip-http")))]
1228+
#[test]
1229+
fn test_build_span_exporter_with_gzip_without_feature() {
1230+
use super::super::HttpExporterBuilder;
1231+
use crate::{WithHttpConfig, ExporterBuildError};
1232+
1233+
let builder = HttpExporterBuilder::default()
1234+
.with_compression(crate::Compression::Gzip);
1235+
1236+
let result = builder.build_span_exporter();
1237+
// This test will fail until the issue is fixed: compression validation should happen at build time
1238+
assert!(matches!(result, Err(ExporterBuildError::UnsupportedCompressionAlgorithm(_))));
1239+
}
1240+
1241+
#[cfg(all(feature = "trace", not(feature = "zstd-http")))]
1242+
#[test]
1243+
fn test_build_span_exporter_with_zstd_without_feature() {
1244+
use super::super::HttpExporterBuilder;
1245+
use crate::{WithHttpConfig, ExporterBuildError};
1246+
1247+
let builder = HttpExporterBuilder::default()
1248+
.with_compression(crate::Compression::Zstd);
1249+
1250+
let result = builder.build_span_exporter();
1251+
// This test will fail until the issue is fixed: compression validation should happen at build time
1252+
assert!(matches!(result, Err(ExporterBuildError::UnsupportedCompressionAlgorithm(_))));
1253+
}
12021254
}
12031255
}

‎opentelemetry-otlp/src/lib.rs‎

Lines changed: 31 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -261,6 +261,8 @@
261261
//! The following feature flags offer additional configurations on http:
262262
//!
263263
//! * `http-proto`: Use http as transport layer, protobuf as body format. This feature is enabled by default.
264+
//! * `gzip-http`: Use gzip compression for HTTP transport.
265+
//! * `zstd-http`: Use zstd compression for HTTP transport.
264266
//! * `reqwest-blocking-client`: Use reqwest blocking http client. This feature is enabled by default.
265267
//! * `reqwest-client`: Use reqwest http client.
266268
//! * `reqwest-rustls`: Use reqwest with TLS with system trust roots via `rustls-native-certs` crate.
@@ -281,9 +283,11 @@
281283
//! use opentelemetry_sdk::{trace::{self, RandomIdGenerator, Sampler}, Resource};
282284
//! # #[cfg(feature = "metrics")]
283285
//! use opentelemetry_sdk::metrics::Temporality;
284-
//! use opentelemetry_otlp::{Protocol, WithExportConfig};
286+
//! use opentelemetry_otlp::{Protocol, WithExportConfig, Compression};
285287
//! # #[cfg(feature = "grpc-tonic")]
286288
//! use opentelemetry_otlp::WithTonicConfig;
289+
//! # #[cfg(any(feature = "http-proto", feature = "http-json"))]
290+
//! use opentelemetry_otlp::WithHttpConfig;
287291
//! use std::time::Duration;
288292
//! # #[cfg(feature = "grpc-tonic")]
289293
//! use tonic::metadata::*;
@@ -316,6 +320,19 @@
316320
//! # tracer
317321
//! # };
318322
//!
323+
//! // HTTP exporter example with compression
324+
//! # #[cfg(all(feature = "trace", feature = "http-proto"))]
325+
//! # let _http_tracer = {
326+
//! let exporter = opentelemetry_otlp::SpanExporter::builder()
327+
//! .with_http()
328+
//! .with_endpoint("http://localhost:4318/v1/traces")
329+
//! .with_timeout(Duration::from_secs(3))
330+
//! .with_protocol(Protocol::HttpBinary)
331+
//! .with_compression(Compression::Gzip) // Requires gzip-http feature
332+
//! .build()?;
333+
//! # exporter
334+
//! # };
335+
//!
319336
//! # #[cfg(all(feature = "metrics", feature = "grpc-tonic"))]
320337
//! # {
321338
//! let exporter = opentelemetry_otlp::MetricExporter::builder()
@@ -332,6 +349,19 @@
332349
//! .build();
333350
//! # }
334351
//!
352+
//! // HTTP metrics exporter example with compression
353+
//! # #[cfg(all(feature = "metrics", feature = "http-proto"))]
354+
//! # {
355+
//! let exporter = opentelemetry_otlp::MetricExporter::builder()
356+
//! .with_http()
357+
//! .with_endpoint("http://localhost:4318/v1/metrics")
358+
//! .with_protocol(Protocol::HttpBinary)
359+
//! .with_timeout(Duration::from_secs(3))
360+
//! .with_compression(Compression::Zstd) // Requires zstd-http feature
361+
//! .build()
362+
//! .unwrap();
363+
//! # }
364+
//!
335365
//! # #[cfg(all(feature = "trace", feature = "grpc-tonic"))]
336366
//! # {
337367
//! tracer.in_span("doing_work", |cx| {

0 commit comments

Comments
 (0)