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

feat: use Cow<'static, str> instead of &'static str #1018

Merged
merged 12 commits into from
Apr 18, 2023
2 changes: 1 addition & 1 deletion examples/hyper-prometheus/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ pub async fn main() -> Result<(), Box<dyn std::error::Error + Send + Sync>> {
let provider = MeterProvider::builder().with_reader(exporter).build();
let cx = Context::new();

let meter = provider.meter("hyper-prometheus-example");
let meter = provider.meter("hyper-prometheus-example".into());
let state = Arc::new(AppState {
registry,
http_counter: meter
Expand Down
25 changes: 14 additions & 11 deletions opentelemetry-api/src/global/metrics.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
use crate::metrics::{self, Meter, MeterProvider};
use core::fmt;
use once_cell::sync::Lazy;
use std::sync::{Arc, RwLock};
use std::{
borrow::Cow,
sync::{Arc, RwLock},
};

/// The global `Meter` provider singleton.
static GLOBAL_METER_PROVIDER: Lazy<RwLock<GlobalMeterProvider>> = Lazy::new(|| {
Expand All @@ -26,9 +29,9 @@ impl fmt::Debug for GlobalMeterProvider {
impl MeterProvider for GlobalMeterProvider {
fn versioned_meter(
&self,
name: &'static str,
version: Option<&'static str>,
schema_url: Option<&'static str>,
name: Cow<'static, str>,
Copy link
Contributor

Choose a reason for hiding this comment

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

What was the reason for not using impl Into<Cow<'static, str>> again?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can use impl Into here, but not for the Option types unfortunately, as you cannot convert &str into Option<Cow<...>>.

So the best i can do is just the first parameter for name. Would you like me to make this change?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think to be consistent with trace that would be good :)

version: Option<Cow<'static, str>>,
schema_url: Option<Cow<'static, str>>,
) -> Meter {
self.provider.versioned_meter(name, version, schema_url)
}
Expand Down Expand Up @@ -72,8 +75,8 @@ pub fn meter_provider() -> GlobalMeterProvider {
/// If the name is an empty string, the provider will use a default name.
///
/// This is a more convenient way of expressing `global::meter_provider().meter(name, None, None)`.
pub fn meter(name: &'static str) -> Meter {
meter_provider().versioned_meter(name, None, None)
pub fn meter(name: impl Into<Cow<'static, str>>) -> Meter {
meter_provider().versioned_meter(name.into(), None, None)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Can we use meter_provider().meter(name.into()) to avoid the extra Nones?

}

/// Creates a [`Meter`] with the name, version and schema url.
Expand All @@ -86,13 +89,13 @@ pub fn meter(name: &'static str) -> Meter {
/// # Example
/// ```rust
/// use opentelemetry_api::global::meter_with_version;
/// let meter = meter_with_version("io.opentelemetry", Some("0.17"), Some("https://opentelemetry.io/schemas/1.2.0"));
/// let meter = meter_with_version("io.opentelemetry", Some("0.17".into()), Some("https://opentelemetry.io/schemas/1.2.0".into()));
/// ```
///
pub fn meter_with_version(
name: &'static str,
version: Option<&'static str>,
schema_url: Option<&'static str>,
name: impl Into<Cow<'static, str>>,
version: Option<Cow<'static, str>>,
schema_url: Option<Cow<'static, str>>,
) -> Meter {
meter_provider().versioned_meter(name, version, schema_url)
meter_provider().versioned_meter(name.into(), version, schema_url)
}
4 changes: 2 additions & 2 deletions opentelemetry-api/src/global/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,8 @@
//! // End users of your library will configure their global tracer provider
//! // so you can use the global tracer without any setup
//! let tracer = global::tracer_provider().versioned_tracer(
//! "my-library-name",
//! Some(env!("CARGO_PKG_VERSION")),
//! "my-library-name".into(),
//! Some(env!("CARGO_PKG_VERSION").into()),
//! None,
//! );
//!
Expand Down
16 changes: 8 additions & 8 deletions opentelemetry-api/src/global/trace.rs
Original file line number Diff line number Diff line change
Expand Up @@ -291,8 +291,8 @@ pub trait ObjectSafeTracerProvider {
fn versioned_tracer_boxed(
&self,
name: Cow<'static, str>,
version: Option<&'static str>,
schema_url: Option<&'static str>,
version: Option<Cow<'static, str>>,
schema_url: Option<Cow<'static, str>>,
) -> Box<dyn ObjectSafeTracer + Send + Sync>;
}

Expand All @@ -306,8 +306,8 @@ where
fn versioned_tracer_boxed(
&self,
name: Cow<'static, str>,
version: Option<&'static str>,
schema_url: Option<&'static str>,
version: Option<Cow<'static, str>>,
schema_url: Option<Cow<'static, str>>,
) -> Box<dyn ObjectSafeTracer + Send + Sync> {
Box::new(self.versioned_tracer(name, version, schema_url))
}
Expand Down Expand Up @@ -349,13 +349,13 @@ impl trace::TracerProvider for GlobalTracerProvider {
/// Create a versioned tracer using the global provider.
fn versioned_tracer(
&self,
name: impl Into<Cow<'static, str>>,
Copy link
Contributor

@lzchen lzchen Apr 10, 2023

Choose a reason for hiding this comment

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

Why remove impl Into??

version: Option<&'static str>,
schema_url: Option<&'static str>,
name: Cow<'static, str>,
version: Option<Cow<'static, str>>,
schema_url: Option<Cow<'static, str>>,
) -> Self::Tracer {
BoxedTracer(
self.provider
.versioned_tracer_boxed(name.into(), version, schema_url),
.versioned_tracer_boxed(name, version, schema_url),
)
}
}
Expand Down
8 changes: 4 additions & 4 deletions opentelemetry-api/src/metrics/meter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ pub trait MeterProvider {
///
/// If the name is empty, then an implementation defined default name will
/// be used instead.
fn meter(&self, name: &'static str) -> Meter {
fn meter(&self, name: Cow<'static, str>) -> Meter {
self.versioned_meter(name, None, None)
}

Expand All @@ -34,9 +34,9 @@ pub trait MeterProvider {
/// default name will be used instead.
fn versioned_meter(
&self,
name: &'static str,
version: Option<&'static str>,
schema_url: Option<&'static str>,
name: Cow<'static, str>,
version: Option<Cow<'static, str>>,
schema_url: Option<Cow<'static, str>>,
) -> Meter;
}

Expand Down
8 changes: 4 additions & 4 deletions opentelemetry-api/src/metrics/noop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ use crate::{
},
Context, KeyValue,
};
use std::{any::Any, sync::Arc};
use std::{any::Any, borrow::Cow, sync::Arc};

/// A no-op instance of a `MetricProvider`
#[derive(Debug, Default)]
Expand All @@ -28,9 +28,9 @@ impl NoopMeterProvider {
impl MeterProvider for NoopMeterProvider {
fn versioned_meter(
&self,
_name: &'static str,
_version: Option<&'static str>,
_schema_url: Option<&'static str>,
_name: Cow<'static, str>,
_version: Option<Cow<'static, str>>,
_schema_url: Option<Cow<'static, str>>,
) -> Meter {
Meter::new(Arc::new(NoopMeterCore::new()))
}
Expand Down
4 changes: 2 additions & 2 deletions opentelemetry-api/src/trace/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,8 @@
//!
//! // Get a tracer for this library
//! let tracer = tracer_provider.versioned_tracer(
//! "my_name",
//! Some(env!("CARGO_PKG_VERSION")),
//! "my_name".into(),
//! Some(env!("CARGO_PKG_VERSION").into()),
//! None
//! );
//!
Expand Down
6 changes: 3 additions & 3 deletions opentelemetry-api/src/trace/noop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,9 @@ impl trace::TracerProvider for NoopTracerProvider {
/// Returns a new `NoopTracer` instance.
fn versioned_tracer(
&self,
_name: impl Into<Cow<'static, str>>,
_version: Option<&'static str>,
_schema_url: Option<&'static str>,
_name: Cow<'static, str>,
Copy link
Contributor

@lzchen lzchen Apr 10, 2023

Choose a reason for hiding this comment

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

What would be the implications of using Cow<'static, str> over impl Into<Cow<'static, str>> from a user standpoint?

_version: Option<Cow<'static, str>>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious as to why we are removing the & for version and schema_url? Are we allowing accepting of both String and &str for these fields?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes previously it only allowed for static strings, but here it allows for Cow<'static, str>, which means it can be an owned string if the version is not static (eg might come from user input, io, etc).

_schema_url: Option<Cow<'static, str>>,
) -> Self::Tracer {
NoopTracer::new()
}
Expand Down
24 changes: 12 additions & 12 deletions opentelemetry-api/src/trace/tracer_provider.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,16 +25,16 @@ pub trait TracerProvider {
/// let provider = global::tracer_provider();
///
/// // tracer used in applications/binaries
/// let tracer = provider.tracer("my_app");
/// let tracer = provider.tracer("my_app".into());
///
/// // tracer used in libraries/crates that optionally includes version and schema url
/// let tracer = provider.versioned_tracer(
/// "my_library",
/// Some(env!("CARGO_PKG_VERSION")),
/// Some("https://opentelemetry.io/schema/1.0.0")
/// "my_library".into(),
/// Some(env!("CARGO_PKG_VERSION").into()),
/// Some("https://opentelemetry.io/schema/1.0.0".into())
/// );
/// ```
fn tracer(&self, name: impl Into<Cow<'static, str>>) -> Self::Tracer {
fn tracer(&self, name: Cow<'static, str>) -> Self::Tracer {
self.versioned_tracer(name, None, None)
}

Expand All @@ -52,19 +52,19 @@ pub trait TracerProvider {
/// let provider = global::tracer_provider();
///
/// // tracer used in applications/binaries
/// let tracer = provider.tracer("my_app");
/// let tracer = provider.tracer("my_app".into());
///
/// // tracer used in libraries/crates that optionally includes version and schema url
/// let tracer = provider.versioned_tracer(
/// "my_library",
/// Some(env!("CARGO_PKG_VERSION")),
/// Some("https://opentelemetry.io/schema/1.0.0")
/// "my_library".into(),
/// Some(env!("CARGO_PKG_VERSION").into()),
/// Some("https://opentelemetry.io/schema/1.0.0".into())
/// );
/// ```
fn versioned_tracer(
&self,
name: impl Into<Cow<'static, str>>,
version: Option<&'static str>,
schema_url: Option<&'static str>,
name: Cow<'static, str>,
version: Option<Cow<'static, str>>,
schema_url: Option<Cow<'static, str>>,
) -> Self::Tracer;
}
7 changes: 5 additions & 2 deletions opentelemetry-contrib/src/trace/exporter/jaeger_json.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,11 @@ impl<R: JaegerJsonRuntime> JaegerJsonExporter<R> {

let provider = provider_builder.build();

let tracer =
provider.versioned_tracer("opentelemetry", Some(env!("CARGO_PKG_VERSION")), None);
let tracer = provider.versioned_tracer(
"opentelemetry".into(),
Some(env!("CARGO_PKG_VERSION").into()),
None,
);
let _ = opentelemetry::global::set_tracer_provider(provider);

tracer
Expand Down
8 changes: 4 additions & 4 deletions opentelemetry-datadog/src/exporter/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -290,8 +290,8 @@ impl DatadogPipelineBuilder {
provider_builder = provider_builder.with_config(config);
let provider = provider_builder.build();
let tracer = provider.versioned_tracer(
"opentelemetry-datadog",
Some(env!("CARGO_PKG_VERSION")),
"opentelemetry-datadog".into(),
Some(env!("CARGO_PKG_VERSION").into()),
None,
);
let _ = global::set_tracer_provider(provider);
Expand All @@ -311,8 +311,8 @@ impl DatadogPipelineBuilder {
provider_builder = provider_builder.with_config(config);
let provider = provider_builder.build();
let tracer = provider.versioned_tracer(
"opentelemetry-datadog",
Some(env!("CARGO_PKG_VERSION")),
"opentelemetry-datadog".into(),
Some(env!("CARGO_PKG_VERSION").into()),
None,
);
let _ = global::set_tracer_provider(provider);
Expand Down
4 changes: 2 additions & 2 deletions opentelemetry-jaeger/src/exporter/config/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -121,8 +121,8 @@ pub(crate) fn install_tracer_provider_and_get_tracer(
tracer_provider: sdk::trace::TracerProvider,
) -> Result<sdk::trace::Tracer, TraceError> {
let tracer = tracer_provider.versioned_tracer(
"opentelemetry-jaeger",
Some(env!("CARGO_PKG_VERSION")),
"opentelemetry-jaeger".into(),
Some(env!("CARGO_PKG_VERSION").into()),
None,
);
let _ = global::set_tracer_provider(tracer_provider);
Expand Down
14 changes: 10 additions & 4 deletions opentelemetry-otlp/src/span.rs
Original file line number Diff line number Diff line change
Expand Up @@ -164,8 +164,11 @@ fn build_simple_with_exporter(
provider_builder = provider_builder.with_config(config);
}
let provider = provider_builder.build();
let tracer =
provider.versioned_tracer("opentelemetry-otlp", Some(env!("CARGO_PKG_VERSION")), None);
let tracer = provider.versioned_tracer(
"opentelemetry-otlp".into(),
Some(env!("CARGO_PKG_VERSION").into()),
None,
);
let _ = global::set_tracer_provider(provider);
tracer
}
Expand All @@ -186,8 +189,11 @@ fn build_batch_with_exporter<R: TraceRuntime>(
provider_builder = provider_builder.with_config(config);
}
let provider = provider_builder.build();
let tracer =
provider.versioned_tracer("opentelemetry-otlp", Some(env!("CARGO_PKG_VERSION")), None);
let tracer = provider.versioned_tracer(
"opentelemetry-otlp".into(),
Some(env!("CARGO_PKG_VERSION").into()),
None,
);
let _ = global::set_tracer_provider(provider);
tracer
}
Expand Down
2 changes: 1 addition & 1 deletion opentelemetry-prometheus/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
//!
//! // set up a meter meter to create instruments
//! let provider = MeterProvider::builder().with_reader(exporter).build();
//! let meter = provider.meter("my-app");
//! let meter = provider.meter("my-app".into());
//!
//! // Use two instruments
//! let counter = meter
Expand Down
10 changes: 5 additions & 5 deletions opentelemetry-prometheus/tests/integration_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -302,7 +302,7 @@ fn prometheus_exporter_integration() {
.unwrap(),
)
.build();
let meter = provider.versioned_meter("testmeter", Some("v0.1.0"), None);
let meter = provider.versioned_meter("testmeter".into(), Some("v0.1.0".into()), None);
(tc.record_metrics)(&cx, meter);

let content = fs::read_to_string(Path::new("./tests/data").join(tc.expected_file))
Expand Down Expand Up @@ -354,15 +354,15 @@ fn multiple_scopes() {
.build();

let foo_counter = provider
.versioned_meter("meterfoo", Some("v0.1.0"), None)
.versioned_meter("meterfoo".into(), Some("v0.1.0".into()), None)
.u64_counter("foo")
.with_unit(Unit::new("ms"))
.with_description("meter foo counter")
.init();
foo_counter.add(&cx, 100, &[KeyValue::new("type", "foo")]);

let bar_counter = provider
.versioned_meter("meterbar", Some("v0.1.0"), None)
.versioned_meter("meterbar".into(), Some("v0.1.0".into()), None)
.u64_counter("bar")
.with_unit(Unit::new("ms"))
.with_description("meter bar counter")
Expand Down Expand Up @@ -689,8 +689,8 @@ fn duplicate_metrics() {
.with_reader(exporter)
.build();

let meter_a = provider.versioned_meter("ma", Some("v0.1.0"), None);
let meter_b = provider.versioned_meter("mb", Some("v0.1.0"), None);
let meter_a = provider.versioned_meter("ma".into(), Some("v0.1.0".into()), None);
let meter_b = provider.versioned_meter("mb".into(), Some("v0.1.0".into()), None);

(tc.record_metrics)(&cx, meter_a, meter_b);

Expand Down
4 changes: 2 additions & 2 deletions opentelemetry-sdk/benches/metric.rs
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ fn bench_counter(view: Option<Box<dyn View>>) -> (Context, SharedReader, Counter
builder = builder.with_view(view);
}
let provider = builder.build();
let cntr = provider.meter("test").u64_counter("hello").init();
let cntr = provider.meter("test".into()).u64_counter("hello").init();

(cx, rdr, cntr)
}
Expand Down Expand Up @@ -150,7 +150,7 @@ fn benchmark_collect_histogram(b: &mut Bencher, n: usize) {
let mtr = MeterProvider::builder()
.with_reader(r.clone())
.build()
.meter("sdk/metric/bench/histogram");
.meter("sdk/metric/bench/histogram".into());

for i in 0..n {
let h = mtr.i64_histogram(format!("fake_data_{i}")).init();
Expand Down
4 changes: 2 additions & 2 deletions opentelemetry-sdk/benches/trace.rs
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ fn trace_benchmark_group<F: Fn(&sdktrace::Tracer)>(c: &mut Criterion, name: &str
.with_config(sdktrace::config().with_sampler(sdktrace::Sampler::AlwaysOn))
.with_simple_exporter(VoidExporter)
.build();
let always_sample = provider.tracer("always-sample");
let always_sample = provider.tracer("always-sample".into());

b.iter(|| f(&always_sample));
});
Expand All @@ -125,7 +125,7 @@ fn trace_benchmark_group<F: Fn(&sdktrace::Tracer)>(c: &mut Criterion, name: &str
.with_config(sdktrace::config().with_sampler(sdktrace::Sampler::AlwaysOff))
.with_simple_exporter(VoidExporter)
.build();
let never_sample = provider.tracer("never-sample");
let never_sample = provider.tracer("never-sample".into());
b.iter(|| f(&never_sample));
});

Expand Down
Loading