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

SimpleExporter is not useable with OTLP/Http #2189

Closed
cijothomas opened this issue Oct 9, 2024 · 6 comments · Fixed by #2529
Closed

SimpleExporter is not useable with OTLP/Http #2189

cijothomas opened this issue Oct 9, 2024 · 6 comments · Fixed by #2529
Assignees
Milestone

Comments

@cijothomas
Copy link
Member

cijothomas commented Oct 9, 2024

SimpleExporter is not useable with OTLP/HTTP (Reqwest client and Hyper client both fails), as export just hangs forever.

Minimal repro when using Reqwest

use tracing::info;
use tracing_subscriber::layer::SubscriberExt;
use tracing_subscriber::Registry;
use tracing_subscriber::util::SubscriberInitExt;
use opentelemetry_appender_tracing::layer::OpenTelemetryTracingBridge;
use opentelemetry_otlp::WithExportConfig;

fn main() {
     let exporter = opentelemetry_otlp::new_exporter().http();
    let logger_provider = opentelemetry_otlp::new_pipeline()
        .logging()
        .with_exporter(
            exporter
                .with_protocol(Protocol::HttpBinary)
                .with_endpoint("http://localhost:4318/v1/logs"),
        )
        .install_simple()
        .unwrap();

    // Create a new OpenTelemetryTracingBridge using the above LoggerProvider.
    let layer = OpenTelemetryTracingBridge::new(&logger_provider);
    tracing_subscriber::registry()
        .with(layer)
        .init();

    info!(name: "my-event", target: "my-target", "hello from {}. My price is {}", "apple", 1.99);

    let _ = logger_provider.shutdown();
}

Minimal repro when using Hyper

let exporter = opentelemetry_otlp::new_exporter().http();
    let exporter = exporter.with_http_client(hyper::HyperClient::default());
    let logger_provider = opentelemetry_otlp::new_pipeline()
        .logging()
        .with_exporter(
            exporter
                .with_protocol(Protocol::HttpBinary)
                .with_endpoint("http://localhost:4318/v1/logs"),
        )
        .install_simple()
        .unwrap();
``
@lalitb
Copy link
Member

lalitb commented Oct 9, 2024

Just to be clear - it doesn't work with reqwest client (both blocking and async) ?

@cijothomas
Copy link
Member Author

Just to be clear - it doesn't work with reqwest client (both blocking and async) ?

"reqwest-blocking-client" works fine in this case. (only as long as it is not called from a tokio managed thread)

@lalitb
Copy link
Member

lalitb commented Oct 16, 2024

Also to add, async-compat crate under smol-rs org, provides the compatibility between futures and tokio runtimes. This means we can use the reqwest client in the non-blocking mode under SimpleExporter. Eg. with:

use async_compat::{Compat, CompatExt};
futures_executor::block_on(Compat::new((exporter.export(LogBatch::new(log_tuple)))))

This is as documented here - https://docs.rs/async-compat/latest/async_compat/#

fn main() -> std::io::Result<()> {
    futures::executor::block_on(Compat::new(async {
        let stdin = tokio::io::stdin();
        let mut stdout = tokio::io::stdout();

        futures::io::copy(stdin.compat(), &mut stdout.compat_mut()).await?;
        Ok(())
    }))
}

As per the docs, this adapter internally creates the tokio runtime if not already running, and then allows using types and traits from the futures crate in this tokio runtime.

If a runtime is already available via tokio
//!       thread-locals, then it will be used. Otherwise, a new single-threaded runtime will be
//!       created on demand.

Have tested this with OTLP + reqwest and it works. Will validate if it works for tonic and hyper.

@cijothomas
Copy link
Member Author

Planning to close the issue with the following:

  1. To use simple exporter with otlp/http, reqwest-blocking feature should be used. (which would be the default soon).
  2. AND the logs/span must be emitted from non-tokio context. If the main if tokio, then span/log must be emitted from separate throwaway thread. OR use batchexporter

Don't think we need to modify SimpleExporter to spin up its own thread and make this work more easily....
Any comments/thoughts on this?

@lalitb
Copy link
Member

lalitb commented Jan 15, 2025

Don't think we need to modify SimpleExporter to spin up its own thread and make this work more easily....
Any comments/thoughts on this?

Moving back to thread-based approach as earlier - /~https://github.com/open-telemetry/opentelemetry-rust/blob/v0.20.0/opentelemetry-sdk/src/logs/log_processor.rs#L49 - will work for with reqwest-blocking for both main as tokio and non-tokio context?

@cijothomas
Copy link
Member Author

Don't think we need to modify SimpleExporter to spin up its own thread and make this work more easily....
Any comments/thoughts on this?

Moving back to thread-based approach as earlier - /~https://github.com/open-telemetry/opentelemetry-rust/blob/v0.20.0/opentelemetry-sdk/src/logs/log_processor.rs#L49 - will work for with reqwest-blocking for both main as tokio and non-tokio context?

For SimpleProcessor users, they can enable feature "reqwest", and it should work. Listing the working options for SimpleProcessor customers below. There is one working gRPC and HTTP client option each. Given this, do we need to go back to background thread creation in Simple case?

Works:
SimpleExporter
ProviderCreation inside tokio_RT
LoggingInside tokio_RT
Reqwest,Tonic

SimpleExporter
ProviderCreation inside tokio_RT
Logging outside tokio_RT
Reqwest-Blocking, Tonic

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants