Python: Best effort to serialize tool def to Json for observability#7029
Python: Best effort to serialize tool def to Json for observability#7029TaoChenOSU wants to merge 5 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR hardens Python OpenTelemetry (OTel) observability by making tool-definition serialization best-effort, preventing telemetry capture from raising when tools contain non-JSON-serializable values (notably Azure SDK tool models and nested mappings).
Changes:
- Add best-effort serialization for
gen_ai.tool.definitions, including per-tool fallback that skips only the offending tool and logs a warning with its name. - Improve tool-to-OTel conversion for Azure SDK
Mapping-based tool models by usingas_dict()when available to recursively produce JSON-serializable dicts. - Add regression and behavior tests covering Azure Foundry code-interpreter tools and partial/all unserializable tool lists.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| python/packages/foundry/tests/foundry/test_foundry_chat_client.py | Adds a regression test ensuring Foundry CodeInterpreterTool telemetry tool definitions serialize correctly. |
| python/packages/core/tests/core/test_observability.py | Adds unit tests for skipping/omitting unserializable tool definitions while preserving serializable ones and warning with tool names. |
| python/packages/core/agent_framework/observability.py | Implements best-effort tool-definition serialization and as_dict() handling for Azure SDK tool model mappings; wires new serializer into span attribute extraction. |
Python Test Coverage Report •
Python Unit Test Overview
|
||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Automated Code Review
Reviewers: 4 | Confidence: 90%
✓ Correctness
This PR correctly fixes a bug where Azure SDK tool models (non-dict Mappings) caused TypeError during JSON serialization in the observability module. The fix uses
as_dict()for recursive conversion of Azure SDK models and adds a best-effort serialization wrapper that gracefully skips individual unserializable tools. The implementation is well-structured with appropriate error handling, and the tests thoroughly validate both the happy path and fallback behavior.
✓ Test Coverage
The PR adds good test coverage for the new serialization logic. The two new core tests cover partial and total serialization failure, verifying both graceful degradation and that warnings name the offending tool. The foundry regression test exercises the real Azure SDK model end-to-end through the
as_dict()path. One minor gap: the defensiveexceptaround_tools_to_dictitself raising (lines 2338-2345) has no dedicated test, and theas_dict()branch in_build_tool_otel_definitionhas no isolated unit test in core (only the integration test in foundry). These are nice-to-haves, not blocking.
✓ Failure Modes
This PR correctly implements best-effort serialization for telemetry tool definitions. The error handling is properly layered: conversion failures are caught at the top level, JSON serialization failures trigger a per-tool fallback that preserves good tools while skipping and identifying bad ones. The
as_dict()integration for Azure SDK types is sound — if it fails for any tool, the exception propagates to the outer try/except in_serialize_tool_definitionswhich logs a warning and returns None rather than crashing. No silent data corruption or exception leakage paths were found.
✓ Design Approach
I did not find a design-level issue that meets the required evidence bar. The change addresses the stated regression by converting Azure SDK mapping models through
as_dict()before observability reshaping, and the new best-effort serializer is consistent with existing behavior that already omits unparseable tool definitions rather than failing the caller.
Suggestions
- Consider adding a unit test in
test_observability.pyfor when_tools_to_dictitself raises an exception (the try/except at observability.py:2338-2345). You could mocknormalize_toolsto raise and verify that_serialize_tool_definitionsreturns None with a warning, without the caller crashing. - Consider adding a unit test in the core package for the
as_dict()branch of_build_tool_otel_definition(observability.py:2458-2459) using a mock Mapping object with anas_dictmethod, so the core tests don't depend on the foundry package to cover this path.
Automated review by TaoChenOSU's agents
Motivation & Context
A bug was discovered while working on a task that uses an agent that has the code interpreter tool. The observability module reported "TypeError: Object of type AutoCodeInterpreterToolParam is not JSON serializable". This happens because the observability module tries to serialize the tool definition to json while the object is not serilizable.
Description & Review Guide
Contribution Checklist
breaking changelabel (or add "[BREAKING]" to the title prefix, before or after any language prefix) — a workflow keeps the label and title prefix in sync automatically.