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

Enforce timeouts #2576

Open
cijothomas opened this issue Jan 30, 2025 · 3 comments
Open

Enforce timeouts #2576

cijothomas opened this issue Jan 30, 2025 · 3 comments
Assignees
Labels
A-common Area:common issues that not related to specific pillar M-exporter-otlp

Comments

@cijothomas
Copy link
Member

BatchExportProcessor, PeriodicReader has been rewritten to use own thread. In the new implementation, they do not enforce timeouts and instead relies on Exporters to enforce timeouts.

Tonic currently enforce timeout - default 10, and respects both OTEL_EXPORTER_OTLP_TIMEOUT and the signal specific override of this.
Reqwest-Blocking does not enforce timeout, so the default timeout from Reqwest-Blocking client, which is 30 seconds, is the only one.
Need to fix this to also enforce timeout like the tonic crate.

We also need to document that OTEL_BLRP_EXPORT_TIMEOUT,OTEL_BSP_EXPORT_TIMEOUT, OTEL_METRIC_EXPORT_TIMEOUT are not honored by default in the SDK.

@cijothomas cijothomas added A-common Area:common issues that not related to specific pillar M-exporter-otlp labels Jan 30, 2025
@cijothomas
Copy link
Member Author

#[cfg(not(target_arch = "wasm32"))]
#[async_trait]
impl HttpClient for reqwest::blocking::Client {
async fn send_bytes(&self, request: Request) -> Result<Response, HttpError> {
otel_debug!(name: "ReqwestBlockingClient.Send");
let mut request: reqwest::blocking::Request = request.try_into()?;
let request_timeout = request.timeout_mut();
if request_timeout.is_none() {
request_timeout.replace(std::time::Duration::from_secs(30));
}
let mut response = self.execute(request)?.error_for_status()?;
let headers = std::mem::take(response.headers_mut());
let mut http_response = Response::builder()
.status(response.status())
.body(response.bytes()?)?;
*http_response.headers_mut() = headers;

        Ok(http_response)
    }
}

The above should work for Reqwest::Blocking.

@cijothomas
Copy link
Member Author

Its better to do it in the opentelemetry-otlp exporter rather than in the http crate, as it needs to get the time out vases from otlp specific env variables.

@cijothomas
Copy link
Member Author

Additional fix to be done
BatchProcessor currently has a timeout it is not enforced. It is better to remove it from API.

/// .with_max_export_timeout(Duration::from_secs(30))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-common Area:common issues that not related to specific pillar M-exporter-otlp
Projects
None yet
Development

No branches or pull requests

2 participants