opentelemetry-rust icon indicating copy to clipboard operation
opentelemetry-rust copied to clipboard

SpanId and TraceId improvements and their usage in SpanData

Open valkum opened this issue 1 year ago • 2 comments

Currently, SpanId and TraceId are simple new types around u128/u64.

The value of 0 as used as an invalid placeholder. In some places (e.g. with span parents) we see a pattern of Option<SpanId>. This can be optimized by size if we would use NonZeroU128/NonZeroU64

The reason for this change gets clearer if we look at sdk::SpanData. This type holds a span_parent_id and is used by exporters. When this gets constructed for a root span the parent_span_id gets set to SpanId::INVALID.

opentelemetry_stackdriver currently exports the parant_span_id without checkout the validity, which it should. A recent change in googles backend now marks spans with a parent of 0x0 as having missing parent spans.

To fix this, we can either check the validity in the exporter or change SpanId to SpanId(NonZeroU64) and use Option<SpanId> in SpanData. The optimization holds for Options of new types as well, see:

pub struct SpanId(core::num::NonZeroU64);

fn main() {
    use std::mem::size_of;
    assert_eq!(size_of::<Option<SpanId>>(), size_of::<u64>());
}

I think the Option<SpanId> way feels more ergonomic, at least for SpanData::parent_span_id.

The inner type of SpanId and TraceId is not exposed publicly. SpanData::parent_span_id is public and thus changing this would be a breaking change.

valkum avatar Jun 15 '23 18:06 valkum