SDK: skip completely empty span links (invalid SpanContext, no attributes, no TraceState) consistently in StartSpan / AddLink / AddLinks #4749
Description
Is your feature request related to a problem?
Location (open-telemetry/opentelemetry-cpp @ default branch 3ee38aa):
sdk/src/trace/span.cc:86-90:Span::Spansdk/src/trace/span.cc:170-180:Span::AddLinksdk/src/trace/span.cc:182-194:Span::AddLinks
links.ForEachKeyValue([&](const opentelemetry::trace::SpanContext &span_context,
const common::KeyValueIterable &attributes) {
recordable_->AddLink(span_context, attributes);
return true;
});Problem
Code that builds links from an upstream context, for example batch or messaging consumers that link to each incoming message's context, sometimes gets an invalid SpanContext: all-zero trace and span IDs, no TraceState, and the caller passes no attributes. In all three places above, every link goes directly to recordable_->AddLink(...) without any check, so the link is exported as an entry with no identifiers and no metadata. The entry tells a backend nothing, and it still uses one slot of SpanLimits::link_count_limit (default 128).
To be clear about scope: the spec does not require dropping invalid-context links in general, and I am not asking for that. This is a small data-quality improvement for links that carry no information at all. Valid links and meaningful zero-ID links are unaffected.
Reproduction
Using InMemorySpanExporter:
tracer->StartSpan("root", {}, {{SpanContext(false, false), {}}})->End();
// exported span has links.size() == 1 (link with all-zero IDs, no attributes)
tracer->StartSpan("root", {},
{{SpanContext(false, false), {}},
{SpanContext(false, false), {{"name", "neighbor"}}}})->End();
// exported span has links.size() == 2tracer_link_validation_test.cc:58: Failure
Expected equality of these values:
0
links.size()
Which is: 1
...
tracer_link_validation_test.cc:99: Failure
Expected equality of these values:
1
links.size()
Which is: 2
Environment:
- Commit:
e854d29.sdk/src/trace/span.ccis byte-identical on currentmain3ee38aa. - Toolchain: GCC 11.5.0
Describe the solution you'd like
Skip a link only when all three of these hold: its SpanContext is not valid (!IsValid()), its attribute set is empty, and its TraceState is empty. Links with an invalid context but non-empty attributes or TraceState keep being recorded. This matches the spec (v1.62.0, Trace API §Link): "Implementations SHOULD record links containing SpanContext with empty TraceId or SpanId (all zeros) as long as either the attribute set or TraceState is non-empty."
Already implemented in open-telemetry/opentelemetry-python
PR: open-telemetry/opentelemetry-python#3991 — "Validate links at span creation" (merged 2024-06-24T16:26:21Z, merge commit d474e7a2b0e). It applies the same "no valid context, no attributes, no tracestate" rule to links passed at span creation, by filtering them through _is_valid_link before they are stored. It keeps invalid-context links that have attributes or tracestate.
How opentelemetry-python fixed it:
--- a/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py
+++ b/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py
@@ -816,25 +816,30 @@ def __init__(
)
self._events.append(event)
- if links is None:
- self._links = self._new_links()
- else:
- for link in links:
- link._attributes = BoundedAttributes(
- self._limits.max_link_attributes,
- link.attributes,
- max_value_len=self._limits.max_attribute_length,
- )
- self._links = BoundedList.from_seq(self._limits.max_links, links)
+ self._links = self._new_links(links)
def __repr__(self):
return f'{type(self).__name__}(name="{self._name}", context={self._context})'
def _new_events(self):
return BoundedList(self._limits.max_events)
- def _new_links(self):
- return BoundedList(self._limits.max_links)
+ def _new_links(self, links: Sequence[trace_api.Link]):
+ if not links:
+ return BoundedList(self._limits.max_links)
+
+ valid_links = []
+ for link in links:
+ if link and _is_valid_link(link.context, link.attributes):
+ # pylint: disable=protected-access
+ link._attributes = BoundedAttributes(
+ self._limits.max_link_attributes,
+ link.attributes,
+ max_value_len=self._limits.max_attribute_length,
+ )
+ valid_links.append(link)
+
+ return BoundedList.from_seq(self._limits.max_links, valid_links)
def get_span_context(self):
return self._contextSuggested fix
Skip a link in StartSpan, AddLink and AddLinks when its SpanContext is invalid and it has no attributes and no TraceState:
diff --git a/sdk/src/trace/span.cc b/sdk/src/trace/span.cc
--- a/sdk/src/trace/span.cc
+++ b/sdk/src/trace/span.cc
@@ -50,6 +50,16 @@ SteadyTimestamp NowOr(const SteadyTimestamp &steady)
return steady;
}
}
+
+// A link without a valid context, attributes or trace state carries no information.
+// https://opentelemetry.io/docs/specs/otel/trace/api/#link
+bool IsEmptyLink(const opentelemetry::trace::SpanContext &span_context,
+ const common::KeyValueIterable &attributes) noexcept
+{
+ const auto &trace_state = span_context.trace_state();
+ return !span_context.IsValid() && attributes.size() == 0 &&
+ (!trace_state || trace_state->Empty());
+}
} // namespace
Span::Span(std::shared_ptr<Tracer> &&tracer,
@@ -85,7 +95,10 @@ Span::Span(std::shared_ptr<Tracer> &&tracer,
links.ForEachKeyValue([&](const opentelemetry::trace::SpanContext &span_context,
const common::KeyValueIterable &attributes) {
- recordable_->AddLink(span_context, attributes);
+ if (!IsEmptyLink(span_context, attributes))
+ {
+ recordable_->AddLink(span_context, attributes);
+ }
return true;
});
@@ -176,7 +189,10 @@ void Span::AddLink(const opentelemetry::trace::SpanContext &target,
return;
}
- recordable_->AddLink(target, attrs);
+ if (!IsEmptyLink(target, attrs))
+ {
+ recordable_->AddLink(target, attrs);
+ }
}
void Span::AddLinks(const opentelemetry::trace::SpanContextKeyValueIterable &links) noexcept
@@ -189,7 +205,10 @@ void Span::AddLinks(const opentelemetry::trace::SpanContextKeyValueIterable &lin
links.ForEachKeyValue([&](const opentelemetry::trace::SpanContext &span_context,
const common::KeyValueIterable &attributes) {
- recordable_->AddLink(span_context, attributes);
+ if (!IsEmptyLink(span_context, attributes))
+ {
+ recordable_->AddLink(span_context, attributes);
+ }
return true;
});
}Verified on e854d29 (GCC 11.4.0, googletest 1.14.0, SDK and tests compiled directly with g++): the test above fails without this patch and passes with it, and a link with an invalid context but a non-empty TraceState is still recorded; tracer_test and span_data_test pass with it. AddLink/AddLinks (ABI v2) were compile-checked only.
I can submit a PR for this.
Describe alternatives you've considered
- Have every caller check
IsValid()before adding a link. This works, but every instrumentation has to repeat the check, and it is easy to forget for links passed at span creation. - Filter in exporters. That is too late, because the empty link has already used up the link limit.
Additional context
Checked: #3037 (invalid span from GetSpan) and #4046 (span limit configuration) are unrelated to link filtering.
Activity