opentelemetry-rust icon indicating copy to clipboard operation
opentelemetry-rust copied to clipboard

feat: replace OtlpPipeline with exporter builders

Open pitoniak32 opened this issue 1 year ago • 9 comments
trafficstars

Fixes #1810

Migration Guide

To move from opentelemetry_otlp::new_exporter(), and opentelemetry_otlp::new_pipeline().{tracing,metrics,logging}(), you will need to select the Exporter, and Provider for the signal you are using.

Below there is a guide with details about Exporters and Providers. There are also Examples for each type of signal.

Exporter Guide

The following are the available exporters:

  • SpanExporter
  • MetricsExporter
  • LogExporter

The exporter interface should have the same options as with the old method. For example, a tonic grpc exporter being configured old vs new:

// old
let old_exporter = opentelemetry_otlp::new_exporter()
  .tonic()
  .with_endpoint("http://localhost:4317");

// new
let new_exporter = SpanExporter::builder()
  .tonic()
  .with_endpoint("http://localhost:4317")
  .build()?; // note: you need to build the exporter after setting configuration.

Provider Guide

The following are the available providers:

  • TracingProvider
  • SdkMeterProvider
  • LoggerProvider

The provider interface should be similar to the old method. For example, a TracerProvider being configured old vs new:

// old
let tracer_provider: TracerProvider = opentelemetry_otlp::new_pipeline()
    .tracing()
    .with_exporter(old_exporter /* ^^ See exporter guide above ^^ */)
    .with_trace_config(Config::default().with_resource(RESOURCE.clone()))
    .install_batch(runtime::Tokio)?;

// new
let tracer_provider: TracerProvider = TracerProvider::builder()
    .with_batch_exporter(new_exporter /* ^^ See exporter guide above ^^ */, runtime::Tokio)
    .with_config(Config::default().with_resource(RESOURCE.clone()))
    .build(); // note: you need to build the provider after setting configuration.

Signal Examples

Trace Example

// old
fn init_tracer_provider() -> Result<sdktrace::TracerProvider, TraceError> {
    opentelemetry_otlp::new_pipeline()
        .tracing()
        .with_exporter(
            opentelemetry_otlp::new_exporter()
                .tonic()
                .with_endpoint("http://localhost:4317"),
        )
        .with_trace_config(Config::default().with_resource(RESOURCE.clone()))
        .install_batch(runtime::Tokio)
}

// new
fn init_tracer_provider() -> Result<sdktrace::TracerProvider, TraceError> {
    let exporter = SpanExporter::builder()
        .with_tonic()
        .with_endpoint("http://localhost:4317")
        .build()?;
    Ok(sdktrace::TracerProvider::builder()
        .with_config(Config::default().with_resource(RESOURCE.clone()))
        .with_batch_exporter(exporter, runtime::Tokio)
        .build())
}

Metric Example

// old
fn init_metrics() -> Result<opentelemetry_sdk::metrics::SdkMeterProvider, MetricsError> {
    let export_config = ExportConfig {
        endpoint: "http://localhost:4317".to_string(),
        ..ExportConfig::default()
    };
    opentelemetry_otlp::new_pipeline()
        .metrics(runtime::Tokio)
        .with_exporter(
            opentelemetry_otlp::new_exporter()
                .tonic()
                .with_export_config(export_config),
        )
}

// new
fn init_metrics() -> Result<opentelemetry_sdk::metrics::SdkMeterProvider, MetricsError> {
    let exporter = MetricsExporter::builder().with_tonic().build()?;

    let reader = PeriodicReader::builder(exporter, runtime::Tokio).build();

    Ok(SdkMeterProvider::builder()
        .with_reader(reader)
        .with_resource(RESOURCE.clone())
        .build()
        .build())
}

Log Example

// old
fn init_logs() -> Result<opentelemetry_sdk::logs::LoggerProvider, LogError> {
    opentelemetry_otlp::new_pipeline()
        .logging()
        .with_resource(RESOURCE.clone())
        .with_exporter(
            opentelemetry_otlp::new_exporter()
                .tonic()
                .with_endpoint("http://localhost:4317"),
        )
        .install_batch(runtime::Tokio)
        .with_batch_exporter(exporter, runtime::Tokio)
        .build())
}

// new
fn init_logs() -> Result<opentelemetry_sdk::logs::LoggerProvider, LogError> {
    let exporter = LogExporter::builder()
        .with_tonic()
        .with_endpoint("http://localhost:4317")
        .build()?;
  
    Ok(LoggerProvider::builder()
        .with_resource(RESOURCE.clone())
        .with_exporter(
            exporter,
        )
        .with_batch_exporter(exporter, runtime::Tokio)
        .build())
}

Changes

Started to replace the OTLPPipeline pattern in the opentelemetry-otlp crate.

Merge requirement checklist

  • [x] CONTRIBUTING guidelines followed
  • [x] Unit tests added/updated (if applicable)
  • [x] Appropriate CHANGELOG.md files updated for non-trivial, user-facing changes
  • [ ] Changes in public API reviewed (if applicable)

pitoniak32 avatar Oct 19 '24 01:10 pitoniak32

I have started looking into replicating opentelemetry-stdout patterns. I have gotten to a place that with very basic and defaulted behavior its similar. I just wanted to confirm that it seems reasonable before I start getting into the details of allowing more customization to the exporter builders.

If you have a chance to review this @cijothomas I would really appreciate it!

pitoniak32 avatar Oct 19 '24 01:10 pitoniak32

This might be another issue / PR, but I'm noticing the SdkMeterProvider and TracerProvider have different interfaces for configuring options like Resource for example. I have a few ideas for possibly unifying this if its open for discussion in another location?

pitoniak32 avatar Oct 19 '24 13:10 pitoniak32

Codecov Report

Attention: Patch coverage is 62.04380% with 104 lines in your changes missing coverage. Please review.

Project coverage is 79.6%. Comparing base (a7de955) to head (d6ab3de). Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
opentelemetry-otlp/src/metric.rs 0.0% 33 Missing :warning:
opentelemetry-otlp/src/logs.rs 0.0% 31 Missing :warning:
opentelemetry-otlp/src/exporter/tonic/mod.rs 78.7% 23 Missing :warning:
opentelemetry-otlp/src/span.rs 53.8% 12 Missing :warning:
opentelemetry-otlp/src/exporter/http/mod.rs 92.7% 5 Missing :warning:
Additional details and impacted files
@@           Coverage Diff           @@
##            main   #2221     +/-   ##
=======================================
+ Coverage   79.1%   79.6%   +0.4%     
=======================================
  Files        121     121             
  Lines      21171   21094     -77     
=======================================
+ Hits       16762   16805     +43     
+ Misses      4409    4289    -120     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Oct 19 '24 18:10 codecov[bot]

This might be another issue / PR, but I'm noticing the SdkMeterProvider and TracerProvider have different interfaces for configuring options like Resource for example. I have a few ideas for possibly unifying this if its open for discussion in another location?

Yes, ensuring consistency across signals wherever feasible is definitely something we need to tackle soon!

cijothomas avatar Oct 20 '24 04:10 cijothomas

I have started looking into replicating opentelemetry-stdout patterns. I have gotten to a place that with very basic and defaulted behavior its similar. I just wanted to confirm that it seems reasonable before I start getting into the details of allowing more customization to the exporter builders.

If you have a chance to review this @cijothomas I would really appreciate it!

Took a brief look and I like this direction. Left some comments, mostly about limiting the scope of PR. We really prefer short PRs each solving one particular aspect.

cijothomas avatar Oct 20 '24 05:10 cijothomas

Took a brief look and I like this direction. Left some comments, mostly about limiting the scope of PR. We really prefer short PRs each solving one particular aspect.

I'll go through and remove some of the changes I made that weren't specific to this issue 👍


done

pitoniak32 avatar Oct 20 '24 12:10 pitoniak32

As an output of this PR I would like to open a discussion / issue to talk about the following refactors:

  • Renaming the types named Metics* to Metric* to follow the spec.
  • Renaming opentelemetry::metrics::Result to opentelemetry::metrics:MetricResult (or doing the opposite and using the mod path for LogResult, and TraceResult).
  • Modifying the TracerProvider to follow a similar configuration approach that is used for the other "Providers" instead of or in addition to the with_config(...) option for consistency across signals.

pitoniak32 avatar Oct 20 '24 13:10 pitoniak32

As an output of this PR I would like to open a discussion / issue to talk about the following refactors:

  • Renaming the types named Metics* to Metric* to follow the spec.
  • Renaming opentelemetry::metrics::Result to opentelemetry::metrics:MetricResult (or doing the opposite and using the mod path for LogResult, and TraceResult).
  • Modifying the TracerProvider to follow a similar configuration approach that is used for the other "Providers" instead of or in addition to the with_config(...) option for consistency across signals.

:100: Very good points. Lets do it as follow ups.

cijothomas avatar Oct 21 '24 04:10 cijothomas

LGTM. We'll need some follow ups, as noted in comments.

Most important thing is a detailed migration guide, which can be written in this PR's description for users to follow.

Also the changelog can mention the before/after for a common scenario for quick reference.

Awesome! I'll address comments and add the migration details / CHANGELOG entry sometime today 🙏


I will update the PR description with migration guide during lunch / after work today. Thanks for the review!


Added changelog entry, and the description has an overview of the changes for each signal.

pitoniak32 avatar Oct 21 '24 10:10 pitoniak32

@pitoniak32 Very good migration guide in the PR description! We can keep editing it, as we get feedback from users.

cijothomas avatar Oct 23 '24 04:10 cijothomas

Guys, I know this is still 0.x and stuff, but breaking the API on almost every release is, from a user point of view, quite painful. Staying at 0.26 for the moment.

keltia avatar Nov 13 '24 09:11 keltia

@pitoniak32, can you help me with with_batch_config(), how to use it now with 0.27?

frederikhors avatar Nov 14 '24 20:11 frederikhors

@pitoniak32, can you help me with with_batch_config(), how to use it now with 0.27?

Sure! Do you have a specific issue? Did you checkout the migration guide in the overview comment already?

pitoniak32 avatar Nov 14 '24 21:11 pitoniak32

Yeah, but I cannot find with_batch_config() in there.

frederikhors avatar Nov 14 '24 21:11 frederikhors

@frederikhors Can you try something like (not tested)

fn init_tracer_provider() -> Result<sdktrace::TracerProvider, TraceError> {
    // Create a span exporter
    let exporter = SpanExporter::builder()
        .with_http()
        .with_protocol(Protocol::HttpBinary) 
        .with_endpoint("http://localhost:4318/v1/traces")
        .build()?;

    let batch_config = BatchConfigBuilder::default()
        .with_max_queue_size(2048)
        .build();

    let batch_processor = sdktrace::BatchSpanProcessor::builder(exporter, runtime::Tokio)
        .with_batch_config(batch_config)
        .build();

    Ok(TracerProvider::builder()
        .with_span_processor(batch_processor)
        .with_config(Config::default().with_resource(RESOURCE.clone()))
        .build())
}

I think we should modify the basic-otlp-http example to show this more clearly now.

lalitb avatar Nov 14 '24 21:11 lalitb

It works.

I'm having only this error now:

error[E0599]: no method named `tracer` found for struct `opentelemetry_sdk::trace::TracerProvider` in the current scope
   |
87 |             .with_tracer(otlp_tracer_provider.tracer("otlp-tracer"))
   |                                               ^^^^^^
   |
  ::: C:\Users\Fred\.cargo\registry\src\index.crates.io-6f17d22bba15001f\opentelemetry-0.27.0\src\trace\tracer_provider.rs:31:8
   |
31 |     fn tracer(&self, name: impl Into<Cow<'static, str>>) -> Self::Tracer {
   |        ------ the method is available for `opentelemetry_sdk::trace::TracerProvider` here
   |
   = help: items from traits can only be used if the trait is in scope
help: trait `TracerProvider` which provides `tracer` is implemented but not in scope; perhaps you want to import it
   |
1  + use opentelemetry::trace::TracerProvider;
   |
help: there is a method `boxed_tracer` with a similar name
   |
87 |             .with_tracer(otlp_tracer_provider.boxed_tracer("otlp-tracer"))
   |                                               ~~~~~~~~~~~~

for the code:

let layer = tracing_opentelemetry::layer()
    .with_tracer(otlp_tracer_provider.tracer("otlp-tracer"))
    .with_filter(EnvFilter::from("INFO"))
    .boxed();

Importing TracerProvider oviously doesn't work because this is not the fix.

frederikhors avatar Nov 14 '24 22:11 frederikhors

Importing TracerProvider oviously doesn't work because this is not the fix.

Try importing use opentelemetry::trace::TracerProvider as _; if that works?

lalitb avatar Nov 14 '24 23:11 lalitb

Try importing use opentelemetry::trace::TracerProvider as _; if that works?

It works. But ??? Is this OK?

frederikhors avatar Nov 15 '24 00:11 frederikhors

It works. But ??? Is this OK?

Yes, this is typical of Rust — the trait must be in scope for its methods to be accessible.

lalitb avatar Nov 15 '24 00:11 lalitb

Thanks all. If there are any issues/concerns with this change that need help - please open a new issue.

cijothomas avatar Nov 15 '24 21:11 cijothomas