core: consider changing `Subscriber::new_span` to return `Option<Id>`

[hawkw]

Feature Request

Crates

• tracing-core

Motivation

Currently, spans and events may be disabled by a subscriber by returning Interest::never from Subscriber::register_callsite, or by returning false from Subscriber::enabled. However, in some cases, a Subscriber may also wish to disable a span or event based on the values of its fields.

For performance reasons, the values of span/event fields are not available in enabled (or register_callsite, of course) — these methods are called before constructing the ValueSet for an instance of a span/event, since their purpose is to allow skipping ValueSet construction for unwanted spans or events. This means that if a Subscriber decides it doesn't want something based on the values of its fields, it has to choose to ignore that span or event in its new_span and event methods.

For events, this is fine — the Subscriber::event method doesn't return anything, so if a subscriber decides it doesn't want an event inside the event method, it can just...not record it, and return immediately. On the other hand, this is not possible for spans. Subscriber::new_span is required to return a span ID for the new span, and once it returns an Id, the span will be created with that Id and a reference to the subscriber (an Arc clone of its Dispatch).

If the goal is just to avoid printing/recording data related to that span, the subscriber could add its Id to a list of disabled spans, and ignore it in all subsequent calls to enter, exit, record, follows_from, clone_span, and try_close. This is not a great option for performance reasons, though: once the span is created, it will store a Dispatch clone, so cloning it will bump the Dispatch's ref count; calling enter and other methods on the Span will dereference the dispatcher and call methods on it that would not be called if the span was disabled, and so on.

Furthermore, the Subscriber now needs some way of tracking which IDs are disabled. A naive solution, which I imagine would be the obvious one to most people, is to store some kind of set of disabled spans in the subscriber. Because Subscribers may be shared by multiple threads, this might mean a RwLock<HashSet<Id>> or something...and in order to ignore disabled spans correctly, every call to enter, exit, record, record_follows_from, clone_span, and try_close would require checking if the provided ID is in that set. This has a massive performance implication, by forcing every span-notification method to contend a lock. This means that disabling a span might actually cost more than enabling it, if it is disabled in new_span.

Of course, there are better approaches: since the subscriber controls ID assignment, it could just flip a bit in the ID that indicates that the span is disabled, and test if that bit is set when IDs are passed to its other methods. But this is a much less obvious solution, so we are running the risk of users doing the obvious thing rather than the performant one. And, there is still the overhead of calling the subscriber methods in the first place, which could be avoided if the span was properly disabled.

Proposal

I propose changing new_span to return Option<Id> rather than Id. If a subscriber doesn't like a span's fields that are passed to new_span, it can simply return None, and the span will be disabled. It won't clone the subscriber's Dispatch ref, and all subsequent calls to enter and record, clones and drops of clones, etc, will all be no-ops that don't access the subscriber.

Since Id is represented by a NonZeroU64, it will always benefit from niche optimization when stored in an Option, so this is essentially the same as reserving the least-significant bit as a bit flag as in the approach I discussed above. However, the differences are:

1. Users don't have to be aware of the bit flag.
2. It saves an extra bit from the Id, allowing for slightly more ID values to exist concurrently (this doesn't matter all that much, since u64::MAX >> 2 is still a huge number...).
3. We avoid the overhead of calling any subscriber methods and of cloning the Dispatch for disabled spans (which checking a bit flag in the subscriber does not).

We would want to ensure that the performance difference between returning None from new_span and returning false from enabled or Interest::never from register_callsite is documented, so that Subscriber implementations know to only filter in new_span when data from a span's fields is necessary to perform filtering.

Of course, changing the method signature is inherently a breaking change, so if we make this change, it would be part of tracing-core 0.2.

Alternatives

We could, uh, not do this, and leave it up to the user to implement one of the less efficient approaches I mentioned above.

[mladedav]

I think Subscribe should also have a way to signal that it does not want to track the span.

The way I usually use tracing (although I don't know if that's how most people use it) is with the stock Registry and then adding different Subscribe implementations using CollectExt::with. The Registry itself cannot ever know whether the spans should be created or not so Layered couldn't benefit from this.

So I think that either

• Subscribe::on_new_span should keep its current arguments (including a pre-allocated span::Id) and return a bool indicating whether it is interested or not. If all inner Subscribes return false, the Collect implementation can consider the ID unused again and return None.
• New Subscribe::before_new_span(&self, attrs: &span::Attributes<'_>, ctx: Context<'_, C>) can be added. This avoids the early allocation of the span ID so some atomic operations for Registry but clutters the API so I'm not sure it's worth it.