Skip to content

OpenAI instrumentation refactoring#1769

Merged
alexmojaki merged 6 commits intomainfrom
tighten/replace-cast-with-direct-construction
Mar 7, 2026
Merged

OpenAI instrumentation refactoring#1769
alexmojaki merged 6 commits intomainfrom
tighten/replace-cast-with-direct-construction

Conversation

@alexmojaki
Copy link
Copy Markdown
Collaborator

@alexmojaki alexmojaki commented Mar 7, 2026

Summary by cubic

Refactored the OpenAI integration to centralize content and tool-call parsing and build semconv messages directly. Also preserves non-text output parts (e.g., refusal) in Responses outputs.

  • Refactors

    • Added _convert_content_part_or_parts and applied it across chat completions, Responses inputs/outputs, and ChatCompletion messages.
    • Introduced make_tool_call_part to standardize tool-call creation and JSON argument decoding.
    • Replaced casts with direct ChatMessage/OutputMessage construction and simplified role handling.
  • Bug Fixes

    • Preserve non-text content (e.g., refusal) in responses_outputs; updated test asserts the part is present.

Written for commit a637dc1. Summary will update on new commits.

@cloudflare-workers-and-pages
Copy link
Copy Markdown

cloudflare-workers-and-pages bot commented Mar 7, 2026

Deploying logfire-docs with  Cloudflare Pages  Cloudflare Pages

Latest commit: a637dc1
Status: ✅  Deploy successful!
Preview URL: https://d809dab7.logfire-docs.pages.dev
Branch Preview URL: https://tighten-replace-cast-with-di.logfire-docs.pages.dev

View logs

Copy link
Copy Markdown

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

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

No issues found across 2 files

Confidence score: 5/5

  • Automated review surfaced no issues in the provided summaries.
  • No files require special attention.

Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration bot left a comment

Choose a reason for hiding this comment

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

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no potential bugs to report.

View in Devin Review to see 4 additional findings.

Open in Devin Review

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR refactors the OpenAI integration’s message/content parsing to centralize conversion into OTel GenAI semconv “parts”, and updates tests to ensure non-text Responses output parts (e.g., refusals) are preserved.

Changes:

  • Centralized content conversion via _convert_content_part_or_parts and applied it across chat completions + Responses input/output handling.
  • Standardized tool-call part creation and JSON-argument decoding via make_tool_call_part.
  • Updated test expectations to assert non-text output parts are retained in semconv outputs.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
logfire/_internal/integrations/llm_providers/openai.py Refactors content/tool-call conversion to build semconv message parts consistently across endpoints.
tests/otel_integrations/test_openai.py Updates snapshot assertion to verify refusal (non-text) content parts are preserved.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +297 to +304
def _convert_content_part_or_parts(content: object) -> list[MessagePart]:
if not content:
return []

if isinstance(content, list):
return [_convert_content_part(part) for part in cast(list[Any], content)]
else:
return [_convert_content_part(content)]
Copy link

Copilot AI Mar 7, 2026

Choose a reason for hiding this comment

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

_convert_content_part_or_parts() treats any falsy content as “no parts” (via if not content:). This changes prior behavior for chat completions where content == '' (empty string) would still be recorded as a text part; now it will be dropped. To avoid losing information, consider checking specifically for content is None (and optionally content == []) rather than truthiness.

Copilot uses AI. Check for mistakes.
"""Convert a single content part to semconv format."""
if isinstance(part, str): # pragma: no cover
return TextPart(type='text', content=part)
if not isinstance(part, dict): # pragma: no cover
Copy link

Copilot AI Mar 7, 2026

Choose a reason for hiding this comment

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

The # pragma: no cover on the non-dict branch in _convert_content_part is no longer accurate after refactoring: chat completions commonly pass content as a plain string, so this branch will run in practice and should be covered. Consider removing the pragma (or adding a test that exercises this path) so coverage can catch regressions here.

Suggested change
if not isinstance(part, dict): # pragma: no cover
if not isinstance(part, dict):

Copilot uses AI. Check for mistakes.
part = cast('dict[str, Any]', part)
part_type = part.get('type', 'unknown')
if part_type == 'text':
if part_type in ('text', 'output_text'):
Copy link

Copilot AI Mar 7, 2026

Choose a reason for hiding this comment

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

_convert_content_part normalizes OpenAI output_text parts to semconv text, but does not normalize input_text parts (which are used by the Responses API for input content parts). As a result, Responses inputs that use content-part objects may end up with raw {type: 'input_text', ...} dicts in parts instead of semconv {type: 'text', content: ...}. Consider treating input_text the same as output_text here.

Suggested change
if part_type in ('text', 'output_text'):
if part_type in ('text', 'output_text', 'input_text'):

Copilot uses AI. Check for mistakes.
@alexmojaki alexmojaki merged commit 86c6c7b into main Mar 7, 2026
20 checks passed
@alexmojaki alexmojaki deleted the tighten/replace-cast-with-direct-construction branch March 7, 2026 14:53
alexmojaki added a commit that referenced this pull request Mar 11, 2026
…arsing

Apply equivalent changes from OpenAI refactoring PR #1769:
- Add _convert_content_part_or_parts for str/list content handling
- Add make_tool_call_part helper for standardized tool-call creation
- Update _convert_content_part to accept object, handle non-dict inputs
- Replace dict literals with ChatMessage/OutputMessage constructors
- Remove cast('Role', ...) in favor of direct attribute access
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