-
Notifications
You must be signed in to change notification settings - Fork 83
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
Update opentelemetry to 0.19 #12
Conversation
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>
@jtescher ping? What is this waiting for? |
tests/trace_state_propagation.rs
Outdated
inner.append(&mut batch); | ||
} | ||
Ok(()) | ||
fn export(&mut self, mut batch: Vec<SpanData>) -> BoxFuture<'static, ExportResult> { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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.
Hi there, looks like this PR needs rebasing! :) |
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)?)
} |
@liamwh I think opentelemetry-rust supports |
@jaysonsantos The other PR has been merged :) |
### 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>
This is a stacked PR with #9.
It should be a clean update, as opentelemetry 0.19 did not break any API.