Skip to content

fix: export parent span identifiers - #59

Open
incidere wants to merge 1 commit into
open-telemetry:mainfrom
incidere:fix/parent-span-id
Open

fix: export parent span identifiers#59
incidere wants to merge 1 commit into
open-telemetry:mainfrom
incidere:fix/parent-span-id

Conversation

@incidere

Copy link
Copy Markdown

Fixes #55.

What changed

  • retain the parent span ID when a span starts from a parent context
  • preserve it through the batching processor
  • emit it from writer exporters and OTLP

Validation

  • zig fmt --check opentelemetry-sdk/src/api/trace/span.zig opentelemetry-sdk/src/sdk/trace/provider.zig opentelemetry-sdk/src/sdk/trace/span_processor.zig opentelemetry-sdk/src/sdk/trace/exporters/generic.zig opentelemetry-sdk/src/sdk/trace/exporters/otlp.zig
  • zig build sdk-test -Dtarget=x86_64-linux-musl — 291 tests passed
  • zig build sdk-examples -Dtarget=x86_64-linux-musl

@inge4pres inge4pres left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great stuff @incidere thanks for fixing this 🙏🏼
Couple of suggestions, other than those LGTM 👍🏼

/// Span represents a single operation within a trace.
pub const Span = struct {
span_context: SpanContext,
parent_span_id: ?trace.SpanID,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you omit the default null value assignment on purpose?
Since this is a public API field, adding the optional field would break compilation for all users.

Suggested change
parent_span_id: ?trace.SpanID,
parent_span_id: ?trace.SpanID = null,


// Create the span with instrumentation scope
var span = trace_api.Span.init(allocator, span_context, span_name, options.kind, self.scope);
span.parent_span_id = if (parent_span_context) |parent_sc| parent_sc.span_id else null;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The spec says an invalid parent SpanContext makes the new span a root span.
Since we don't check the parent when we extract it from context, so we should add a validity check

Suggested change
span.parent_span_id = if (parent_span_context) |parent_sc| parent_sc.span_id else null;
span.parent_span_id = if (parent_span_context) |parent_sc|
(if (parent_sc.span_id.isValid()) parent_sc.span_id else null)
else
null;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Trace: exported spans never carry parent_span_id, so traces are flat

2 participants