feat(toolbox_core): enable telemetry_attribute for SQLCommenter#634
feat(toolbox_core): enable telemetry_attribute for SQLCommenter#634AjmeraParth132 wants to merge 4 commits intogoogleapis:mainfrom
Conversation
|
/gcbrun |
anubhav756
force-pushed
the
feat/sql-commenter
branch
from
40a4b32 to
eca4c1c
Compare
|
/gcbrun |
3 similar comments
|
/gcbrun |
|
/gcbrun |
|
/gcbrun |
anubhav756
force-pushed
the
feat/sql-commenter
branch
from
4dbea07 to
0e0f07c
Compare
|
/gcbrun |
| # Execute the core tool | ||
| # Execute the core tool, injecting telemetry_attributes if configured | ||
| if self._telemetry_attributes: | ||
| args["telemetry_attributes"] = self._telemetry_attributes |
There was a problem hiding this comment.
Maybe for a future PR, but this could technically conflict with the user/agent provided args (eg. if a tool happens to have a telemetry_attributes param). It works for now, assuming the chances for that would be low, but since we are also considering adding more APIs to the core tool to set telemetry attributes during initialization and an async setter, we could instead use that API to create a temporary copy of the core tool, if provided, assuming that API would be immutable, with the telemetry attributes cleanly injected through that setter, and leave the args untouched. Let me know if that makes sense? :)
| if meta is None: | ||
| meta = types.MCPMeta() | ||
| meta.traceparent = traceparent or None | ||
| meta.tracestate = tracestate or None | ||
| for key, value in (telemetry_payload or {}).items(): | ||
| span.set_attribute(key, value) |
There was a problem hiding this comment.
optional: Does it make sense to delegate it to the base transport to reduce redundancy?
| def _build_telemetry_payload( | ||
| self, | ||
| telemetry_attributes: Optional[TelemetryAttributes] = None, | ||
| ) -> Optional[dict[str, str]]: |
There was a problem hiding this comment.
optional: Does it make sense to somehow reuse the TelemetryAttributes pydantic model, perhaps set the serialization keys to match the ones we want? If that works, then IIUC, we might not need this helper (maybe then also use the default attribute for client.name)?
|
|
||
| async def tool_invoke( | ||
| self, tool_name: str, arguments: dict, headers: Mapping[str, str] | ||
| self, tool_name: str, arguments: dict, headers: Mapping[str, str], **kwargs |
There was a problem hiding this comment.
Curious, is there a reason we pass kwargs vs telemetry_attributes?
anubhav756
added
the
priority: p1
2 participants
The server needs client identity and user-provided metadata (model, user ID, agent ID) on tools/call to propagate into database query logs via SQLCommenter. Currently _meta only carries traceparent/tracestate
Solution:
Add telemetry_attributes to MCPMeta with serialization alias dev.mcp-toolbox/telemetry.
Accept at call time: pop telemetry_attributes from kwargs before signature validation:
Build payload in transport: always include client.name + client.version from transport state; merge user-provided attrs with client. prefix. Attach to _meta on every tools/call
Issue: #632