ValueSet::len() is not accurate

[kmdreko]

Bug Report

Version

Latest tracing v0.1.40. Tested below with tracing-subscriber v0.3.18 but that isn't particularly relevant.

Description

The documentation says that ValueSet::len:

Returns the number of fields in this ValueSet that would be visited by a given visitor to the ValueSet::record() method.

But that is not correct. The length does not consider that None values (via Empty or Option) which will not be given to a visitor.

Demo

use std::fmt::Debug;

use tracing::field::{Empty, Field, Visit};
use tracing::span::{Attributes, Id};
use tracing::{info_span, Subscriber};
use tracing_subscriber::layer::{Context, SubscriberExt};
use tracing_subscriber::util::SubscriberInitExt;
use tracing_subscriber::Layer;

struct CountingVisitor {
    count: usize,
}

impl Visit for CountingVisitor {
    fn record_debug(&mut self, _field: &Field, _value: &dyn Debug) {
        self.count += 1;
    }
}

struct CountingLayer;

impl<S: Subscriber> Layer<S> for CountingLayer {
    fn on_new_span(&self, attrs: &Attributes<'_>, _id: &Id, _ctx: Context<'_, S>) {
        println!("len() = {}", attrs.values().len());

        let mut visitor = CountingVisitor { count: 0 };
        attrs.values().record(&mut visitor);
        println!("actual = {}", visitor.count);
    }
}

fn main() {
    tracing_subscriber::registry().with(CountingLayer).init();

    info_span!("counting_test", a = "b", c = Empty, d = None::<i32>);
}

len() = 3
actual = 1

I am willing to fix this whether the "fix" is to adjust the documentation or the implementation.

[kaffarell]

Don't have any hard feelings towards either solution, though it would be nice if attrs.fields().len() would return 3 and attrs.values().len() would return 1 IMO. Don't know if that's hard to change implementation-wise though. We intentionally don't visit Option::None and tracing::field::Empty, but hard coding these doesn't seem right.

[kmdreko]

Without changes to the Value trait or how values are put in the valueset, making len() return 1 would pretty much require visiting each one just to see if it would yield a value (essentially what I've done above). The len() implementation is already doing an O(n) walk (to ensure the callsites are the same), so it wouldn't be a dramatic change in that regard.