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

Use Cow<'static, str> for Metadata strings #1020

Closed
wants to merge 29 commits into from
Closed
Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
5868a6d
[WIP] Metadata name is Cow<'static, str>
dvdplm Sep 28, 2020
f40de7b
Fetch patch from git
dvdplm Sep 29, 2020
ed04e30
Clean up MockSpan
dvdplm Sep 29, 2020
645c133
Don't clone the name in Metadata::name
dvdplm Sep 30, 2020
5a69f07
Use Cow in assert_last_closed
dvdplm Sep 30, 2020
f922cc1
If Metadata is always used with a 'static lifetime it might make sens…
dvdplm Sep 30, 2020
fa4a808
cleanup
dvdplm Sep 30, 2020
183f75f
Merge branch 'master' into dp-use-cow
dvdplm Oct 5, 2020
97e5b31
Metadata.target is Cow – breaks AsLog impl
dvdplm Oct 6, 2020
028ae65
Make it build (ty Ben)
dvdplm Oct 6, 2020
a3661f8
Make tests pass
dvdplm Oct 6, 2020
f0a6443
Undo changes to TestTracer and patched opentelemetry, not needed
dvdplm Oct 6, 2020
a9a0568
Remove patch
dvdplm Oct 6, 2020
2ee5da9
cleanup
dvdplm Oct 6, 2020
d8afcbe
cleanup
dvdplm Oct 6, 2020
381d574
Merge remote-tracking branch 'upstream/master' into dp-target-is-cow
dvdplm Oct 7, 2020
8913759
Store file as Cow
dvdplm Oct 7, 2020
b250109
Store module_path as Cow
dvdplm Oct 7, 2020
e29d3ca
Merge remote-tracking branch 'upstream/master' into dp-target-is-cow
dvdplm Oct 16, 2020
f7274dc
Merge remote-tracking branch 'upstream/master' into dp-target-is-cow
dvdplm Oct 19, 2020
b3bec86
Feature gate usage of Cow in Metadata
dvdplm Oct 19, 2020
fda189b
Add constructor for dynamic data
dvdplm Oct 19, 2020
c46c352
No need for extra lifetime
dvdplm Oct 19, 2020
7b0dfe5
Merge remote-tracking branch 'upstream/master' into dp-target-is-cow
dvdplm Oct 19, 2020
280f0bf
Use impl Into<Cow<'a, str>
dvdplm Oct 20, 2020
4225d96
Merge remote-tracking branch 'upstream/master' into dp-target-is-cow
dvdplm Oct 22, 2020
f42a644
Merge remote-tracking branch 'upstream/master' into dp-target-is-cow
dvdplm Oct 23, 2020
47e9cd5
Merge remote-tracking branch 'upstream/master' into dp-target-is-cow
Oct 28, 2020
de588f4
Merge remote-tracking branch 'origin/master' into dp-target-is-cow
dvdplm Dec 14, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
19 changes: 10 additions & 9 deletions tracing-core/src/metadata.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
//! Metadata describing trace data.
use super::{callsite, field};
use crate::stdlib::{
borrow::Cow,
cmp, fmt,
str::FromStr,
sync::atomic::{AtomicUsize, Ordering},
Expand Down Expand Up @@ -60,11 +61,11 @@ use crate::stdlib::{
/// [callsite identifier]: ../callsite/struct.Identifier.html
pub struct Metadata<'a> {
/// The name of the span described by this metadata.
name: &'static str,
name: Cow<'a, str>,

/// The part of the system that the span that this metadata describes
/// occurred in.
target: &'a str,
target: Cow<'a, str>,

/// The level of verbosity of the described span.
level: Level,
dvdplm marked this conversation as resolved.
Show resolved Hide resolved
Expand Down Expand Up @@ -122,7 +123,7 @@ impl<'a> Metadata<'a> {
/// Construct new metadata for a span or event, with a name, target, level, field
/// names, and optional source code location.
pub const fn new(

Choose a reason for hiding this comment

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

while with this we now have general support for dynamic creation, the current API, still won't allow for that. Probably want to add another fn dynamic_new where we can pass Cow<'a, str> into for dynamic dispatching.

Choose a reason for hiding this comment

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

I am afraid so ....

name: &'static str,
name: &'a str,
target: &'a str,
level: Level,
file: Option<&'a str>,
Expand All @@ -132,8 +133,8 @@ impl<'a> Metadata<'a> {
kind: Kind,
) -> Self {
Metadata {
name,
target,
name: Cow::Borrowed(name),
target: Cow::Borrowed(target),
level,
module_path,
file,
Expand All @@ -154,17 +155,17 @@ impl<'a> Metadata<'a> {
}

/// Returns the name of the span.
pub fn name(&self) -> &'static str {
self.name
pub fn name(&self) -> &str {
&self.name
}

/// Returns a string describing the part of the system where the span or
/// event that this metadata describes occurred.
///
/// Typically, this is the module path, but alternate targets may be set
/// when spans or events are constructed.
pub fn target(&self) -> &'a str {
self.target
pub fn target(&self) -> &str {
&self.target
}

/// Returns the path to the Rust module where the span occurred, or
Expand Down
14 changes: 7 additions & 7 deletions tracing-log/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -208,11 +208,11 @@ pub fn format_trace(record: &log::Record<'_>) -> io::Result<()> {

/// Trait implemented for `tracing` types that can be converted to a `log`
/// equivalent.
pub trait AsLog: crate::sealed::Sealed {
pub trait AsLog<'a>: crate::sealed::Sealed {
/// The `log` type that this type can be converted into.
type Log;
/// Returns the `log` equivalent of `self`.
fn as_log(&self) -> Self::Log;
fn as_log<'b: 'a>(&'b self) -> Self::Log;
dvdplm marked this conversation as resolved.
Show resolved Hide resolved
}

/// Trait implemented for `log` types that can be converted to a `tracing`
Expand All @@ -226,12 +226,12 @@ pub trait AsTrace: crate::sealed::Sealed {

impl<'a> crate::sealed::Sealed for Metadata<'a> {}

impl<'a> AsLog for Metadata<'a> {
impl<'a> AsLog<'a> for Metadata<'a> {
type Log = log::Metadata<'a>;
fn as_log(&self) -> Self::Log {
fn as_log<'b: 'a>(&'b self) -> Self::Log {
log::Metadata::builder()
.level(self.level().as_log())
.target(self.target())
.target(&self.target())
.build()
}
}
Expand Down Expand Up @@ -351,9 +351,9 @@ impl<'a> AsTrace for log::Record<'a> {

impl crate::sealed::Sealed for tracing_core::Level {}

impl AsLog for tracing_core::Level {
impl<'a> AsLog<'a> for tracing_core::Level {
type Log = log::Level;
fn as_log(&self) -> log::Level {
fn as_log<'b: 'a>(&'b self) -> log::Level {
match *self {
tracing_core::Level::ERROR => log::Level::Error,
tracing_core::Level::WARN => log::Level::Warn,
Expand Down
33 changes: 17 additions & 16 deletions tracing-log/src/trace_logger.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
)]
use crate::AsLog;
use std::{
borrow::Cow,
cell::RefCell,
collections::HashMap,
fmt::{self, Write},
Expand All @@ -42,9 +43,9 @@ use tracing_core::{
///
/// [`Subscriber`]: https://docs.rs/tracing/0.1.7/tracing/subscriber/trait.Subscriber.html
/// [flags]: https://docs.rs/tracing/latest/tracing/#crate-feature-flags
pub struct TraceLogger {
pub struct TraceLogger<'a> {
dvdplm marked this conversation as resolved.
Show resolved Hide resolved
settings: Builder,
spans: Mutex<HashMap<Id, SpanLineBuilder>>,
spans: Mutex<HashMap<Id, SpanLineBuilder<'a>>>,
next_id: AtomicUsize,
}

Expand All @@ -66,7 +67,7 @@ pub struct Builder {

// ===== impl TraceLogger =====

impl TraceLogger {
impl TraceLogger<'static> {
/// Returns a new `TraceLogger` with the default configuration.
pub fn new() -> Self {
Self::builder().finish()
Expand Down Expand Up @@ -152,7 +153,7 @@ impl Builder {
/// Complete the builder, returning a configured [`TraceLogger`].
///
/// [`TraceLogger`]: struct.TraceLogger.html
pub fn finish(self) -> TraceLogger {
pub fn finish(self) -> TraceLogger<'static> {
TraceLogger::from_builder(self)
}
}
Expand All @@ -170,7 +171,7 @@ impl Default for Builder {
}
}

impl Default for TraceLogger {
impl Default for TraceLogger<'_> {
fn default() -> Self {
TraceLogger {
settings: Default::default(),
Expand All @@ -181,7 +182,7 @@ impl Default for TraceLogger {
}

#[derive(Debug)]
struct SpanLineBuilder {
struct SpanLineBuilder<'a> {
parent: Option<Id>,
ref_count: usize,
fields: String,
Expand All @@ -190,11 +191,11 @@ struct SpanLineBuilder {
module_path: Option<String>,
target: String,
level: log::Level,
name: &'static str,
name: Cow<'a, str>,
}

impl SpanLineBuilder {
fn new(parent: Option<Id>, meta: &Metadata<'_>, fields: String) -> Self {
impl<'a> SpanLineBuilder<'a> {
fn new(parent: Option<Id>, meta: &'a Metadata<'_>, fields: String) -> Self {
Self {
parent,
ref_count: 1,
Expand All @@ -204,7 +205,7 @@ impl SpanLineBuilder {
module_path: meta.module_path().map(String::from),
target: String::from(meta.target()),
level: meta.level().as_log(),
name: meta.name(),
name: meta.name().into(),
}
}

Expand Down Expand Up @@ -233,14 +234,14 @@ impl SpanLineBuilder {
}
}

impl field::Visit for SpanLineBuilder {
impl field::Visit for SpanLineBuilder<'_> {
fn record_debug(&mut self, field: &field::Field, value: &dyn fmt::Debug) {
write!(self.fields, " {}={:?};", field.name(), value)
.expect("write to string should never fail")
}
}

impl Subscriber for TraceLogger {
impl Subscriber for TraceLogger<'static> {
fn enabled(&self, metadata: &Metadata<'_>) -> bool {
log::logger().enabled(&metadata.as_log())
}
Expand Down Expand Up @@ -373,7 +374,7 @@ impl Subscriber for TraceLogger {
.map(|span| {
let fields = span.fields.as_ref();
let parent = if self.settings.log_parent {
Some(span.name)
Some(span.name.as_ref())
} else {
None
};
Expand All @@ -383,7 +384,7 @@ impl Subscriber for TraceLogger {
logger.log(
&log::Record::builder()
.metadata(log_meta)
.target(meta.target())
.target(&meta.target())
.module_path(meta.module_path().as_ref().cloned())
.file(meta.file().as_ref().cloned())
.line(meta.line())
Expand Down Expand Up @@ -424,7 +425,7 @@ impl Subscriber for TraceLogger {
}
}

impl TraceLogger {
impl TraceLogger<'static> {
#[inline]
fn current_id(&self) -> Option<Id> {
CURRENT
Expand Down Expand Up @@ -455,7 +456,7 @@ impl<'a> fmt::Display for LogEvent<'a> {
}
}

impl fmt::Debug for TraceLogger {
impl fmt::Debug for TraceLogger<'static> {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
f.debug_struct("TraceLogger")
.field("settings", &self.settings)
Expand Down
4 changes: 2 additions & 2 deletions tracing-serde/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -244,8 +244,8 @@ impl<'a> Serialize for SerializeMetadata<'a> {
S: Serializer,
{
let mut state = serializer.serialize_struct("Metadata", 9)?;
state.serialize_field("name", self.0.name())?;
state.serialize_field("target", self.0.target())?;
state.serialize_field("name", &self.0.name())?;
state.serialize_field("target", &self.0.target())?;
state.serialize_field("level", &SerializeLevel(self.0.level()))?;
state.serialize_field("module_path", &self.0.module_path())?;
state.serialize_field("file", &self.0.file())?;
Expand Down
2 changes: 1 addition & 1 deletion tracing-subscriber/src/fmt/format/json.rs
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,7 @@ where
// that the fields are not supposed to be missing.
Err(e) => serializer.serialize_entry("field_error", &format!("{}", e))?,
};
serializer.serialize_entry("name", self.0.metadata().name())?;
serializer.serialize_entry("name", &self.0.metadata().name())?;
serializer.end()
}
}
Expand Down
4 changes: 2 additions & 2 deletions tracing-subscriber/src/registry/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -213,8 +213,8 @@ where
}

/// Returns the span's name,
pub fn name(&self) -> &'static str {
self.data.metadata().name()
pub fn name(&self) -> &str {
self.metadata().name()
}

/// Returns a list of [fields] defined by the span.
Expand Down
16 changes: 8 additions & 8 deletions tracing-subscriber/src/registry/sharded.rs
Original file line number Diff line number Diff line change
Expand Up @@ -422,8 +422,8 @@ mod tests {

#[derive(Default)]
struct CloseState {
open: HashMap<&'static str, Weak<()>>,
closed: Vec<(&'static str, Weak<()>)>,
open: HashMap<String, Weak<()>>,
closed: Vec<(String, Weak<()>)>,
}

struct SetRemoved(Arc<()>);
Expand All @@ -438,7 +438,7 @@ mod tests {
let is_removed = Arc::new(());
assert!(
lock.open
.insert(span.name(), Arc::downgrade(&is_removed))
.insert(span.name().to_string(), Arc::downgrade(&is_removed))
.is_none(),
"test layer saw multiple spans with the same name, the test is probably messed up"
);
Expand All @@ -461,7 +461,7 @@ mod tests {
if let Ok(mut lock) = self.inner.lock() {
if let Some(is_removed) = lock.open.remove(name) {
assert!(is_removed.upgrade().is_some());
lock.closed.push((name, is_removed));
lock.closed.push((name.to_string(), is_removed));
}
}
}
Expand Down Expand Up @@ -554,14 +554,14 @@ mod tests {
}

#[allow(unused)] // may want this for future tests
fn assert_last_closed(&self, span: Option<&str>) {
fn assert_last_closed(&self, span: Option<String>) {
let lock = self.state.lock().unwrap();
let last = lock.closed.last().map(|(span, _)| span);
assert_eq!(
last,
span.as_ref(),
"expected {:?} to have closed last",
span
span.as_ref()
);
}

Expand All @@ -570,8 +570,8 @@ mod tests {
let order = order.as_ref();
for (i, name) in order.iter().enumerate() {
assert_eq!(
lock.closed.get(i).map(|(span, _)| span),
Some(name),
lock.closed.get(i).map(|(span, _)| span.as_ref()),
Some(*name),
"expected close order: {:?}, actual: {:?}",
order,
lock.closed.iter().map(|(name, _)| name).collect::<Vec<_>>()
Expand Down
2 changes: 1 addition & 1 deletion tracing/tests/filter_caching_is_lexically_scoped.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ fn filter_caching_is_lexically_scoped() {
let count2 = count.clone();

let subscriber = subscriber::mock()
.with_filter(move |meta| match meta.name() {
.with_filter(move |meta| match meta.name().as_ref() {
"emily" | "frank" => {
count2.fetch_add(1, Ordering::Relaxed);
true
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ fn filters_are_not_reevaluated_for_the_same_span() {
let bob_count2 = bob_count.clone();

let (subscriber, handle) = subscriber::mock()
.with_filter(move |meta| match meta.name() {
.with_filter(move |meta| match meta.name().as_ref() {
"alice" => {
alice_count2.fetch_add(1, Ordering::Relaxed);
false
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ fn filters_are_reevaluated_for_different_call_sites() {
let subscriber = subscriber::mock()
.with_filter(move |meta| {
println!("Filter: {:?}", meta.name());
match meta.name() {
match meta.name().as_ref() {
"charlie" => {
charlie_count2.fetch_add(1, Ordering::Relaxed);
false
Expand Down
2 changes: 1 addition & 1 deletion tracing/tests/support/event.rs
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ impl MockEvent {

pub fn with_explicit_parent(self, parent: Option<&str>) -> MockEvent {
let parent = match parent {
Some(name) => Parent::Explicit(name.into()),
Some(name) => Parent::Explicit(name.to_string()),
None => Parent::ExplicitRoot,
};
Self {
Expand Down
2 changes: 1 addition & 1 deletion tracing/tests/support/metadata.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ impl Expect {
if let Some(ref expected_name) = self.name {
let name = actual.name();
assert!(
expected_name == name,
*expected_name == name,
"expected {} to be named `{}`, but got one named `{}`",
ctx,
expected_name,
Expand Down