Skip to content

fix(openai): dump pydantic input message #2979

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jun 9, 2025

Conversation

dinmukhamedm
Copy link
Collaborator

@dinmukhamedm dinmukhamedm commented Jun 5, 2025

Fixes #2976

  • I have added tests that cover my changes.
  • If adding a new instrumentation or changing an existing one, I've added screenshots from some observability platform showing the change.
  • PR name follows conventional commits format: feat(instrumentation): ... or fix(instrumentation): ....
  • (If applicable) I have updated the documentation accordingly.

Important

Fixes handling of Pydantic input messages in OpenAI instrumentation by converting them to dicts and adds tests for verification.

  • Behavior:
    • Fixes handling of Pydantic input messages in _set_prompts() in chat_wrappers.py by converting them to dicts using model_as_dict().
  • Tests:
    • Adds test_chat_history_message_dict and test_chat_history_message_pydantic in test_chat.py to verify correct handling of dict and Pydantic message formats.
    • Includes new YAML test cassettes test_chat_history_message_dict.yaml and test_chat_history_message_pydantic.yaml for VCR testing.

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

Sorry, something went wrong.

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 e62514a in 1 minute and 23 seconds. Click for details.
  • Reviewed 565 lines of code in 4 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 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-openai/opentelemetry/instrumentation/openai/shared/chat_wrappers.py:370
  • Draft comment:
    Good fix: converting non-dict messages (e.g. Pydantic models) using model_as_dict ensures the prompts are dumped correctly. Consider adding a brief comment or docstring note to clarify that non-dict messages are expected and converted.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% While the comment points out something real - the code does handle non-dict messages - adding a comment about this is not strictly necessary. The code is fairly self-explanatory with the isinstance check. Documentation about implementation details like this can become outdated. The behavior is clear from the code itself. The code handles an important edge case that future maintainers should be aware of. Documentation could help them understand why this conversion is needed. The code is already clear and explicit about what it's doing - checking if it's a dict and converting if not. The conversion method name model_as_dict is also self-documenting. Additional comments would be redundant. The comment should be deleted. The code is sufficiently self-documenting through clear variable names and explicit type checking. Additional documentation would be redundant and could become outdated.

Workflow ID: wflow_DBNpz8Hx32d9QQ2Q

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

@skull8888888
Copy link
Collaborator

hey @nirga, would really appreciate if you can take a look at it.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@nirga
Copy link
Member

nirga commented Jun 9, 2025

Sorry for the delay @dinmukhamedm / @skull8888888 - team was traveling
You can approve PRs yourself now though 😅

@nirga nirga merged commit 8669171 into traceloop:main Jun 9, 2025
9 checks passed
@nirga nirga deleted the openai-pydantic branch June 9, 2025 00:15
DrishyaDas pushed a commit to DrishyaDas/openllmetry that referenced this pull request Jun 16, 2025
Co-authored-by: Nir Gazit <nirga@users.noreply.github.com>
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.

🐛 Bug Report: OpenAI instrumentation fails when sending a response message from a previous call
3 participants