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

SpanId and TraceId improvements and their usage in SpanData #1115

Open
valkum opened this issue Jun 15, 2023 · 2 comments
Open

SpanId and TraceId improvements and their usage in SpanData #1115

valkum opened this issue Jun 15, 2023 · 2 comments
Labels
A-trace Area: issues related to tracing M-api triage:todo Needs to be traiged.

Comments

@valkum
Copy link
Contributor

valkum commented Jun 15, 2023

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.

@djc
Copy link
Contributor

djc commented Jun 15, 2023

See similar discussion in #1089.

@shaun-cox
Copy link
Contributor

shaun-cox commented Jun 19, 2023

I think Option<SpanId> feels way more idiomatic for Rust than SpanID::INVALID as well. Thanks for pointing it out!

Though there is one current gotcha in Tracer::build_with_context... the SpanBuilder already contains an Option<SpanId>, and the inner SpanId can be Invalid. If the Option is None, it signals to generate a new (valid) SpanId. But if the OptionisSome, it uses the (possibly Invalid) SpanId`. Would have to evaluate to see if this behavior can be changed.

@valkum valkum mentioned this issue Jun 22, 2023
4 tasks
@TommyCpp TommyCpp added the A-trace Area: issues related to tracing label Jun 26, 2023
@hdost hdost added the M-api label Feb 21, 2024
@hdost hdost added this to the Tracing API Stable milestone Feb 21, 2024
@hdost hdost added the triage:todo Needs to be traiged. label Feb 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-trace Area: issues related to tracing M-api triage:todo Needs to be traiged.
Projects
None yet
Development

No branches or pull requests

5 participants