Skip to content

Commit f902aad

Browse files
CopilotnikhilNava
andcommitted
Fix parent_id to set on span context instead of span attribute
Changed the parent_id implementation to properly link spans by parsing the W3C Trace Context format parent ID and using it to create a proper span context. This aligns with the .NET implementation which calls `activity?.SetParentId(parentId!)`. Added validation for W3C Trace Context version and trace_id/span_id lengths. Co-authored-by: nikhilNava <211831449+nikhilNava@users.noreply.github.com>
1 parent f1e7c5d commit f902aad

File tree

2 files changed

+110
-31
lines changed

2 files changed

+110
-31
lines changed

libraries/microsoft-agents-a365-observability-core/microsoft_agents_a365/observability/core/opentelemetry_scope.py

Lines changed: 81 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -10,10 +10,19 @@
1010
from typing import TYPE_CHECKING, Any
1111

1212
from opentelemetry import baggage, context, trace
13-
from opentelemetry.trace import Span, SpanKind, Status, StatusCode, Tracer, set_span_in_context
13+
from opentelemetry.trace import (
14+
NonRecordingSpan,
15+
Span,
16+
SpanContext,
17+
SpanKind,
18+
Status,
19+
StatusCode,
20+
TraceFlags,
21+
Tracer,
22+
set_span_in_context,
23+
)
1424

1525
from .constants import (
16-
CUSTOM_PARENT_SPAN_ID_KEY,
1726
ENABLE_A365_OBSERVABILITY,
1827
ENABLE_OBSERVABILITY,
1928
ERROR_TYPE_KEY,
@@ -42,6 +51,70 @@
4251
logger = logging.getLogger(__name__)
4352

4453

54+
def _parse_parent_id_to_context(parent_id: str | None) -> context.Context | None:
55+
"""Parse a W3C trace context parent ID and return a context with the parent span.
56+
57+
The parent_id format is expected to be W3C Trace Context format:
58+
"00-{trace_id}-{span_id}-{trace_flags}"
59+
Example: "00-1234567890abcdef1234567890abcdef-abcdefabcdef1234-01"
60+
61+
Args:
62+
parent_id: The W3C Trace Context format parent ID string
63+
64+
Returns:
65+
A context containing the parent span, or None if parent_id is invalid
66+
"""
67+
if not parent_id:
68+
return None
69+
70+
try:
71+
# W3C Trace Context format: "00-{trace_id}-{span_id}-{trace_flags}"
72+
parts = parent_id.split("-")
73+
if len(parts) != 4:
74+
logger.warning(f"Invalid parent_id format (expected 4 parts): {parent_id}")
75+
return None
76+
77+
version, trace_id_hex, span_id_hex, trace_flags_hex = parts
78+
79+
# Validate W3C Trace Context version
80+
if version != "00":
81+
logger.warning(f"Unsupported W3C Trace Context version: {version}")
82+
return None
83+
84+
# Validate trace_id length (must be 32 hex chars)
85+
if len(trace_id_hex) != 32:
86+
logger.warning(f"Invalid trace_id length (expected 32 chars): {len(trace_id_hex)}")
87+
return None
88+
89+
# Validate span_id length (must be 16 hex chars)
90+
if len(span_id_hex) != 16:
91+
logger.warning(f"Invalid span_id length (expected 16 chars): {len(span_id_hex)}")
92+
return None
93+
94+
# Parse the hex values
95+
trace_id = int(trace_id_hex, 16)
96+
span_id = int(span_id_hex, 16)
97+
trace_flags = TraceFlags(int(trace_flags_hex, 16))
98+
99+
# Create a SpanContext from the parsed values
100+
parent_span_context = SpanContext(
101+
trace_id=trace_id,
102+
span_id=span_id,
103+
is_remote=True,
104+
trace_flags=trace_flags,
105+
)
106+
107+
# Create a NonRecordingSpan with the parent context
108+
parent_span = NonRecordingSpan(parent_span_context)
109+
110+
# Create a context with the parent span
111+
return set_span_in_context(parent_span)
112+
113+
except (ValueError, IndexError) as e:
114+
logger.warning(f"Failed to parse parent_id '{parent_id}': {e}")
115+
return None
116+
117+
45118
class OpenTelemetryScope:
46119
"""Base class for OpenTelemetry tracing scopes in the SDK."""
47120

@@ -106,12 +179,13 @@ def __init__(
106179
elif kind.lower() == "consumer":
107180
activity_kind = SpanKind.CONSUMER
108181

109-
# Get current context for parent relationship
110-
current_context = context.get_current()
182+
# Get context for parent relationship
183+
# If parent_id is provided, parse it and use it as the parent context
184+
# Otherwise, use the current context
185+
parent_context = _parse_parent_id_to_context(parent_id)
186+
span_context = parent_context if parent_context else context.get_current()
111187

112-
self._span = tracer.start_span(
113-
activity_name, kind=activity_kind, context=current_context
114-
)
188+
self._span = tracer.start_span(activity_name, kind=activity_kind, context=span_context)
115189

116190
# Log span creation
117191
if self._span:
@@ -149,9 +223,6 @@ def __init__(
149223
if tenant_details:
150224
self.set_tag_maybe(TENANT_ID_KEY, str(tenant_details.tenant_id))
151225

152-
# Set parent ID if provided
153-
self.set_tag_maybe(CUSTOM_PARENT_SPAN_ID_KEY, parent_id)
154-
155226
def record_error(self, exception: Exception) -> None:
156227
"""Record an error in the span.
157228

tests/observability/core/test_output_scope.py

Lines changed: 29 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -14,10 +14,7 @@
1414
get_tracer_provider,
1515
)
1616
from microsoft_agents_a365.observability.core.config import _telemetry_manager
17-
from microsoft_agents_a365.observability.core.constants import (
18-
CUSTOM_PARENT_SPAN_ID_KEY,
19-
GEN_AI_OUTPUT_MESSAGES_KEY,
20-
)
17+
from microsoft_agents_a365.observability.core.constants import GEN_AI_OUTPUT_MESSAGES_KEY
2118
from microsoft_agents_a365.observability.core.models.response import Response
2219
from microsoft_agents_a365.observability.core.opentelemetry_scope import OpenTelemetryScope
2320
from microsoft_agents_a365.observability.core.spans_scopes.output_scope import OutputScope
@@ -223,9 +220,13 @@ def test_output_scope_span_name(self):
223220
self.assertIn(self.agent_details.agent_id, span.name)
224221

225222
def test_output_scope_with_parent_id(self):
226-
"""Test that OutputScope records parent_id when provided."""
223+
"""Test that OutputScope uses parent_id to link span to parent."""
227224
response = Response(messages=["Test message with parent"])
228-
parent_id = "00-1234567890abcdef1234567890abcdef-abcdefabcdef1234-01"
225+
# W3C Trace Context format: "00-{trace_id}-{span_id}-{trace_flags}"
226+
# trace_id: 32 hex chars, span_id: 16 hex chars
227+
parent_trace_id = "1234567890abcdef1234567890abcdef"
228+
parent_span_id = "abcdefabcdef1234"
229+
parent_id = f"00-{parent_trace_id}-{parent_span_id}-01"
229230

230231
scope = OutputScope.start(
231232
self.agent_details, self.tenant_details, response, parent_id=parent_id
@@ -238,18 +239,28 @@ def test_output_scope_with_parent_id(self):
238239
self.assertTrue(finished_spans, "Expected at least one span to be created")
239240

240241
span = finished_spans[-1]
241-
span_attributes = getattr(span, "attributes", {}) or {}
242242

243-
# Verify the parent ID is set as a span attribute
244-
self.assertIn(
245-
CUSTOM_PARENT_SPAN_ID_KEY,
246-
span_attributes,
247-
"Expected custom parent span ID to be set on span",
243+
# Verify the span has the correct parent trace context
244+
# The span's trace_id should match the parent's trace_id
245+
span_trace_id = f"{span.context.trace_id:032x}"
246+
self.assertEqual(
247+
span_trace_id,
248+
parent_trace_id,
249+
"Expected span's trace_id to match parent's trace_id",
250+
)
251+
252+
# The span's parent_span_id should match the parent's span_id
253+
span_parent_id = None
254+
if span.parent and hasattr(span.parent, "span_id"):
255+
span_parent_id = f"{span.parent.span_id:016x}"
256+
self.assertEqual(
257+
span_parent_id,
258+
parent_span_id,
259+
"Expected span's parent_span_id to match parent's span_id",
248260
)
249-
self.assertEqual(span_attributes[CUSTOM_PARENT_SPAN_ID_KEY], parent_id)
250261

251262
def test_output_scope_without_parent_id(self):
252-
"""Test that OutputScope doesn't set parent_id attribute when not provided."""
263+
"""Test that OutputScope creates a span without forced parent when not provided."""
253264
response = Response(messages=["Test message without parent"])
254265

255266
scope = OutputScope.start(self.agent_details, self.tenant_details, response)
@@ -261,14 +272,11 @@ def test_output_scope_without_parent_id(self):
261272
self.assertTrue(finished_spans, "Expected at least one span to be created")
262273

263274
span = finished_spans[-1]
264-
span_attributes = getattr(span, "attributes", {}) or {}
265275

266-
# Verify the parent ID attribute is NOT set when not provided
267-
self.assertNotIn(
268-
CUSTOM_PARENT_SPAN_ID_KEY,
269-
span_attributes,
270-
"Expected custom parent span ID NOT to be set when not provided",
271-
)
276+
# When no parent_id is provided, the span should either have no parent
277+
# or inherit from the current context (which in tests is typically empty)
278+
# We just verify the span was created successfully
279+
self.assertIsNotNone(span.context.span_id)
272280

273281

274282
if __name__ == "__main__":

0 commit comments

Comments
 (0)