fix(langchain): correct unknown role in completion spans #3532
Conversation
📝 WalkthroughWalkthroughset_chat_response now derives a generation's role from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.py📄 CodeRabbit inference engine (CLAUDE.md)
Files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
🔇 Additional comments (1)
Comment |
OzBenSimhonTraceloop
changed the title
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to e0628cb in 35 seconds. Click for details.
- Reviewed
172lines of code in2files - Skipped
0files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. packages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/span_utils.py:215
- Draft comment:
Good fix extracting the role fromgeneration.message.typeinstead ofgeneration.type. Consider usinggetattr(generation.message, 'type', None)for a more succinct and safe attribute access. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
2. packages/opentelemetry-instrumentation-langchain/tests/test_generation_role_extraction.py:118
- Draft comment:
The test suite comprehensively covers various scenarios. For extra robustness, consider adding a test for an unexpected or unknownmessage.typeto ensure it correctly returns 'unknown'. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
Workflow ID: wflow_aTnbI0giSCW6gFzw
You can customize 
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/span_utils.py (1)
278-284: Redundant role assignment when tool_calls are present.The role is already determined and set at lines 216-226. Lines 280-283 unconditionally reassign
role = "assistant"whentool_callsare present, which could:
- Override a correctly derived role from earlier logic
- Create maintenance confusion with duplicate role assignments
While "assistant" is typically correct for tool_calls, this hardcoded override is inconsistent with the earlier conditional logic.
🔎 Suggested refactor to remove redundant assignment
# Handle new tool_calls format (multiple tool calls) tool_calls = ( generation.message.tool_calls if hasattr(generation.message, "tool_calls") else generation.message.additional_kwargs.get("tool_calls") ) if tool_calls and isinstance(tool_calls, list): - _set_span_attribute( - span, - f"{prefix}.role", - "assistant", - ) _set_chat_tool_calls(span, prefix, tool_calls)The role has already been set at line 222-226, so this reassignment is unnecessary.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/span_utils.pypackages/opentelemetry-instrumentation-langchain/tests/test_generation_role_extraction.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Store API keys only in environment variables/secure vaults; never hardcode secrets in code
Use Flake8 for code linting and adhere to its rules
Files:
packages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/span_utils.pypackages/opentelemetry-instrumentation-langchain/tests/test_generation_role_extraction.py
🧬 Code graph analysis (1)
packages/opentelemetry-instrumentation-langchain/tests/test_generation_role_extraction.py (1)
packages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/span_utils.py (1)
set_chat_response(206-285)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Test Packages (3.11)
- GitHub Check: Test Packages (3.10)
- GitHub Check: Build Packages (3.11)
- GitHub Check: Test Packages (3.12)
- GitHub Check: Lint
🔇 Additional comments (2)
packages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/span_utils.py (1)
214-221: LGTM! Root cause correctly addressed.The fix properly extracts role from
generation.message.type(message types like "ai", "tool") instead ofgeneration.type(class names like "ChatGeneration"). The defensive checks and fallback to "assistant" for legacy Generation objects are appropriate.packages/opentelemetry-instrumentation-langchain/tests/test_generation_role_extraction.py (1)
1-142: Excellent test coverage for the role extraction fix.The test suite comprehensively validates:
- AIMessage → "assistant" role
- ToolMessage → "tool" role
- Legacy Generation → "assistant" fallback
- Mixed generation types with correct per-index roles
- Explicit verification that
generation.type(class name) is not used as the sourceThe test at lines 130-131 is particularly valuable as it documents the bug:
generation.type == "ChatGeneration"vs. the correctgeneration.message.type == "ai".
| for generation in generations: | ||
| prefix = f"{GenAIAttributes.GEN_AI_COMPLETION}.{i}" | ||
|
|
||
| # Extract role from generation message if available, otherwise default to assistant |
There was a problem hiding this comment.
this comment is redundant imo, the other on is essential
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 5e050d6 in 28 seconds. Click for details.
- Reviewed
12lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. packages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/span_utils.py:215
- Draft comment:
The outdated comment was removed and the logic properly extracts the role from generation.message.type, with a fallback to 'assistant' for legacy Generation objects. This fix aligns with the intended behavior. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_8kmfqUaTFMWJZWgo
You can customize
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/span_utils.py (1)
278-282: Remove redundant role override that defeats the PR's fix.The role is being hardcoded to "assistant" here, which overwrites the carefully determined role from lines 215-219. This is inconsistent with the PR's objective to correctly extract roles from
generation.message.type.Impact:
- For AIMessages with tool_calls: redundant (role is already "assistant")
- For any other message type with tool_calls: incorrectly changes the role to "assistant"
- Defeats the purpose of the role extraction logic introduced in this PR
Note: The
set_chat_requestfunction (lines 173-182) handles tool_calls without modifying the role, showing the inconsistency.🔧 Proposed fix: Remove the redundant role assignment
if tool_calls and isinstance(tool_calls, list): - _set_span_attribute( - span, - f"{prefix}.role", - "assistant", - ) _set_chat_tool_calls(span, prefix, tool_calls)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/span_utils.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Store API keys only in environment variables/secure vaults; never hardcode secrets in code
Use Flake8 for code linting and adhere to its rules
Files:
packages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/span_utils.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Test Packages (3.10)
- GitHub Check: Test Packages (3.12)
- GitHub Check: Lint
- GitHub Check: Test Packages (3.11)
- GitHub Check: Build Packages (3.11)
🔇 Additional comments (1)
packages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/span_utils.py (1)
215-219: LGTM! Role extraction logic is correct.The defensive checks ensure safe access to
generation.message.type, and the fallback to "assistant" for non-chat completions is appropriate.
Description
Fixes completion roles showing as "unknown" in LangChain instrumentation traces.
Root Cause
The code was incorrectly using
generation.typewhich returns the class name ("ChatGeneration","Generation") instead of extracting the message type fromgeneration.message.type("ai","tool", etc.).Since the
_message_type_to_role()function expects message types, passing class names resulted in the default case returning"unknown".Changes
set_chat_response()inspan_utils.pyto extract role fromgeneration.message.typewhen available"assistant"for legacyGenerationobjects (non-chat completions) that don't have a message wrapper"assistant"role"tool"role"assistant"role (default)Testing
test_completion_role_fix.pyScreenshots
[Add before/after screenshots from your trace UI]
Before: Completions showing
role: "unknown"After: Completions showing
role: "assistant"androle: "tool"Important
Fixes role extraction in LangChain completion spans by using message type instead of class name, with comprehensive tests added.
set_chat_response()inspan_utils.pyby usinggeneration.message.typeinstead ofgeneration.type.Generationobjects without a message.test_generation_role_extraction.pyto validate role extraction forChatGenerationwithAIMessageandToolMessage, andGenerationwithout a message.generation.typeis not used directly.This description was created by
for 5e050d6. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.