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

Update opentelemetry to 0.19 #12

Merged
merged 7 commits into from
May 23, 2023

Conversation

jaysonsantos
Copy link
Contributor

This is a stacked PR with #9.
It should be a clean update, as opentelemetry 0.19 did not break any API.

jtescher and others added 2 commits March 25, 2023 11:54
This is pretty much a backport from the code that @jtescher did on the main repo

Signed-off-by: Jayson Reis <santosdosreis@gmail.com>
Signed-off-by: Jayson Reis <santosdosreis@gmail.com>
Signed-off-by: Jayson Reis <santosdosreis@gmail.com>
@djc
Copy link
Collaborator

djc commented Apr 11, 2023

@jtescher ping? What is this waiting for?

@Oliboy50
Copy link
Contributor

Oliboy50 commented Apr 12, 2023

@djc I would say that this is waiting at least for #9

BTW, me too, I would love to see an opentelemetry 0.19 compatible release of tracing-opentelemetry soon... but I understand that moving sources from another repository may have slow things down here... good luck contributors 💪

inner.append(&mut batch);
}
Ok(())
fn export(&mut self, mut batch: Vec<SpanData>) -> BoxFuture<'static, ExportResult> {
Copy link

Choose a reason for hiding this comment

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

This seems like a drive-by change, is there some reason behind it other than personal preference? Does it allow removing async-trait from dependencies?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tomkarw this is from #9 which is an exact backport from the main repo, I am not the original author of it

Copy link

Choose a reason for hiding this comment

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

Got it. It's a nitpick, so I don't think chasing the original author makes sense.

@fenollp
Copy link

fenollp commented Apr 19, 2023

Hi there, looks like this PR needs rebasing! :)

@jaysonsantos
Copy link
Contributor Author

@fenollp this pr is stacked with the 0.18 #9 , I have to wait for that one to be merged first

@liamwh
Copy link

liamwh commented May 4, 2023

Perhaps it's also a good idea to include an example with OpenTelemetry OTLP in this PR also, to ensure compilation checks with OpenTelemetry OTLP?

For example, something like the following might serve as a good best practice example base case:

use opentelemetry_otlp::WithExportConfig;
use tracing_subscriber::{prelude::__tracing_subscriber_SubscriberExt, Layer};

const OTEL_EXPORTER_OTLP_ENDPOINT_ENV_VAR: &str = "OTEL_EXPORTER_OTLP_ENDPOINT";
const OTEL_EXPORTER_OTLP_ENDPOINT_DEFAULT: &str = "http://localhost:4317";

const OBSERVABILITY_SERVICE_NAME_ENV_VAR: &str = "OBSERVABILITY_SERVICE_NAME";
const OBSERVABILITY_SERVICE_NAME_DEFAULT: &str = "veloxide-server";

#[tracing::instrument]
pub async fn configure_observability() -> std::result::Result<(), crate::error::Error> {
    let otel_exporter_endpoint =
        dotenvy::var(OTEL_EXPORTER_OTLP_ENDPOINT_ENV_VAR).unwrap_or_else(|_| {
            tracing::warn!(
                "{} Env var not set, using default",
                OTEL_EXPORTER_OTLP_ENDPOINT_DEFAULT
            );
            OTEL_EXPORTER_OTLP_ENDPOINT_DEFAULT.to_string()
        });

    let observability_service_name = dotenvy::var(OBSERVABILITY_SERVICE_NAME_ENV_VAR)
        .unwrap_or_else(|_| OBSERVABILITY_SERVICE_NAME_DEFAULT.to_string());

    let tracer = opentelemetry_otlp::new_pipeline()
        .tracing()
        .with_exporter(
            opentelemetry_otlp::new_exporter()
                .tonic()
                .with_endpoint(otel_exporter_endpoint),
        )
        .with_trace_config(opentelemetry::sdk::trace::config().with_resource(
            opentelemetry::sdk::Resource::new(vec![opentelemetry::KeyValue::new(
                "service.name",
                observability_service_name,
            )]),
        ))
        .install_batch(opentelemetry::runtime::Tokio)?;

    // Create a tracing layer with the configured tracer
    let telemetry_layer = tracing_opentelemetry::layer().with_tracer(tracer);

    let filter = tracing_subscriber::EnvFilter::from_default_env();

    // Use the tracing subscriber `Registry`, or any other subscriber
    // that impls `LookupSpan`
    let subscriber = tracing_subscriber::Registry::default()
        .with(telemetry_layer)
        .with(
            tracing_subscriber::fmt::layer()
                .with_writer(std::io::stdout)
                .with_filter(filter),
        );

    Ok(tracing::subscriber::set_global_default(subscriber)?)
}

@tekul
Copy link

tekul commented May 10, 2023

@liamwh I think opentelemetry-rust supports OTEL_EXPORTER_OTLP_ENDPOINT and also OTEL_SERVICE_NAME out of the box, so these could be used to simplify the example.

Cargo.toml Outdated Show resolved Hide resolved
@fenollp
Copy link

fenollp commented May 23, 2023

@jaysonsantos The other PR has been merged :)

jtescher added a commit that referenced this pull request May 23, 2023
### Breaking Changes

- Upgrade to `v0.19.0` of `opentelemetry` (#12)
For list of breaking changes in OpenTelemetry, see the [v0.19.0
changelog](https://github.com/open-telemetry/opentelemetry-rust/blob/main/opentelemetry/CHANGELOG.md#v0190).
- Update MSRV to require rust v1.60+, as `opentelemetry` requires it now
(#12)

Thanks to @jaysonsantos, @briankung, and @humb1t for contributing to
this release!

Co-authored-by: Eliza Weisman <eliza@buoyant.io>
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 this pull request may close these issues.

8 participants