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

tracing: fix macros "use of moved value" with log #1823

Merged
merged 12 commits into from
Jan 29, 2022
2 changes: 1 addition & 1 deletion tracing-core/src/field.rs
Original file line number Diff line number Diff line change
Expand Up @@ -748,7 +748,7 @@ impl<'a> ValueSet<'a> {
/// Visits all the fields in this `ValueSet` with the provided [visitor].
///
/// [visitor]: super::Visit
hawkw marked this conversation as resolved.
Show resolved Hide resolved
pub(crate) fn record(&self, visitor: &mut dyn Visit) {
pub fn record(&self, visitor: &mut dyn Visit) {
Skepfyr marked this conversation as resolved.
Show resolved Hide resolved
let my_callsite = self.callsite();
for (field, value) in self.values {
if field.callsite() != my_callsite {
Expand Down
34 changes: 33 additions & 1 deletion tracing/benches/global_subscriber.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
use std::fmt::Write;

use criterion::{black_box, criterion_group, criterion_main, Criterion};
use tracing::Level;

Expand Down Expand Up @@ -43,11 +45,39 @@ impl tracing::Collect for EnabledCollector {
}
}

const NOP_LOGGER: NopLogger = NopLogger;

struct NopLogger;

impl log::Log for NopLogger {
fn enabled(&self, _metadata: &log::Metadata) -> bool {
true
}

fn log(&self, record: &log::Record) {
if self.enabled(record.metadata()) {
let mut this = self;
let _ = write!(this, "{}", record.args());
}
}

fn flush(&self) {}
}

impl Write for &NopLogger {
fn write_str(&mut self, s: &str) -> std::fmt::Result {
black_box(s);
Ok(())
}
}

const N_SPANS: usize = 100;

fn criterion_benchmark(c: &mut Criterion) {
let mut c = c.benchmark_group("global/collector");
let _ = tracing::collect::set_global_default(EnabledCollector);
let _ = log::set_logger(&NOP_LOGGER);
log::set_max_level(log::LevelFilter::Trace);
c.bench_function("span_no_fields", |b| b.iter(|| span!(Level::TRACE, "span")));

c.bench_function("event", |b| {
Expand Down Expand Up @@ -87,8 +117,10 @@ fn criterion_benchmark(c: &mut Criterion) {
}

fn bench_dispatch(c: &mut Criterion) {
let _ = tracing::collect::set_global_default(EnabledCollector);
let mut group = c.benchmark_group("global/dispatch");
let _ = tracing::collect::set_global_default(EnabledCollector);
let _ = log::set_logger(&NOP_LOGGER);
log::set_max_level(log::LevelFilter::Trace);
group.bench_function("get_ref", |b| {
b.iter(|| {
tracing::dispatch::get_default(|current| {
Expand Down
62 changes: 62 additions & 0 deletions tracing/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1109,6 +1109,68 @@ pub mod __macro_support {
.finish()
}
}

#[cfg(feature = "log")]
use tracing_core::field::{Field, ValueSet, Visit};

/// Utility to format [`ValueSet`] for logging, used by macro-generated code.
///
/// /!\ WARNING: This is *not* a stable API! /!\
/// This type, and all code contained in the `__macro_support` module, is
/// a *private* API of `tracing`. It is exposed publicly because it is used
/// by the `tracing` macros, but it is not part of the stable versioned API.
/// Breaking changes to this module may occur in small-numbered versions
/// without warning.
#[cfg(feature = "log")]
#[allow(missing_debug_implementations)]
pub struct LogValueSet<'a>(pub &'a ValueSet<'a>);
Copy link
Member

Choose a reason for hiding this comment

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

i have a slight preference for giving this a new constructor, rather than a pub field, so that we can add new fields to it while still supporting earlier versions of the macros. however, i doubt this is an issue in practice, so it may not really matter...

Skepfyr marked this conversation as resolved.
Show resolved Hide resolved

#[cfg(feature = "log")]
impl<'a> fmt::Display for LogValueSet<'a> {
Skepfyr marked this conversation as resolved.
Show resolved Hide resolved
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
let mut visit = LogVisitor {
f,
is_first: true,
result: Ok(()),
};
self.0.record(&mut visit);
visit.result
}
}

#[cfg(feature = "log")]
struct LogVisitor<'a, 'b> {
Skepfyr marked this conversation as resolved.
Show resolved Hide resolved
f: &'a mut fmt::Formatter<'b>,
is_first: bool,
result: fmt::Result,
}

#[cfg(feature = "log")]
impl Visit for LogVisitor<'_, '_> {
Skepfyr marked this conversation as resolved.
Show resolved Hide resolved
fn record_debug(&mut self, field: &Field, value: &dyn fmt::Debug) {
let res = if self.is_first {
self.is_first = false;
if field.name() == "message" {
write!(self.f, "{:?}", value)
} else {
write!(self.f, "{}={:?}", field.name(), value)
}
} else {
write!(self.f, " {}={:?}", field.name(), value)
};
Comment on lines +1154 to +1163
Copy link
Member

Choose a reason for hiding this comment

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

nit, take it or leave it: this might look slightly nicer as a match?

Suggested change
let res = if self.is_first {
self.is_first = false;
if field.name() == "message" {
write!(self.f, "{:?}", value)
} else {
write!(self.f, "{}={:?}", field.name(), value)
}
} else {
write!(self.f, " {}={:?}", field.name(), value)
};
let res = match field.name() {
"message" if self.is_first() => write!(self.f, "{:?}", value),
"message" => write!(self.f, " {:?}", value),
name if self.is_first => write!(self.f, "{}={:?}", name, value),
name => write!(self.f, " {}={:?}", name, value,
};
self.is_first = false;

though, up to you

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 prefer the explicit something is only weird about the first one of the if. However, your snippet does produce a different output to mine, in the case of message not being the first field. Mine would produce:

foo=bar message=Hi! baz=quux

whereas yours would produce:

foo=bar Hi! baz=quux

Is that behaviour ok, or should I change it to be in line with yours?

Copy link
Member

Choose a reason for hiding this comment

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

in practice, the valueset! macro will always re-order the message field to come first, so it doesn't really matter whether we handle it differently if it isn't the first field. i don't have a strong opinion about this code, so it's fine to leave it the way you had it if you prefer.

if let Err(err) = res {
self.result = self.result.and(Err(err));
}
}

fn record_str(&mut self, field: &Field, value: &str) {
if field.name() == "message" {
self.record_debug(field, &format_args!("{}", value))
} else {
self.record_debug(field, &value)
}
}
}
}

mod sealed {
Expand Down