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

[Bug]: example code is misleading about how with_view works #1849

Open
CobaltCause opened this issue May 31, 2024 · 1 comment
Open

[Bug]: example code is misleading about how with_view works #1849

CobaltCause opened this issue May 31, 2024 · 1 comment
Labels
bug Something isn't working

Comments

@CobaltCause
Copy link

What happened?

The way these variable names are worded make it sound like that's the only effect each view has and neglects to mention that the implementation of these closures also drops all pieces of information about each instrument that aren't specified again via Stream combinators. To demonstrate:

$ cargo run -qp metrics-advanced | jq '.resourceMetrics.scopeMetrics[].metrics[].name'
"my_histogram_renamed"
""
""

It's quite surprising that the names are dropped despite no indication that this would happen.

Here's a small program that demonstrates how this is problematic and also demonstrates how to achieve the desired (and implied) behavior:

// [dependencies]
// opentelemetry = { version = "0.23.0", features = ["metrics"] }
// opentelemetry-prometheus = "0.16.0"
// opentelemetry_sdk = "0.23.0"
// prometheus = "0.13.4"

use std::error::Error;

use opentelemetry::metrics::{MeterProvider, Unit};
use opentelemetry_prometheus::exporter;
use opentelemetry_sdk::metrics::{
    new_view, Aggregation, Instrument, SdkMeterProvider, Stream,
};
use prometheus::{Registry, TextEncoder};

fn main() {
    if let Err(e) = minimal_repro() {
        eprintln!("error: {e}");
    }
    if this_works().is_err() {
        unreachable!()
    }
}

// This does what the example implies you should do to only change the
// boundaries but leave everything else the same.
fn minimal_repro() -> Result<(), Box<dyn Error>> {
    let registry = Registry::new();
    let exporter = exporter().with_registry(registry.clone()).build()?;
    let provider = SdkMeterProvider::builder()
        .with_reader(exporter)
        .with_view(|_: &Instrument| {
            Some(Stream::new().aggregation(
                Aggregation::ExplicitBucketHistogram {
                    boundaries: vec![1., 2., 3.],
                    record_min_max: true,
                },
            ))
        })
        .build();
    let meter = provider.meter("example");
    let histogram =
        meter.u64_histogram("histogram").with_unit(Unit::new("seconds")).init();
    histogram.record(1, &[]);
    let encoded = TextEncoder::new().encode_to_string(&registry.gather())?;
    println!("{encoded}");
    Ok(())
}

// This version actually works. The secret sauce is to use the `new_view`
// function.
fn this_works() -> Result<(), Box<dyn Error>> {
    let registry = Registry::new();
    let exporter = exporter().with_registry(registry.clone()).build()?;
    let provider = SdkMeterProvider::builder()
        .with_reader(exporter)
        .with_view(new_view(
            Instrument::new().name("histogram"),
            Stream::new().aggregation(Aggregation::ExplicitBucketHistogram {
                boundaries: vec![1., 2., 3.],
                record_min_max: true,
            }),
        )?)
        .build();
    let meter = provider.meter("example");
    let histogram =
        meter.u64_histogram("histogram").with_unit(Unit::new("seconds")).init();
    histogram.record(1, &[]);
    let encoded = TextEncoder::new().encode_to_string(&registry.gather())?;
    println!("{encoded}");
    Ok(())
}

I've pasted the output of this program in the "Relevant log output" section.

API Version

0.23.0

SDK Version

0.23.0

What Exporter(s) are you seeing the problem on?

Prometheus

Relevant log output

attempt one failed: Error: MetricFamily has no name: name: "" help: "" type: HISTOGRAM metric {label {name: "otel_scope_name" value: "example"} histogram {sample_count: 1 sample_sum: 1 bucket {cumulative_count: 1upper_bound: 1} bucket {cumulative_count: 1 upper_bound: 2} bucket {cumulative_count: 1 upper_bound: 3}}}
# TYPE histogram histogram
histogram_bucket{otel_scope_name="example",le="1"} 1
histogram_bucket{otel_scope_name="example",le="2"} 1
histogram_bucket{otel_scope_name="example",le="3"} 1
histogram_bucket{otel_scope_name="example",le="+Inf"} 1
histogram_sum{otel_scope_name="example"} 1
histogram_count{otel_scope_name="example"} 1
# HELP target_info Target metadata
# TYPE target_info gauge
target_info{service_name="unknown_service",telemetry_sdk_language="rust",telemetry_sdk_name="opentelemetry",telemetry_sdk_version="0.23.0"} 1
@CobaltCause CobaltCause added bug Something isn't working triage:todo Needs to be traiged. labels May 31, 2024
@cijothomas
Copy link
Member

Thanks for reporting! It looks like a bug in view implementation, not just an issue with doc.
We won't be fixing the View bug, as Views are about to be removed for the initial stable release.

For customizing histogram buckets, #1241 will be offered instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants