Skip to content

Commit 59dcbfb

Browse files
fix(telemetry): let trace_function alone decide a span's status (#1309)
# Description `trace_function` opened its span with `start_as_current_span`'s defaults (`record_exception=True`, `set_status_on_exception=True`). OTel's `use_span` therefore recorded any `Exception` leaving the block and marked the span `ERROR`, on top of what the wrapper had already done: - `QueueShutDown` still ended as `ERROR` on every queue teardown, even though #1075 treats it as a non-error. In practice that is one `ERROR` `EventQueueSource.dequeue_event` span per streaming request. - Every exception was recorded twice on the span, and OTel's `Type: message` description replaced the wrapper's `str(e)`. ## Change - `src/a2a/utils/telemetry.py`: pass `record_exception=False, set_status_on_exception=False` to `start_as_current_span` in both the async and the sync wrapper. The wrapper already records the exception and sets the status itself. - `tests/utils/test_telemetry.py`: add `test_span_status_with_real_otel_sdk`. It goes through a real `TracerProvider` + `InMemorySpanExporter`, because the existing mocked tracer can't see what OTel does on exit. It is parametrized over: - `QueueShutDown`, expected not `ERROR`; - `ValueError`, expected `ERROR`. Both cases check that the span has exactly one `exception` event. Both fail on `main` and pass with this change. ## Before / after The reproduction script from #1308 (a2a-sdk `main` at 6ff0a82, opentelemetry-sdk 1.42.1): ``` # before shutdown ERROR 'AsyncQueueShutDown: ' ['exception', 'exception'] boom ERROR 'ValueError: real failure' ['exception', 'exception'] # after shutdown UNSET None ['exception'] boom ERROR 'real failure' ['exception'] ``` ## Checks - `./scripts/lint.sh`: ruff check, ruff format and ty all pass. - `uv run pytest --cov=src`: 2206 passed, 93 % coverage. - **Pre-existing failure:** `tests/integration/cross_version/client_server/test_client_server.py` fails locally on `main` too, without this change (the 0.3 server/client subprocess doesn't start on my machine). This PR doesn't touch it. - [x] Follow the [`CONTRIBUTING` Guide](https://github.com/a2aproject/a2a-python/blob/main/CONTRIBUTING.md). - [x] Make your Pull Request title in the <https://www.conventionalcommits.org/> specification. - [x] Ensure the tests and linter pass (Run `bash scripts/format.sh` from the repository root to format) - [x] Appropriate docs were updated (if necessary) Fixes #1308 🦕 Co-authored-by: Atenea Agent @svc_atenea_gitlab Co-authored-by: mykytanetipa <mykytanetipa@google.com>
1 parent 486f0f6 commit 59dcbfb

2 files changed

Lines changed: 59 additions & 2 deletions

File tree

‎src/a2a/utils/telemetry.py‎

Lines changed: 17 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -229,7 +229,15 @@ async def async_wrapper(*args, **kwargs) -> Any:
229229
tracer = trace.get_tracer(
230230
INSTRUMENTING_MODULE_NAME, INSTRUMENTING_MODULE_VERSION
231231
)
232-
with tracer.start_as_current_span(actual_span_name, kind=kind) as span:
232+
# The wrapper records exceptions and sets the span status itself, so
233+
# OTel must not do it again: it would mark non-error exceptions
234+
# (e.g. `QueueShutDown`) as ERROR and record every exception twice.
235+
with tracer.start_as_current_span(
236+
actual_span_name,
237+
kind=kind,
238+
record_exception=False,
239+
set_status_on_exception=False,
240+
) as span:
233241
if attributes:
234242
for k, v in attributes.items():
235243
span.set_attribute(k, v)
@@ -272,7 +280,14 @@ async def async_wrapper(*args, **kwargs) -> Any:
272280
def sync_wrapper(*args, **kwargs) -> Any:
273281
"""Sync Wrapper for the decorator."""
274282
tracer = trace.get_tracer(INSTRUMENTING_MODULE_NAME)
275-
with tracer.start_as_current_span(actual_span_name, kind=kind) as span:
283+
# The wrapper records exceptions and sets the span status itself;
284+
# letting OTel do it too would record every exception twice.
285+
with tracer.start_as_current_span(
286+
actual_span_name,
287+
kind=kind,
288+
record_exception=False,
289+
set_status_on_exception=False,
290+
) as span:
276291
if attributes:
277292
for k, v in attributes.items():
278293
span.set_attribute(k, v)

‎tests/utils/test_telemetry.py‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -294,3 +294,45 @@ async def non_error_exception() -> NoReturn:
294294
# the span was never marked as failed.
295295
for call in mock_span.set_status.call_args_list:
296296
assert 'description' not in call.kwargs
297+
298+
299+
@pytest.mark.asyncio
300+
@pytest.mark.parametrize(
301+
('exc_cls', 'is_error'), [(QueueShutDown, False), (ValueError, True)]
302+
)
303+
async def test_span_status_with_real_otel_sdk(
304+
exc_cls: type[Exception], is_error: bool
305+
) -> None:
306+
"""Check the status a real OTel SDK span ends with.
307+
308+
The mocked-tracer tests only see the wrapper's own calls. By default,
309+
`start_as_current_span` also records any exception leaving its block and
310+
marks the span ERROR, which would turn a `QueueShutDown` into an error
311+
span and record every exception twice.
312+
"""
313+
from opentelemetry.sdk.trace import TracerProvider
314+
from opentelemetry.sdk.trace.export import SimpleSpanProcessor
315+
from opentelemetry.sdk.trace.export.in_memory_span_exporter import (
316+
InMemorySpanExporter,
317+
)
318+
from opentelemetry.trace import StatusCode
319+
320+
exporter = InMemorySpanExporter()
321+
provider = TracerProvider()
322+
provider.add_span_processor(SimpleSpanProcessor(exporter))
323+
324+
with mock.patch(
325+
'opentelemetry.trace.get_tracer', side_effect=provider.get_tracer
326+
):
327+
328+
@trace_function
329+
async def fail() -> NoReturn:
330+
await asyncio.sleep(0)
331+
raise exc_cls
332+
333+
with pytest.raises(exc_cls):
334+
await fail()
335+
336+
(span,) = exporter.get_finished_spans()
337+
assert (span.status.status_code is StatusCode.ERROR) is is_error
338+
assert [event.name for event in span.events] == ['exception']

0 commit comments

Comments
 (0)