Repository navigation
Conversation
Record child spans for canonical callbacks under the existing experimental telemetry opt-in. Preserve callback short-circuiting and cover awaited results, exceptions, cancellation, nesting and concurrency. Related: google#2354
There was a problem hiding this comment.
Code Review
This pull request introduces experimental telemetry for callback execution by recording child spans for each callback in the pipeline. The changes include updates to the callback execution logic in _callback_pipeline.py, documentation updates, and comprehensive unit tests. The reviewer suggested improving the context handling by using duck typing to check for get_invocation_context instead of an explicit isinstance check, which would also eliminate the need for a local import.
| from ..agents.context import Context | ||
| from ..telemetry import tracing | ||
|
|
||
| if not callbacks: | ||
| return None | ||
| callback_context = kwargs.get('callback_context') or kwargs.get( | ||
| 'tool_context' | ||
| ) | ||
| invocation_context = ( | ||
| callback_context.get_invocation_context() | ||
| if isinstance(callback_context, Context) | ||
| else None | ||
| ) |
There was a problem hiding this comment.
Instead of importing Context locally and performing an isinstance check, we can use duck typing to check for the presence of get_invocation_context. This is more robust, supports any context object (such as ToolContext or custom user-defined contexts) that implements get_invocation_context(), and completely avoids the local import of Context (which was introduced to prevent circular dependencies).
from ..telemetry import tracing
if not callbacks:
return None
callback_context = kwargs.get('callback_context') or kwargs.get(
'tool_context'
)
get_invocation_context = getattr(callback_context, 'get_invocation_context', None)
invocation_context = (
get_invocation_context() if callable(get_invocation_context) else None
)There was a problem hiding this comment.
ToolContext and CallbackContext are both aliases of Context in this tree, so this check already accepts both and their subclasses. I kept the concrete check because that is the context the callback API provides; duck typing would also invoke a method on unrelated objects passed under those keyword names. The local import avoids loading the agent layer when this utility module is imported. I can revisit this if custom context protocols are intended to be supported here.
|
Could someone approve the CI run and rerun the triage job? The triage job stopped on Gemini |
Link to Issue or Description of Change
Related: #2354. I asked about the scope on the issue.
Callback latency is currently included in the surrounding agent, model or tool span. This adds an
execute_callback <qualified name>child span around each canonical callback, including its awaited result, so that latency can be measured separately.The spans use the existing experimental telemetry opt-in and respect the per-request override. They do not attach callback arguments or return values. Callback ordering, exceptions and short-circuit behavior stay the same. Plugin hooks are outside this change. The TelemetryConfig developer guide describes the proposed spans and gate.
Testing Plan
37a71cf: pre-commit, full unit suites on Python 3.11–3.14, mypy comparison on 3.11–3.13, A2A 0.3 compatibility and MCP 2 compatibility.The full suites passed 19,135 tests on Python 3.11 and 19,126 on each of 3.12–3.14, with the existing skips and expected-failure results reported in the run. Mypy had 538 baseline errors and 538 branch errors on each checked version, with no new diagnostics. The fork workflow excludes
lancedbfrom dependency installation.On Windows, 61 targeted callback/tool tests passed. The broader affected suites passed 2,065 tests with one unrelated absolute-path error-message test excluded; that test also fails on unmodified main. The final new regression file fails 10 cases on unmodified main and passes with this change.
Manual End-to-End (E2E) Tests:
A fake model calls a real function tool through
Runner. I ran the same test from source and from a wheel installed into a fresh environment. Both passed the default-off, environment-on, request-on and request-off cases. Each enabled run produced eight callback spans with the expected parents; disabled runs produced none, and the final reply was unchanged.The reproduction script and saved results are on a separate fork branch. Run the first command with the source checkout and the second with the fresh wheel environment's Python:
Checklist
Additional context
The span name and existing experimental gate are the proposal raised on the issue; there has not yet been a maintainer response on those choices. This adds no public configuration field.