Design of SpanBuilder not aligned with goals of sampling?

[shaun-cox]

A mutable SpanBuilder is given to Tracer::build_with_context, which is intended to make a decision about whether the returned Span will be recording (or not) or sampled (or not). The whole goal of this api is to efficiently abstract away details which may impact the performance of instrumentation from the actual instrumenting library.

Part of the input to the decision whether to record/sample the span is the set of attributes provided by the SpanBuilder. But these attributes (and all the other information in the builder) is given up (taken away from the builder) when Tracer::build_with_context is called... even for 99.9% of all spans which are not recording in the case when sdk configures 0.1% sampling rate.

IOW, if the goal of well-performing instrumentation is to avoid unnecessary work (especially memory allocations) when instrumentation is not being recorded, then why/how does it make sense to require the API caller to allocate and give up a set of attributes just to make the sampling decision? Shouldn't the API take this information by immutable reference, then copy from it in the rare cases when the span will be selected for recording/sampling?

Referring to the spec:

The API documentation MUST state that adding attributes at span creation is preferred to calling SetAttribute later, as samplers can only consider information already present during span creation.

I think that in order to align with the intention of minimal performance overhead when not recording, we have to allow a set of immutably borrowed attributes to be the basis of the sampling decision, not ones where ownership must be given away to make each decision and initially populate the span attributes.

@jtescher, @TommyCpp, @djc would love your opinions on this. Thanks!

[TommyCpp]

which is intended to make a decision about whether the returned Span will be recording (or not) or sampled (or not).

I'd say it's part of the job of Tracer::build_with_context. Additionally, if it's decided it should be sampling. The function will append attributes returned as part of the sampling decision and enforce the limit on attributes, links, and events.

Since in build_with_context the SpanBuilder's ownership is transferred from the caller to the function. I don't think the overhead is as heavy as a deep copy. Having a mutable SpanBuilder enable us to transfer the data inside the builder using the Option::take function. Thus, it avoid the overhead to deep copy the attributes/links/events when span is sampled.

Grant that move semantics may still have an overhead compared with the reference. I think the compiler should be smart enough to optimize it(I could be wrong on this though given the SpanBuilder does contains a lot of fields and is not something simple like u64)

[shaun-cox]

Yeah, shallow vs. deep copy is definitely good here, but it's the required memory allocation just to pass attributes to a SpanBuilder, and the subsequent freeing of that allocation when starting a Span (that is not selected to record/sample) I'm more concerned about.

Creating a SpanBuilder with attributes, uses this:

    /// Assign span attributes from an iterable.
    ///
    /// Check out [`SpanBuilder::with_attributes_map`] to assign span attributes
    /// via an [`OrderMap`] instance.
    pub fn with_attributes<I>(self, attributes: I) -> Self
    where
        I: IntoIterator<Item = KeyValue>,
    {
        SpanBuilder {
            attributes: Some(OrderMap::from_iter(attributes.into_iter())),
            ..self
        }
    }

The internal OrderMap needs to allocate to hold however many items are iterated. (Not to mention that it may extend while iterating, thus allocating, free, and re-allocating before it builds the entire map.)

For the cases when a callsite builds many spans, but wants to record the same initial attributes for all of them, it would make more sense for the callsite to allocate the attribute map once, pass it by reference, then defer the above assignment of those attributes into the SpanData when/if the Span is selected for recording. The allocation behavior will end up being no different than current case if every span is recorded, but it will be far less if not every span is recorded.

[TommyCpp]

The generic parameters in with_attribute allow users to pass different collections of attributes and I think it's handy but not performant. We have the following method to allow users to use OrderMap as parameter and avoid any allocation when building the SpanBuilder

    pub fn with_attributes_map(self, attributes: OrderMap<Key, Value>) -> Self {
        SpanBuilder {
            attributes: Some(attributes),
            ..self
        }
    }

Not to mention that it may extend while iterating, thus allocating, free, and re-allocating before it builds the entire map

Depends on the implementation of the iterator but most of the iterators from bounded collection will implement the size_hint method, which can help OrderMap pre-allocate the memory

For the cases when a callsite builds many spans, but wants to record the same initial attributes for all of them, it would make more sense for the callsite to allocate the attribute map once, pass it by reference, then defer the above assignment of those attributes into the SpanData when/if the Span is selected for recording. The allocation behavior will end up being no different than current case if every span is recorded, but it will be far less if not every span is recorded.

Yeah I do agree if users are building multiple spans with the same attributes at the same time having a reference is more performant. However in cases where the users only build one span at the callsite. Having a reference means creating a recorded span that needs to clone the attributes

[davidbarsky]

The devil is in the details, but a lot of the discussion here (especially with regard to lazy formatting and avoidance of allocations) is why tracing is designed the way that it is.

(I won't weight in here directly as my opinion isn't really important, but I obviously think that tracing is useful prior art.