Skip to content

Conversation

@ianchi
Copy link
Contributor

@ianchi ianchi commented Nov 25, 2025

This fixes an error introduced by #3138 that records the wrong role it uses the generation type instead of the message type.
The generation type is only for serialization and is NOT "ai"/"human".


Important

Fixes incorrect role assignment in set_chat_response by using message type instead of generation type in span_utils.py.

  • Bug Fix:
    • Corrects role assignment in set_chat_response in span_utils.py by using generation.message.type instead of generation.type.
    • Ensures role is accurately set to "assistant" when generation.message is not present.

This description was created by Ellipsis for 2a6c479. You can customize this summary. It will automatically update as commits are pushed.

Summary by CodeRabbit

  • Bug Fixes

    • Chat-response spans now derive roles from the message type and retain that role even when tool calls occur, ensuring accurate role labeling and consistent content attribution for AI responses and tool interactions.
  • Tests

    • Added tests that validate role assignment, tool-call recording (including function_call and parsed tool_calls), and content storage across various AI message scenarios; test fixture enables trace-content for the scope.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link

coderabbitai bot commented Nov 25, 2025

Walkthrough

Derives chat-response role from generation.message.type (via _message_type_to_role) and removes the prior hard-coded role based on generation.type; tool-call handling remains but no longer forces a fallback role when tool calls exist. Tests added for these behaviors.

Changes

Cohort / File(s) Change Summary
Role assignment refactor
packages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/span_utils.py
Removed unconditional prefix.role set from generation.type; now sets role from generation.message.type via _message_type_to_role when message exists. Removed fallback that forced "assistant" when tool calls exist. Tool-call processing (_set_chat_tool_calls) remains.
Tests for span behavior
packages/opentelemetry-instrumentation-langchain/tests/test_span_utils.py
Added tests verifying role mapping from AIMessage types, handling of function_call in additional_kwargs and tool_calls (including JSON arg parsing and id/name extraction), and default role/content behavior for plain Generation. Includes fixture enabling trace content and helper to build span/LLMResult.

Sequence Diagram(s)

sequenceDiagram
  participant Caller as Instrumentation code
  participant set_chat_response as set_chat_response
  participant RoleMap as _message_type_to_role
  participant ToolHandler as _set_chat_tool_calls

  Caller->>set_chat_response: call with generation (may include message, tool_calls, additional_kwargs)
  alt message exists
    set_chat_response->>RoleMap: _message_type_to_role(generation.message.type)
    RoleMap-->>set_chat_response: role
    set_chat_response->>set_chat_response: set prefix.role from role
  else no message
    set_chat_response-->>set_chat_response: skip message-derived role
  end
  alt tool_calls or function_call present
    set_chat_response->>ToolHandler: _set_chat_tool_calls(generation.tool_calls / function_call)
    ToolHandler-->>set_chat_response: tool call attrs set on span
  end
  set_chat_response-->>Caller: return (span attrs include role and any tool call attrs)
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Inspect _message_type_to_role coverage for all message types.
  • Verify ordering and side effects between role assignment and _set_chat_tool_calls.
  • Review tests for edge cases (missing message, malformed tool call args).

Poem

🐰 I nibbled code beneath the moon,

I mapped each message, set its tune.
Tool calls queued, but roles took flight,
Span attrs snug in soft twilight.
A hop, a test — the trace is bright.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main fix: correcting an incorrect role assignment on generations in the LangChain instrumentation, which is the primary change across all modified files.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

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.

📥 Commits

Reviewing files that changed from the base of the PR and between b60fc7b and 8581e9e.

📒 Files selected for processing (2)
  • packages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/span_utils.py (2 hunks)
  • packages/opentelemetry-instrumentation-langchain/tests/test_span_utils.py (1 hunks)
✅ Files skipped from review due to trivial changes (1)
  • packages/opentelemetry-instrumentation-langchain/tests/test_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: Test Packages (3.11)
  • GitHub Check: Lint
  • GitHub Check: Build Packages (3.11)
🔇 Additional comments (2)
packages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/span_utils.py (2)

242-246: Inconsistency between PR objectives and implementation.

The PR objectives state: "ensures the role is accurately set to 'assistant' when generation.message is not present." However, the code only sets the role when generation.message exists (line 241 condition). There is no else clause to set a default "assistant" role when the message is absent.

Based on the past review comments and AI summary ("no longer forces a fallback role"), this appears to be intentional. However, the PR objectives description is misleading and should be corrected to reflect the actual behavior: "ensures the role is set from generation.message.type when available; no role is set for generations without a message."


242-246: Correct fix for role assignment.

The change to use generation.message.type instead of generation.type is correct. As noted in the past review comments, generation.type only contains serialization identifiers ("Generation" or "ChatGeneration") and cannot be used to determine the message role. Using generation.message.type properly extracts the actual message type ("ai", "human", "system", "tool") for role assignment.

Note: The PR objectives state the role is set to "assistant" when generation.message is not present, but the code only sets the role when generation.message exists (lines 241-246). Verify this discrepancy is intentional, as the implementation removes the fallback role behavior described in the objectives.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

Looks good to me! 👍

Reviewed everything up to 2a6c479 in 1 minute and 13 seconds. Click for details.
  • Reviewed 41 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 draft 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:211
  • Draft comment:
    Good removal of the incorrect use of generation.type. The role should be derived from generation.message.type, which avoids misclassification.
  • Reason this comment was not posted:
    Comment looked like it was already resolved.
2. packages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/span_utils.py:263
  • Draft comment:
    The new block sets the role attribute using generation.message.type after handling tool calls, with a fallback to 'assistant'. This looks correct and aligns with the intended design.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50% This comment is purely informative and does not provide any actionable feedback or suggestions for improvement. It simply states that the code aligns with the intended design, which is not necessary for the PR author to know.

Workflow ID: wflow_vKaihN1GMn6Uk4xL

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

Copy link
Member

@nirga nirga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ianchi - can you add a test for this?

@ianchi
Copy link
Contributor Author

ianchi commented Nov 25, 2025

Hi @nirga I added a test and aligned the "completions" caso to OpenAI, (no role at all).

@ianchi ianchi requested a review from nirga November 25, 2025 23:53
@nirga
Copy link
Member

nirga commented Nov 27, 2025

@ianchi it should always be with a role. Can you walk me through what are you looking to fix?

@ianchi
Copy link
Contributor Author

ianchi commented Nov 28, 2025

I added a detailed explanation of the error, with printscreen and code snippets, in this comment.
Basically, right now any llm response "without" a tool_calls is logged in a span with .role = unknown instead of "assistant".
This is so because current code tries to extract the "role" from the wrong object. (it uses generation.type instead of generation.message.type).

Please let me know if any additional info is required.

Regarding this:

it should always be with a role.

As far as I undertand the openai implementation, in the case of a plain "completion" (not a chat completion) does NOT add a "role" to the span.
Look at opentelemetry.instrumentation.openai.shared.completion_wrappers the contend is added in _handle_response -> _set_completions which only adds attributes gen_ai.completion.{i}.finish_reason and .content it nevers adds a .role attribute.

In the case of "chat completions" the _set_completions does add a .role, extracting it from the message.

From your previous comment:

yes but look at the OpenAI instrumentation - we differentiate between tool call responses and textual ai responses

I understood that you wanted to mirror this behaviour in the Langchain instrumentation also. And thus I also modified the "textual response" case.
But if you prefer it to return role="assistant", I can add it.

@ianchi
Copy link
Contributor Author

ianchi commented Dec 2, 2025

Hi @nirga, any comments on this?
Let me know if anything else is needed

@ianchi
Copy link
Contributor Author

ianchi commented Dec 4, 2025

@nirga Friendly ping.

We are still getting all responses as role=unkown.
We need a quick fix for this.

Thanks

set_chat_response(span, result)

prefix = f"{GenAIAttributes.GEN_AI_COMPLETION}.0"
assert prefix + ".role" not in span.attributes
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it should always have a role - it should be "assistant" in this case

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants