[haneric00]

Previously tool definition was not available on ADOT langchain/langgraph spans.

We modify the existing instrumentation to add full tool definitions (name, description, parameters).

We add two functions to event handlers

_get_property_value and _extract_tool_definitions both of which are copied/inspired by the upstream method of populating these attributes.

Also added tests that cover the 4 possible format paths in _extract_tool_definitions.

example span with fix:

"gen_ai.request.model": "anthropic.claude-3-haiku-20240307-v1:0",
--
"telemetry.sdk.version": "1.45.1",
"PlatformType": "Generic",
"gen_ai.usage.cache_creation.input_tokens": 0,
"telemetry.auto.version": "0.22.0.dev0-aws",
"gen_ai.tool.definitions": "[{\"type\": \"function\", \"name\": \"get_greeting\", \"description\": \"Get a greeting message for the given name.\", \"parameters\": {\"properties\": {\"name\": {\"type\": \"string\"}}, \"required\": [\"name\"], \"type\": \"object\"}}, {\"type\": \"function\", \"name\": \"get_weather\", \"description\": \"Get the current weather for a city.\", \"parameters\": {\"properties\": {\"city\": {\"type\": \"string\"}}, \"required\": [\"city\"], \"type\": \"object\"}}, {\"type\": \"function\", \"name\": \"calculate\", \"description\": \"Evaluate a math expression and return the result.\", \"parameters\": {\"properties\": {\"expression\": {\"type\": \"string\"}}, \"required\": [\"expression\"], \"type\": \"object\"}}, {\"type\": \"function\", \"name\": \"store_agent_output\", \"description\": \"Store an agent's output and classification tags in Amazon S3.\", \"parameters\": {\"properties\": {\"bucket\": {\"type\": \"string\"}, \"key\": {\"type\": \"string\"}, \"content\": {\"type\": \"string\"}, \"tags\": {\"items\": {\"type\": \"string\"}, \"type\": \"array\"}}, \"required\": [\"bucket\", \"key\", \"content\", \"tags\"], \"type\": \"object\"}}]",
"deployment.environment": "tooldefs-test",
"gen_ai.usage.cache_read.input_tokens": 0
},

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[github-actions]

Claude finished @haneric00's task in 4m 35s —— View job

---

Claude Code Review

• Check prior review comments for dedup
• Read the PR diff
• Inspect changed files
• Post review findings

Thanks for picking this up — the overall approach of adapting the upstream LangChain genai helpers and normalizing the four tool shapes is clean, and the parametrized subtest covering each format is a nice way to cover the surface.

A few findings posted inline:

• Lookup coverage (potential bug): params.get("tools") or params.get("functions") doesn't walk params_model_kwargs / additional_model_request_fields / nested fields the way every other request attribute in _set_llm_request_span_attributes does. For Bedrock paths that pass tools through model_kwargs or additional_model_request_fields, the attribute will silently be absent.
• Import ordering (nit): GEN_AI_TOOL_DEFINITIONS should sort before GEN_AI_TOOL_DESCRIPTION — isort will flip this.
• Formatting nits: trailing whitespace on line 139, needs a second blank line before class _BaseCallbackManagerInitWrapper, and the stray blank line added inside on_chat_model_start (line 204) is unrelated.

Nothing security-sensitive here; the attribute is derived purely from already-serialized provider request params.
· Branch: fix-langgraph-tools

[github-actions]

Claude finished @haneric00's task in 1m 6s —— View job

---

Claude Code Review

• Check prior review comments for dedup
• Read the PR diff
• Inspect changed files
• Post review findings

Re-reviewed on the latest commit (a29be14). My three prior inline findings are still open and still relevant, so per the dedup policy I did not repost them:

• Line 54 — isort ordering: GEN_AI_TOOL_DEFINITIONS should sort before GEN_AI_TOOL_DESCRIPTION.
• Line 139 — trailing whitespace, missing second blank line before the class, and the unrelated blank line added at line 204 in on_chat_model_start.
• Line 903 — potential coverage gap: params.get("tools") or params.get("functions") doesn't walk params_model_kwargs / additional_model_request_fields / nested fields like the rest of _set_llm_request_span_attributes does, so tools bound via Bedrock additional_model_request_fields or model_kwargs will be silently dropped.

Nothing new to flag on this push — the test matrix across the four formats and the normalization helper itself look good.
· Branch: fix-langgraph-tools

[github-actions]

Claude finished @haneric00's task in 1m 52s —— View job

---

Claude Code Review

• Check prior review comments for dedup
• Read the PR diff
• Inspect changed files
• Post review findings

Re-reviewed on the latest commit (0c8eacf). All three of my prior inline findings have been addressed on this push, so there is nothing new to post inline per the dedup policy:

• Line 53 — isort ordering: GEN_AI_TOOL_DEFINITIONS now sorts before GEN_AI_TOOL_DESCRIPTION. ✅
• Line 139 — trailing whitespace, blank-line counts, and the unrelated blank at line 204 are cleaned up. ✅
• Line 903 — the lookup is now first_not_none(...) across params / params_model_kwargs / metadata / config / config_model_kwargs / additional_model_request_fields / nested_additional_model_request_fields, matching the pattern used for every other request attribute. The new ChatBedrockConverse additional_model_request_fields subtest exercises this path. ✅

@haneric00's self-review ask to "augment tests for all models supported" was also addressed: the subtest matrix now covers 14 cases spanning ChatOpenAI (chat + responses), AzureChatOpenAI, ChatDeepSeek, ChatXAI, ChatAnthropic, ChatMistralAI, ChatGroq, ChatCohere, ChatGoogleGenerativeAI, ChatBedrock, ChatBedrockConverse, legacy OpenAI functions, built-in-tool exclusion, and the Bedrock additional_model_request_fields path — each one makes a real SDK call against a mocked HTTP transport so the tool shape reaching the callback is whatever the provider integration actually produces. Nice coverage.

The github-advanced-security CodeQL findings at lines 97/108/118 of the test file ("anthropic.com" in url, etc.) are substring checks against URLs generated by the SDKs themselves inside a test helper, not against untrusted input — they are safe here, so I did not flag them as new findings.

Nothing new to raise on this push.
· Branch: fix-langgraph-tools

[haneric00]

TODO:

1. fix tests
2. test langchain pregel

[github-actions]

Claude Code is working…

I'll analyze this and get back to you.

View job run

[github-actions]

Claude finished @haneric00's task in 1m 12s —— View job

---

Claude Code Review

• Read prior review comments for dedup
• Read the PR diff
• Inspect changed files
• Post review findings

Re-reviewed on the latest commits (63c50c8 on top of a merge from main). All three prior inline findings remain resolved, and the mock-infra cleanup looks good — nothing new to raise per the dedup policy:

• Line 53 — GEN_AI_TOOL_DEFINITIONS still sorts before GEN_AI_TOOL_DESCRIPTION. ✅
• Line 139 — trailing whitespace, blank-line counts, and the unrelated line-204 blank are still clean. ✅
• Line 903 — lookup still uses the full first_not_none(...) chain across params / params_model_kwargs / metadata / config / config_model_kwargs / additional_model_request_fields / nested_additional_model_request_fields. ✅

The 63c50c8 cleanup swapped the per-provider HTTP mock helper (_mock_provider_response + _StubTransport) for patch.object(type(llm), "_generate", ...) / patch.object(type(llm), "_should_stream", ...). This is a cleaner approach for this test since the attribute under test (gen_ai.tool.definitions) is derived from invocation_params passed to on_chat_model_start before _generate runs, so no provider HTTP response is needed. As a side effect, the three CodeQL "incomplete URL substring sanitization" findings at the old lines 97/108/118 of the test file no longer apply — the helper that triggered them is gone.

Spot-checked the 15-case subtest matrix (ChatOpenAI chat + responses, AzureChatOpenAI, ChatDeepSeek, ChatXAI, ChatMistralAI, ChatGroq, ChatGoogleGenerativeAI, ChatCohere, ChatAnthropic, ChatBedrock, ChatBedrockConverse, Anthropic built-in-tool exclusion, legacy OpenAI functions, ChatBedrockConverse additional_model_request_fields) — each one exercises a real bind_tools / bind call so the tool shape reaching the callback is whatever the provider integration actually produces, and the Bedrock additional_model_request_fields case specifically validates the lookup fix.

Nothing new to flag on this push.
· Branch: fix-langgraph-tools

[liustve]

would document this as well in sensitive data: https://github.com/aws-observability/aws-otel-python-instrumentation/blob/main/aws-opentelemetry-distro/src/amazon/opentelemetry/distro/instrumentation/langchain/README.rst#sensitive-data

[github-actions]

Claude finished @haneric00's task in 3m 12s —— View job

---

Claude Code Review

• Read prior review comments for dedup
• Read the PR diff
• Inspect changed files
• Post review findings

Re-reviewed on the latest commit (0783e63). All three prior Claude inline findings and all four of liustve's follow-up asks look addressed on this push, so per the dedup policy there is nothing new to post inline:

• Line 55 isort ordering — GEN_AI_TOOL_DEFINITIONS still sorts before GEN_AI_TOOL_DESCRIPTION. ✅
• Line 139 whitespace / blank-line nits — moot, the helper now lives in instrumentation_utils.py; on_chat_model_start no longer has the stray blank line. ✅
• Line 903 lookup coverage — the tools/functions lookup is still a full first_not_none(...) across params / params_model_kwargs / metadata / config / config_model_kwargs / additional_model_request_fields / nested_additional_model_request_fields, and the ChatBedrockConverse additional_model_request_fields subtest exercises it. ✅

liustve's follow-ups on this push:

• "move to instrumentation_utils.py" — extract_tool_definitions now lives at aws-opentelemetry-distro/src/amazon/opentelemetry/distro/instrumentation/common/instrumentation_utils.py:142. ✅
• "drop _get_property_value, use get_value" — _get_property_value is removed; extract_tool_definitions calls get_value throughout. ✅
• "serialize_to_json_string truncates nested schemas" — the call site now uses json.dumps(tool_defs, default=str) with an inline comment explaining why serialize_to_json_string is deliberately avoided (callback_handler.py:879-881). ✅
• "add a nested-pydantic test case" — the new test defines Address → Item → create_order(items: list[Item]) and asserts items[].properties.ship_to.properties.street.type == "string" at every provider (except ChatCohere, which converts away the nested schema upstream — noted in the test). ✅
• "multiple tools in the test" — bind_tools([get_weather, create_order]) is used across the subtest matrix. ✅

The README sensitive-data ask from liustve's PR comment is also handled: gen_ai.tool.definitions is added to both the bullet list and the AWS_REDACT_SPAN_ATTRIBUTES example in aws-opentelemetry-distro/src/amazon/opentelemetry/distro/instrumentation/langchain/README.rst.

Nothing new to flag on this push.
· Branch: fix-langgraph-tools

[github-actions]

Claude finished @haneric00's task in 3m 37s —— View job

---

Claude Code Review

• Read prior review comments for dedup
• Read the PR diff
• Inspect changed files
• Post review findings

Re-reviewed on the latest commit (fcdbcf9). All three of my prior inline findings and liustve's follow-up asks remain resolved, so per the dedup policy there is nothing new to post inline:

• Line 55 isort ordering — GEN_AI_TOOL_DEFINITIONS still sorts before GEN_AI_TOOL_DESCRIPTION. ✅
• Line 139 whitespace / blank-line nits — still clean; the stray blank in on_chat_model_start is still gone. ✅
• Line 903 lookup coverage — tools/functions lookup is still a full first_not_none(...) across params / params_model_kwargs / metadata / config / config_model_kwargs / additional_model_request_fields / nested_additional_model_request_fields, and the ChatBedrockConverse additional_model_request_fields subtest exercises it. ✅

Changes on this push (fcdbcf9):

• extract_tool_definitions moved from instrumentation_utils.py → callback_handler.py:1079 as a staticmethod _extract_tool_definitions. This reverses liustve's earlier "move to instrumentation-utils.py" nit but is consistent with liustve's follow-up ask that helpers live at the bottom of the file that uses them. Flagging only so the author/maintainer are aware the two nits pull in opposite directions — the current placement is a reasonable call either way.
• Serialization at callback_handler.py:881 is now serialize_to_json_string(tool_defs, max_depth=sys.maxsize) instead of json.dumps(tool_defs, default=str), and serialize_to_json_string grew a RecursionError → '"..."' fallback at instrumentation_utils.py:167-169. The new test_serialize_unlimited_depth_stops_on_self_reference covers the cycle path, and test_serialize_unlimited_depth covers the no-truncation path. Behavior is correct — a legitimately deep (non-cyclic) tool schema that exceeds Python's ~1000-frame recursion limit would also collapse to '"..."', but that is an unreachable shape for a real OpenAI/Anthropic tool spec.

Nothing new to flag on this push.
· · Branch: fix-langgraph-tools