Skip to content
Open
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions opentelemetry-otlp/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,12 @@

## vNext

- Improved error logging for gRPC and HTTP exporters. The status code is now
logged at WARN level, while potentially sensitive details (message, response
body) are logged at DEBUG level only. This prevents sensitive information
(like authentication tokens) from being logged at higher log levels while
still providing actionable error information in production.
[#3021](https://github.com/open-telemetry/opentelemetry-rust/issues/3021)
- Fix `NoHttpClient` error when multiple HTTP client features are enabled by using priority-based selection (`reqwest-client` > `hyper-client` > `reqwest-blocking-client`). [#2994](https://github.com/open-telemetry/opentelemetry-rust/issues/2994)
- Add partial success response handling for OTLP exporters (traces, metrics, logs) per OTLP spec. Exporters now log warnings when the server returns partial success responses with rejected items and error messages. [#865](https://github.com/open-telemetry/opentelemetry-rust/issues/865)
- Refactor `internal-logs` feature in `opentelemetry-otlp` to reduce unnecessary dependencies[3191](https://github.com/open-telemetry/opentelemetry-rust/pull/3192)
Expand Down
30 changes: 23 additions & 7 deletions opentelemetry-otlp/src/exporter/http/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use super::{
};
use crate::{ExportConfig, Protocol, OTEL_EXPORTER_OTLP_ENDPOINT, OTEL_EXPORTER_OTLP_HEADERS};
use http::{HeaderName, HeaderValue, Uri};
use opentelemetry::otel_debug;
use opentelemetry::{otel_debug, otel_warn};
use opentelemetry_http::{Bytes, HttpClient};
use opentelemetry_proto::transform::common::tonic::ResourceAttributesWithSchema;
#[cfg(feature = "logs")]
Expand Down Expand Up @@ -488,7 +488,17 @@ impl OtlpHttpClient {

// Send request
let response = client.send_bytes(request).await.map_err(|e| {
HttpExportError::new(0, format!("Network error: {e:?}")) // Network error
otel_warn!(
name: "HttpClient.ExportFailed",
Copy link
Contributor

Choose a reason for hiding this comment

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

We are using the same event name HttpClient.ExportFailed for two different kinds of errors:

  • Network error from client.send_bytes
  • HTTP status error when response.status() is not successful.

Maybe we could use different event names for better diagnosability: HttpClient.NetworkError and HttpClientStatusError.

Copy link
Member Author

Choose a reason for hiding this comment

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

I did consider that and ended up with same one. I am not 100% happy with this either.

I tried to distinguish between failures where we don't get a response at all and the ones where we get response, but it has status code indicating failure. But then realized that request client don't expose us a way to obtain StatusCode from its Error. Without having firmer control over the client, this is a decent tradeoff.
(The otel-http crate could offer additional logging, as it knows internals of the client it uses, but OTLP exporter does not know that much).

let me know if we should try to make this better or this is acceptable tradeoff.

url = request_uri.as_str()
);
// Error details may contain sensitive information,
// so log them at debug level only.
otel_debug!(
name: "HttpClient.ExportFailedDetails",
error = format!("{e:?}")
);
HttpExportError::new(0, "HTTP export failed".to_string())
})?;

let status_code = response.status().as_u16();
Expand All @@ -499,12 +509,18 @@ impl OtlpHttpClient {
.map(|s| s.to_string());

if !response.status().is_success() {
let message = format!(
"HTTP export failed. Url: {}, Status: {}, Response: {:?}",
request_uri,
status_code,
response.body()
otel_warn!(
name: "HttpClient.ExportFailed",
status_code = status_code,
url = request_uri.as_str()
);
// Response body may contain sensitive information,
// so log it at debug level only.
otel_debug!(
name: "HttpClient.ExportFailedDetails",
response_body = format!("{:?}", response.body())
);
let message = "HTTP export failed".to_string();
return Err(match retry_after {
Some(retry_after) => {
HttpExportError::with_retry_after(status_code, retry_after, message)
Expand Down
30 changes: 26 additions & 4 deletions opentelemetry-otlp/src/exporter/tonic/logs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -94,8 +94,19 @@ impl LogExporter for TonicLogsClient {
.interceptor
.call(Request::new(()))
.map_err(|e| {
otel_warn!(
name: "TonicLogsClient.InterceptorFailed",
grpc_code = format!("{:?}", e.code())
);
// grpc_message and grpc_details may contain sensitive information,
// so log them at debug level only.
otel_debug!(
name: "TonicLogsClient.InterceptorFailedDetails",
grpc_message = e.message(),
grpc_details = format!("{:?}", e.details())
);
// Convert interceptor errors to tonic::Status for retry classification
tonic::Status::internal(format!("interceptor error: {e:?}"))
tonic::Status::internal("Logs export failed in interceptor")
})?
.into_parts();
Ok((inner.client.clone(), m, e))
Expand Down Expand Up @@ -137,9 +148,20 @@ impl LogExporter for TonicLogsClient {
.await
{
Ok(_) => Ok(()),
Err(tonic_status) => Err(OTelSdkError::InternalFailure(format!(
"export error: {tonic_status:?}"
))),
Err(tonic_status) => {
otel_warn!(
name: "TonicLogsClient.ExportFailed",
grpc_code = format!("{:?}", tonic_status.code())
);
// grpc_message and grpc_details may contain sensitive information,
// so log them at debug level only.
otel_debug!(
name: "TonicLogsClient.ExportFailedDetails",
grpc_message = tonic_status.message(),
grpc_details = format!("{:?}", tonic_status.details())
);
Err(OTelSdkError::InternalFailure("Logs export failed".into()))
}
}
}

Expand Down
34 changes: 28 additions & 6 deletions opentelemetry-otlp/src/exporter/tonic/metrics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -85,9 +85,18 @@ impl MetricsClient for TonicMetricsClient {
.interceptor
.call(Request::new(()))
.map_err(|e| {
tonic::Status::internal(format!(
"unexpected status while exporting {e:?}"
))
otel_warn!(
name: "TonicMetricsClient.InterceptorFailed",
grpc_code = format!("{:?}", e.code())
);
// grpc_message and grpc_details may contain sensitive information,
// so log them at debug level only.
otel_debug!(
name: "TonicMetricsClient.InterceptorFailedDetails",
grpc_message = e.message(),
grpc_details = format!("{:?}", e.details())
);
tonic::Status::internal("Metrics export failed in interceptor")
})?
.into_parts();
Ok((inner.client.clone(), m, e))
Expand Down Expand Up @@ -127,9 +136,22 @@ impl MetricsClient for TonicMetricsClient {
.await
{
Ok(_) => Ok(()),
Err(tonic_status) => Err(OTelSdkError::InternalFailure(format!(
"export error: {tonic_status:?}"
))),
Err(tonic_status) => {
otel_warn!(
name: "TonicMetricsClient.ExportFailed",
grpc_code = format!("{:?}", tonic_status.code())
);
// grpc_message and grpc_details may contain sensitive information,
// so log them at debug level only.
otel_debug!(
name: "TonicMetricsClient.ExportFailedDetails",
grpc_message = tonic_status.message(),
grpc_details = format!("{:?}", tonic_status.details())
);
Err(OTelSdkError::InternalFailure(
"Metrics export failed".into(),
Copy link
Member

Choose a reason for hiding this comment

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

grpc_code should also be added here?

))
}
}
}

Expand Down
30 changes: 26 additions & 4 deletions opentelemetry-otlp/src/exporter/tonic/trace.rs
Original file line number Diff line number Diff line change
Expand Up @@ -96,8 +96,19 @@ impl SpanExporter for TonicTracesClient {
.interceptor
.call(Request::new(()))
.map_err(|e| {
otel_warn!(
name: "TonicTracesClient.InterceptorFailed",
grpc_code = format!("{:?}", e.code())
);
// grpc_message and grpc_details may contain sensitive information,
// so log them at debug level only.
otel_debug!(
name: "TonicTracesClient.InterceptorFailedDetails",
grpc_message = e.message(),
grpc_details = format!("{:?}", e.details())
);
// Convert interceptor errors to tonic::Status for retry classification
tonic::Status::internal(format!("interceptor error: {e:?}"))
tonic::Status::internal("Traces export failed in interceptor")
Copy link
Member

Choose a reason for hiding this comment

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

grpc_code here too.

})?
.into_parts();
Ok((inner.client.clone(), m, e))
Expand Down Expand Up @@ -140,9 +151,20 @@ impl SpanExporter for TonicTracesClient {
.await
{
Ok(_) => Ok(()),
Err(tonic_status) => Err(OTelSdkError::InternalFailure(format!(
"export error: {tonic_status:?}"
))),
Err(tonic_status) => {
otel_warn!(
name: "TonicTracesClient.ExportFailed",
grpc_code = format!("{:?}", tonic_status.code())
);
// grpc_message and grpc_details may contain sensitive information,
// so log them at debug level only.
otel_debug!(
name: "TonicTracesClient.ExportFailedDetails",
grpc_message = tonic_status.message(),
grpc_details = format!("{:?}", tonic_status.details())
);
Err(OTelSdkError::InternalFailure("Traces export failed".into()))
}
}
}

Expand Down
Loading