fix(google-genai): migrate span attrs to gen_ai.input/output.messages (fixes #3515) - #3948
fix(google-genai): migrate span attrs to gen_ai.input/output.messages (fixes #3515)#3948abhyudayareddy wants to merge 16 commits into
Conversation
|
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors Google GenAI span serialization to stop emitting deprecated indexed attributes and instead emit structured message arrays Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/opentelemetry-instrumentation-google-generativeai/tests/test_generate_content.py (1)
49-57: Strengthen schema assertions for migrated message payloads.Current checks only validate
role; they won’t catch regressions inpartsstructure (type/content) that this migration depends on.✅ Suggested test hardening
assert GenAIAttributes.GEN_AI_INPUT_MESSAGES in attrs input_msgs = json.loads(attrs[GenAIAttributes.GEN_AI_INPUT_MESSAGES]) assert len(input_msgs) > 0 assert input_msgs[0]["role"] == "user" + assert isinstance(input_msgs[0].get("parts"), list) and len(input_msgs[0]["parts"]) > 0 + assert input_msgs[0]["parts"][0]["type"] == "text" + assert "content" in input_msgs[0]["parts"][0] assert GenAIAttributes.GEN_AI_OUTPUT_MESSAGES in attrs output_msgs = json.loads(attrs[GenAIAttributes.GEN_AI_OUTPUT_MESSAGES]) assert len(output_msgs) > 0 assert output_msgs[0]["role"] == "assistant" + assert isinstance(output_msgs[0].get("parts"), list) and len(output_msgs[0]["parts"]) > 0 + assert output_msgs[0]["parts"][0]["type"] == "text" + assert "content" in output_msgs[0]["parts"][0]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opentelemetry-instrumentation-google-generativeai/tests/test_generate_content.py` around lines 49 - 57, The tests currently only assert the "role" field for messages stored under GenAIAttributes.GEN_AI_INPUT_MESSAGES and GenAIAttributes.GEN_AI_OUTPUT_MESSAGES; extend these assertions to validate the migrated message payload schema by checking each message's "parts" structure (e.g., ensure input_msgs[0] and output_msgs[0] have a "parts" list, that parts[0] contains "type" and "content" keys, and that "content" is the expected string or non-empty value), so the test fails if the required type/content fields are missing or empty.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/opentelemetry-instrumentation-google-generativeai/opentelemetry/instrumentation/google_generativeai/span_utils.py`:
- Around line 409-433: The code currently hardcodes "finish_reason": "stop" for
every appended message; instead, only include a finish_reason field when the
non-streaming Google response provides one (use
response.candidates[0].finish_reason when present) and omit finish_reason for
streaming paths (where response was a list/str or built from response.text).
Update the branches that handle the original non-streaming response object (the
branches referencing response and response.text and appending into
output_messages) to read finish = getattr(response, "candidates", None) and, if
finish and finish[0].finish_reason is truthy, include "finish_reason":
finish[0].finish_reason in the dict; otherwise do not add the finish_reason key.
Leave the streaming branches that handle list/str (the paths enumerating
response or using response as str) unchanged so they do not include
finish_reason.
---
Nitpick comments:
In
`@packages/opentelemetry-instrumentation-google-generativeai/tests/test_generate_content.py`:
- Around line 49-57: The tests currently only assert the "role" field for
messages stored under GenAIAttributes.GEN_AI_INPUT_MESSAGES and
GenAIAttributes.GEN_AI_OUTPUT_MESSAGES; extend these assertions to validate the
migrated message payload schema by checking each message's "parts" structure
(e.g., ensure input_msgs[0] and output_msgs[0] have a "parts" list, that
parts[0] contains "type" and "content" keys, and that "content" is the expected
string or non-empty value), so the test fails if the required type/content
fields are missing or empty.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1215e26d-911c-4d0f-b60a-791811eb5b38
📒 Files selected for processing (2)
packages/opentelemetry-instrumentation-google-generativeai/opentelemetry/instrumentation/google_generativeai/span_utils.pypackages/opentelemetry-instrumentation-google-generativeai/tests/test_generate_content.py
…n unknown Updated output message construction to include finish_reason when available and adjusted handling for both non-streaming and streaming responses.
… in messages Add assertions to validate structure of input and output messages in tests.
|
Addressed both CodeRabbit review items in the latest two commits:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/opentelemetry-instrumentation-google-generativeai/tests/test_generate_content.py (1)
49-63: Assert that the deprecated indexed attributes are gone.These checks only prove the new JSON blobs exist. They would still pass if
gen_ai.prompt.*/gen_ai.completion.*were emitted alongside them, which leaves the core migration behavior unguarded.🧪 Minimal regression guard
assert GenAIAttributes.GEN_AI_INPUT_MESSAGES in attrs + assert not any(key.startswith("gen_ai.prompt.") for key in attrs.keys()) input_msgs = json.loads(attrs[GenAIAttributes.GEN_AI_INPUT_MESSAGES]) assert GenAIAttributes.GEN_AI_OUTPUT_MESSAGES in attrs + assert not any(key.startswith("gen_ai.completion.") for key in attrs.keys()) output_msgs = json.loads(attrs[GenAIAttributes.GEN_AI_OUTPUT_MESSAGES])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opentelemetry-instrumentation-google-generativeai/tests/test_generate_content.py` around lines 49 - 63, Add assertions to ensure deprecated indexed attributes are not present: after loading attrs and verifying GenAIAttributes.GEN_AI_INPUT_MESSAGES and GenAIAttributes.GEN_AI_OUTPUT_MESSAGES, assert that no keys in attrs start with "gen_ai.prompt." or "gen_ai.completion." (e.g., use any(k.startswith("gen_ai.prompt.") or k.startswith("gen_ai.completion.") for k in attrs) and assert that this is False). Reference the attrs variable and the existing GenAIAttributes.GEN_AI_INPUT_MESSAGES / GenAIAttributes.GEN_AI_OUTPUT_MESSAGES checks to locate where to add these assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/opentelemetry-instrumentation-google-generativeai/opentelemetry/instrumentation/google_generativeai/span_utils.py`:
- Around line 202-218: The current handling of kwargs["contents"] in this block
(and the similar block at 253-303) only handles str vs list and treats every
list item as a separate turn, which misclassifies single Content objects and
iterables of Part objects; change the normalization so that you first detect
whether contents is a Content-like object (has attributes role and parts) and
treat it as one turn, detect whether contents is a single Part (has attributes
type/content) and wrap it into a single turn with default role "user", and
detect whether contents is an iterable: if the iterable's items are Content
objects treat each as a separate turn, but if the iterable's items are Part
objects aggregate them into one turn. Use the existing _process_content_item to
process parts, preserve role via getattr(content_item, "role", "user"), and
append consistent dicts to input_messages with keys "role" and "parts"; apply
the same normalization logic in both the block around input_messages
construction and the similar code at lines 253-303.
---
Nitpick comments:
In
`@packages/opentelemetry-instrumentation-google-generativeai/tests/test_generate_content.py`:
- Around line 49-63: Add assertions to ensure deprecated indexed attributes are
not present: after loading attrs and verifying
GenAIAttributes.GEN_AI_INPUT_MESSAGES and
GenAIAttributes.GEN_AI_OUTPUT_MESSAGES, assert that no keys in attrs start with
"gen_ai.prompt." or "gen_ai.completion." (e.g., use
any(k.startswith("gen_ai.prompt.") or k.startswith("gen_ai.completion.") for k
in attrs) and assert that this is False). Reference the attrs variable and the
existing GenAIAttributes.GEN_AI_INPUT_MESSAGES /
GenAIAttributes.GEN_AI_OUTPUT_MESSAGES checks to locate where to add these
assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 970a054d-55c6-4934-90f0-ee656e5f647a
📒 Files selected for processing (2)
packages/opentelemetry-instrumentation-google-generativeai/opentelemetry/instrumentation/google_generativeai/span_utils.pypackages/opentelemetry-instrumentation-google-generativeai/tests/test_generate_content.py
…bsent
Adds negative assertions to test_client_spans to verify deprecated gen_ai.prompt.{N} and gen_ai.completion.{N} attributes are absent after migrating to gen_ai.input.messages / gen_ai.output.messages. Addresses CodeRabbit review feedback (round 2).Adds negative assertions to test_client_spans to guard against deprecated gen_ai.prompt.{N} and gen_ai.completion.{N} attributes leaking back in after the migration to gen_ai.input.messages / gen_ai.output.messages. Addresses CodeRabbit review feedback.Added assertions to verify deprecated attributes are not emitted.
…tent] from list[Part] in contents normalization Addresses CodeRabbit major review comment on contents normalization: 1. Adds elif hasattr(contents, "parts") branch to handle a single Content object passed directly (not wrapped in a list). 2. In the elif isinstance(contents, list) branch, sniffs contents[0] to distinguish multi-turn conversations (list[Content], each with .parts) from single-turn inputs (list[Part] or list[str]). Multi-turn: each Content becomes a separate message. Single-turn: all Parts are collected into one user message. Applied symmetrically to both the async set_input_attributes and sync set_input_attributes_sync.
|
Addressed all CodeRabbit round-2 feedback (commits 08f2f74 and 56f5e51): test_generate_content.py (commit 08f2f74): Added negative assertions after the existing assert not any(key.startswith("gen_ai.prompt.") for key in attrs.keys())
assert not any(key.startswith("gen_ai.completion.") for key in attrs.keys())span_utils.py (commit 56f5e51): Fixed
The fix applies to both the async |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/opentelemetry-instrumentation-google-generativeai/opentelemetry/instrumentation/google_generativeai/span_utils.py (1)
123-138:⚠️ Potential issue | 🟠 MajorNormalize
Partinputs before serializinggen_ai.input.messages.The SDK accepts
contentsas a singlePart, groups non-function-call parts into a user turn, groups function-call parts into a model turn, and allows mixedContentUnionlists to alternate between those grouped turns. This code still only special-casesstr/Content/list, forces non-Contentlists to"user", and falls back tostr(part)for direct Part-like values, so valid tool-calling or multimodal inputs can be dropped or mis-serialized. (googleapis.github.io)Also applies to: 202-239, 274-340
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opentelemetry-instrumentation-google-generativeai/opentelemetry/instrumentation/google_generativeai/span_utils.py` around lines 123 - 138, This block mishandles Part-like inputs by only special-casing str/list and falling back to str(content_item), which can drop or mis-serialize tool-calling or multimodal Parts; update the processing around content_item to first normalize any Part/Part-like or ContentUnion instances into canonical message parts (preserving function-call vs non-function-call semantics and list alternation rules used by gen_ai.input.messages) before serializing: detect Part objects (instead of treating them as generic objects), route image Parts through _is_image_part/_process_image_part as before, group non-function-call Parts into user turns and function-call Parts into model turns, and handle single Part or single-list inputs identically to lists so you never fall back to processed_content.append({"type":"text","content": str(content_item)}); apply the same normalization logic to the other processing sites referenced in the review (the blocks around lines handling content lists) to ensure consistent serialization.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/opentelemetry-instrumentation-google-generativeai/opentelemetry/instrumentation/google_generativeai/span_utils.py`:
- Around line 445-459: The code currently reads a single finish_reason from
_candidates[0] and reuses it for every message in the multi-message branch;
change logic so each message uses its own candidate's finish_reason: iterate
over _candidates (or ensure the loop enumerates response/candidates) and for
each item use that item.finish_reason (handling hasattr(item.finish_reason,
"name") as done now) to set msg["finish_reason"] before appending to
output_messages; update references to response.text/list branching and the
variables _candidates, fr, and _finish_reason accordingly so each candidate's
finish_reason is applied to its own msg.
- Around line 443-487: The handler only serializes response.text; update the
logic in span_utils.py (the block building output_messages used before calling
_set_span_attribute with GenAIAttributes.GEN_AI_OUTPUT_MESSAGES) to iterate
response.parts (and response.function_calls when present) in addition to
response.text so all part types (function_call, inline_data, file_data,
executable_code, code_execution_result, etc.) are captured into the parts list
structure, preserving existing fields like "type", "content" (or structured
payload for function calls), and adding finish_reason when available from
response.candidates; ensure GenerateContentResponse.parts entries are normalized
into the same message format currently used for text so downstream consumers
receive complete gen_ai.output.messages.
---
Duplicate comments:
In
`@packages/opentelemetry-instrumentation-google-generativeai/opentelemetry/instrumentation/google_generativeai/span_utils.py`:
- Around line 123-138: This block mishandles Part-like inputs by only
special-casing str/list and falling back to str(content_item), which can drop or
mis-serialize tool-calling or multimodal Parts; update the processing around
content_item to first normalize any Part/Part-like or ContentUnion instances
into canonical message parts (preserving function-call vs non-function-call
semantics and list alternation rules used by gen_ai.input.messages) before
serializing: detect Part objects (instead of treating them as generic objects),
route image Parts through _is_image_part/_process_image_part as before, group
non-function-call Parts into user turns and function-call Parts into model
turns, and handle single Part or single-list inputs identically to lists so you
never fall back to processed_content.append({"type":"text","content":
str(content_item)}); apply the same normalization logic to the other processing
sites referenced in the review (the blocks around lines handling content lists)
to ensure consistent serialization.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 31c464ec-5fbf-4173-853e-e80ee36d116d
📒 Files selected for processing (2)
packages/opentelemetry-instrumentation-google-generativeai/opentelemetry/instrumentation/google_generativeai/span_utils.pypackages/opentelemetry-instrumentation-google-generativeai/tests/test_generate_content.py
✅ Files skipped from review due to trivial changes (1)
- packages/opentelemetry-instrumentation-google-generativeai/tests/test_generate_content.py
|
Addressed CodeRabbit round-3 feedback in commit 6dc73dc: Item 1 (Major — Handle all response.parts types): Added a Item 2 (Minor — Per-candidate finish_reason): Removed the single shared |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/opentelemetry-instrumentation-google-generativeai/opentelemetry/instrumentation/google_generativeai/span_utils.py (1)
123-139:⚠️ Potential issue | 🟠 MajorStructured inputs are still dropped or flattened outside the main
contentspath.
kwargs["contents"]still only matchesstr, objects with.parts, orlist, so a single part-like object falls through without emitting anything. The list and positional-argsbranches also serialize.text/.partsobjects viastr(...)instead of the existing content/part serializers, which loses real text, roles, and turn boundaries ingen_ai.input.messages. Please funnel both kwargs and positional args through one shared normalizer.Also applies to: 158-189, 202-248, 282-378
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opentelemetry-instrumentation-google-generativeai/opentelemetry/instrumentation/google_generativeai/span_utils.py` around lines 123 - 139, The code currently flattens or drops structured inputs by only handling str, .parts lists, or list types and by using str(...) instead of the proper serializers; update span_utils.py to funnel both kwargs["contents"] and all positional args through a single shared normalizer function (create a helper like _normalize_contents used by the blocks around processed_content) that: detects single part-like objects (use _is_image_part) and other content types, calls the existing serializers for content/part instead of str(), and routes image parts to _process_image_part with span.context.trace_id/span_id; replace the duplicated handling in the branches referenced (including the shown block and the ranges 158-189, 202-248, 282-378) to call this shared normalizer so gen_ai.input.messages preserves text, roles, and turn boundaries.
🧹 Nitpick comments (1)
packages/opentelemetry-instrumentation-google-generativeai/opentelemetry/instrumentation/google_generativeai/span_utils.py (1)
441-447: Align tool-call parts with the shared message schema.This serializer emits
{"type": "function_call"}, butpackages/opentelemetry-instrumentation-anthropic/opentelemetry/instrumentation/anthropic/span_utils.py:57-94already uses{"type": "tool_call"}for the same cross-provider part shape. Using a different type here makes downstream parsers provider-specific again.♻️ Suggested change
return { - "type": "function_call", + "type": "tool_call", "name": fc.name, "arguments": dict(fc.args) if hasattr(fc, "args") else {}, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opentelemetry-instrumentation-google-generativeai/opentelemetry/instrumentation/google_generativeai/span_utils.py` around lines 441 - 447, The serializer currently returns a dict with "type": "function_call" for parts where part.function_call is present; change the emitted type to "tool_call" and keep the same shape (keys "name" and "arguments") so the output matches the cross-provider schema used by the other implementation (see the handling of part.function_call -> fc.name and fc.args in span_utils). Update the return inside the if hasattr(part, "function_call") block to emit "type": "tool_call" and preserve converting fc.args to a dict when present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/opentelemetry-instrumentation-google-generativeai/opentelemetry/instrumentation/google_generativeai/span_utils.py`:
- Around line 473-500: The candidate-level fallback that assigns response.text
when parts is empty should be moved out of the per-candidate loop so one
candidate cannot inherit another candidate’s text; instead, only if no candidate
produced any parts at all use the response-wide text to create a single
assistant message. Also fix the second fallback branch to check
isinstance(response.text, list) and iterate over response.text (not response)
when building messages. Update logic around parts, candidates iteration and the
output_messages append (references: parts, candidates loop, response.text,
output_messages, _finish_reason) accordingly.
---
Duplicate comments:
In
`@packages/opentelemetry-instrumentation-google-generativeai/opentelemetry/instrumentation/google_generativeai/span_utils.py`:
- Around line 123-139: The code currently flattens or drops structured inputs by
only handling str, .parts lists, or list types and by using str(...) instead of
the proper serializers; update span_utils.py to funnel both kwargs["contents"]
and all positional args through a single shared normalizer function (create a
helper like _normalize_contents used by the blocks around processed_content)
that: detects single part-like objects (use _is_image_part) and other content
types, calls the existing serializers for content/part instead of str(), and
routes image parts to _process_image_part with span.context.trace_id/span_id;
replace the duplicated handling in the branches referenced (including the shown
block and the ranges 158-189, 202-248, 282-378) to call this shared normalizer
so gen_ai.input.messages preserves text, roles, and turn boundaries.
---
Nitpick comments:
In
`@packages/opentelemetry-instrumentation-google-generativeai/opentelemetry/instrumentation/google_generativeai/span_utils.py`:
- Around line 441-447: The serializer currently returns a dict with "type":
"function_call" for parts where part.function_call is present; change the
emitted type to "tool_call" and keep the same shape (keys "name" and
"arguments") so the output matches the cross-provider schema used by the other
implementation (see the handling of part.function_call -> fc.name and fc.args in
span_utils). Update the return inside the if hasattr(part, "function_call")
block to emit "type": "tool_call" and preserve converting fc.args to a dict when
present.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8f62549c-6d10-4524-98d5-1d4a7bc278d8
📒 Files selected for processing (1)
packages/opentelemetry-instrumentation-google-generativeai/opentelemetry/instrumentation/google_generativeai/span_utils.py
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/opentelemetry-instrumentation-google-generativeai/tests/test_generate_content.py (1)
167-168:⚠️ Potential issue | 🔴 CriticalCritical:
MagicMockused but not imported.The test uses
MagicMock()on line 167, but there's no import for it. This will causeNameError: name 'MagicMock' is not definedwhen running this test.🐛 Proposed fix - add import
import json import pytest +from unittest.mock import MagicMock from opentelemetry.trace import StatusCode, SpanKind🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opentelemetry-instrumentation-google-generativeai/tests/test_generate_content.py` around lines 167 - 168, The test uses MagicMock (see span = MagicMock() in tests/test_generate_content.py) but MagicMock is not imported; add an import for MagicMock (e.g., from unittest.mock import MagicMock) at the top of the test module so the span = MagicMock() and span.is_recording.return_value = True lines can run without NameError.packages/opentelemetry-instrumentation-google-generativeai/opentelemetry/instrumentation/google_generativeai/span_utils.py (2)
720-735:⚠️ Potential issue | 🔴 CriticalCritical: Variable
umused but never assigned, causingNameError.The code checks
hasattr(response, "usage_metadata")but never assignsum. Lines 724, 729, and 734 referenceum.total_token_count,um.candidates_token_count, andum.prompt_token_countwhich will fail at runtime.🐛 Proposed fix
_set_span_attribute(span, GenAIAttributes.GEN_AI_RESPONSE_MODEL, llm_model) if hasattr(response, "usage_metadata"): + um = response.usage_metadata _set_span_attribute( span, SpanAttributes.GEN_AI_USAGE_TOTAL_TOKENS, um.total_token_count, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opentelemetry-instrumentation-google-generativeai/opentelemetry/instrumentation/google_generativeai/span_utils.py` around lines 720 - 735, The code references an unassigned variable um when reading usage fields; fix by reading the response.usage_metadata into a local variable (e.g., um = response.usage_metadata) immediately after the hasattr(response, "usage_metadata") check, guard for None, then call _set_span_attribute using um.total_token_count, um.candidates_token_count, and um.prompt_token_count; update the block around response, um, and _set_span_attribute (and leave SpanAttributes.GEN_AI_USAGE_TOTAL_TOKENS / GenAIAttributes.GEN_AI_USAGE_OUTPUT_TOKENS / GenAIAttributes.GEN_AI_USAGE_INPUT_TOKENS unchanged) so it no longer raises NameError.
740-741:⚠️ Potential issue | 🔴 CriticalAdd missing imports for
_GCP_GEN_AIand_GEN_CONTENTconstants.These constants are used at lines 740-741 in
span_utils.pybut are not imported. They are defined in__init__.pyand should be imported from there to avoidNameErrorat runtime.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opentelemetry-instrumentation-google-generativeai/opentelemetry/instrumentation/google_generativeai/span_utils.py` around lines 740 - 741, Import the missing constants _GCP_GEN_AI and _GEN_CONTENT into span_utils.py so they are available where used in the attributes dict (GenAIAttributes.GEN_AI_PROVIDER_NAME: _GCP_GEN_AI, GenAIAttributes.GEN_AI_OPERATION_NAME: _GEN_CONTENT); add them to the module-level imports by importing _GCP_GEN_AI and _GEN_CONTENT from the package root (where they are defined in __init__.py) alongside the other constants to prevent NameError at runtime.
♻️ Duplicate comments (2)
packages/opentelemetry-instrumentation-google-generativeai/opentelemetry/instrumentation/google_generativeai/span_utils.py (2)
663-670:⚠️ Potential issue | 🟡 MinorResponse-wide
response.textfallback inside per-candidate loop may cause incorrect attribution.The fallback to
response.text(lines 665-668) is inside the per-candidate loop. For multi-candidate responses, if any candidate has emptycontent.parts, it will use the response-wideresponse.textwhich typically reflects only the first candidate's text. Consider moving this fallback outside the loop or limiting it to single-candidate responses.🛡️ Suggested guard
if not parts: - # Fallback: try response.text for simple single-part responses - try: - text = response.text - if text: - parts = [{"type": "text", "content": text}] - except Exception: - pass + # Fallback only for single-candidate responses + if len(_candidates) == 1: + try: + text = response.text + if text: + parts = [{"type": "text", "content": text}] + except Exception: + pass🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opentelemetry-instrumentation-google-generativeai/opentelemetry/instrumentation/google_generativeai/span_utils.py` around lines 663 - 670, The per-candidate fallback that sets parts from response.text should not run inside the per-candidate loop because response.text typically reflects the first candidate and will misattribute text to other candidates; update the logic in span_utils.py so that you only use response.text when the response has a single candidate (e.g., check len(response.candidates) == 1) or move the response.text fallback out of the candidate-processing loop and apply it only when no candidates produced any parts; adjust the branch that currently checks "if not parts:" inside the candidate loop to instead use a guarded check (single-candidate) or perform a post-loop fallback using response.text.
676-691:⚠️ Potential issue | 🟠 MajorFallback branch iterates
responseinstead ofresponse.text.When
response.textis a list (line 679), the code iterates overresponse(line 680) rather thanresponse.text. Additionally, accessingitem.textassumes items are objects with atextattribute, which may not be true ifresponse.textcontains strings.🐛 Proposed fix
else: # No candidates field: fall back to response.text try: if isinstance(response.text, list): - for item in response: + for item in response.text: output_messages.append({ "role": "assistant", - "parts": [{"type": "text", "content": item.text}], + "parts": [{"type": "text", "content": getattr(item, "text", str(item))}], }) elif isinstance(response.text, str):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opentelemetry-instrumentation-google-generativeai/opentelemetry/instrumentation/google_generativeai/span_utils.py` around lines 676 - 691, In the else fallback in span_utils.py (the block handling response.text when no candidates field exists), fix the loop to iterate over response.text (not response) and handle list item types safely: for each item in response.text, if the item is a string use it directly as the "content", otherwise if it has a .text attribute use that, and skip or stringify other types; update the branch inside the function that builds output_messages so it checks isinstance(response.text, list) and accesses items correctly instead of using item.text blindly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/opentelemetry-instrumentation-google-generativeai/opentelemetry/instrumentation/google_generativeai/span_utils.py`:
- Around line 238-261: The function _process_content_item uses an undefined
accumulator and helper: replace the erroneous parts_acc usage with the local
processed_content (i.e., call processed_content.extend(...)) and ensure the
helper _parts_from_genai_part_async is provided — either implement/import a
coroutine named _parts_from_genai_part_async(part, span, part_index) that
returns a list of processed parts or rename the call to the correct existing
async helper if it already exists; update _process_content_item to extend
processed_content with the awaited result and return processed_content.
In
`@packages/opentelemetry-instrumentation-google-generativeai/tests/test_generate_content.py`:
- Around line 76-78: The test asserts SpanAttributes.LLM_USAGE_TOTAL_TOKENS
while the implementation sets SpanAttributes.GEN_AI_USAGE_TOTAL_TOKENS, causing
a mismatch; fix by making them consistent: either update the test to assert
SpanAttributes.GEN_AI_USAGE_TOTAL_TOKENS (and related GEN_AI_* attributes) or
update the implementation (the code that assigns token attributes in
span_utils.py) to also set the LLM_USAGE_* attributes (or set both keys) so both
constants are present; ensure you update or add the corresponding assertions for
input/output/total tokens (SpanAttributes.LLM_USAGE_TOTAL_TOKENS,
GenAIAttributes.GEN_AI_USAGE_INPUT_TOKENS,
GenAIAttributes.GEN_AI_USAGE_OUTPUT_TOKENS) consistently.
---
Outside diff comments:
In
`@packages/opentelemetry-instrumentation-google-generativeai/opentelemetry/instrumentation/google_generativeai/span_utils.py`:
- Around line 720-735: The code references an unassigned variable um when
reading usage fields; fix by reading the response.usage_metadata into a local
variable (e.g., um = response.usage_metadata) immediately after the
hasattr(response, "usage_metadata") check, guard for None, then call
_set_span_attribute using um.total_token_count, um.candidates_token_count, and
um.prompt_token_count; update the block around response, um, and
_set_span_attribute (and leave SpanAttributes.GEN_AI_USAGE_TOTAL_TOKENS /
GenAIAttributes.GEN_AI_USAGE_OUTPUT_TOKENS /
GenAIAttributes.GEN_AI_USAGE_INPUT_TOKENS unchanged) so it no longer raises
NameError.
- Around line 740-741: Import the missing constants _GCP_GEN_AI and _GEN_CONTENT
into span_utils.py so they are available where used in the attributes dict
(GenAIAttributes.GEN_AI_PROVIDER_NAME: _GCP_GEN_AI,
GenAIAttributes.GEN_AI_OPERATION_NAME: _GEN_CONTENT); add them to the
module-level imports by importing _GCP_GEN_AI and _GEN_CONTENT from the package
root (where they are defined in __init__.py) alongside the other constants to
prevent NameError at runtime.
In
`@packages/opentelemetry-instrumentation-google-generativeai/tests/test_generate_content.py`:
- Around line 167-168: The test uses MagicMock (see span = MagicMock() in
tests/test_generate_content.py) but MagicMock is not imported; add an import for
MagicMock (e.g., from unittest.mock import MagicMock) at the top of the test
module so the span = MagicMock() and span.is_recording.return_value = True lines
can run without NameError.
---
Duplicate comments:
In
`@packages/opentelemetry-instrumentation-google-generativeai/opentelemetry/instrumentation/google_generativeai/span_utils.py`:
- Around line 663-670: The per-candidate fallback that sets parts from
response.text should not run inside the per-candidate loop because response.text
typically reflects the first candidate and will misattribute text to other
candidates; update the logic in span_utils.py so that you only use response.text
when the response has a single candidate (e.g., check len(response.candidates)
== 1) or move the response.text fallback out of the candidate-processing loop
and apply it only when no candidates produced any parts; adjust the branch that
currently checks "if not parts:" inside the candidate loop to instead use a
guarded check (single-candidate) or perform a post-loop fallback using
response.text.
- Around line 676-691: In the else fallback in span_utils.py (the block handling
response.text when no candidates field exists), fix the loop to iterate over
response.text (not response) and handle list item types safely: for each item in
response.text, if the item is a string use it directly as the "content",
otherwise if it has a .text attribute use that, and skip or stringify other
types; update the branch inside the function that builds output_messages so it
checks isinstance(response.text, list) and accesses items correctly instead of
using item.text blindly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: eadafac1-a58b-43fc-9f8c-5c6cfeb6ada0
📒 Files selected for processing (2)
packages/opentelemetry-instrumentation-google-generativeai/opentelemetry/instrumentation/google_generativeai/span_utils.pypackages/opentelemetry-instrumentation-google-generativeai/tests/test_generate_content.py
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
|
👋 Hi @nirga, @galkleinman, @dinmukhamedm — could you take a look at this PR when you get a chance? It migrates the Google GenAI instrumentation span attributes to the stable |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/opentelemetry-instrumentation-google-generativeai/tests/test_generate_content.py (1)
1-12:⚠️ Potential issue | 🔴 CriticalMissing
MagicMockimport causesNameErrorin test.Line 167 uses
MagicMock()but the import was removed. This will cause aNameErrorwhen runningtest_set_model_request_attributes_reads_system_instruction_from_config.🐛 Proposed fix
import json import pytest +from unittest.mock import MagicMock from opentelemetry.trace import StatusCode, SpanKind from opentelemetry.semconv_ai import ( SpanAttributes,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opentelemetry-instrumentation-google-generativeai/tests/test_generate_content.py` around lines 1 - 12, The test fails because MagicMock is used but not imported; add "from unittest.mock import MagicMock" to the imports at the top of the test module so tests like test_set_model_request_attributes_reads_system_instruction_from_config can instantiate MagicMock() without a NameError.packages/opentelemetry-instrumentation-google-generativeai/opentelemetry/instrumentation/google_generativeai/span_utils.py (1)
720-735:⚠️ Potential issue | 🔴 CriticalCritical:
umis referenced but never assigned, causingNameErrorat runtime.Lines 724, 729, 734, and 738 use
um.total_token_count,um.candidates_token_count, andum.prompt_token_count, but the variableumis never assigned fromresponse.usage_metadata.🐛 Proposed fix
_set_span_attribute(span, GenAIAttributes.GEN_AI_RESPONSE_MODEL, llm_model) if hasattr(response, "usage_metadata"): + um = response.usage_metadata _set_span_attribute( span, SpanAttributes.GEN_AI_USAGE_TOTAL_TOKENS, um.total_token_count, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opentelemetry-instrumentation-google-generativeai/opentelemetry/instrumentation/google_generativeai/span_utils.py` around lines 720 - 735, The code block in span_utils.py references um.total_token_count / um.candidates_token_count / um.prompt_token_count but never assigns um from response.usage_metadata; set um = response.usage_metadata (after confirming hasattr(response, "usage_metadata") and that it is not None) before calling _set_span_attribute, and keep the existing guards around _set_span_attribute for SpanAttributes.GEN_AI_USAGE_TOTAL_TOKENS and the GenAIAttributes to avoid NameError and handle missing usage metadata.
♻️ Duplicate comments (1)
packages/opentelemetry-instrumentation-google-generativeai/opentelemetry/instrumentation/google_generativeai/span_utils.py (1)
238-261:⚠️ Potential issue | 🔴 CriticalCritical: Undefined variable
parts_accand function_parts_from_genai_part_asynccause runtimeNameError.Line 244 references
parts_accbut the function initializesprocessed_content. Additionally,_parts_from_genai_part_asyncis never defined—the correct helper is_process_content_part(lines 264-277).🐛 Proposed fix
async def _process_content_item(content_item, span): """Process a single content item, handling different types (Content objects, strings, Parts)""" processed_content = [] if hasattr(content_item, "parts"): # Content with parts (Google GenAI Content object) for part_index, part in enumerate(content_item.parts): - parts_acc.extend(await _parts_from_genai_part_async(part, span, part_index)) + processed_part = await _process_content_part(part, span, part_index) + if processed_part: + processed_content.append(processed_part) elif isinstance(content_item, str):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opentelemetry-instrumentation-google-generativeai/opentelemetry/instrumentation/google_generativeai/span_utils.py` around lines 238 - 261, The function _process_content_item currently references an undefined parts_acc and calls a non-existent helper _parts_from_genai_part_async; update the loop that handles content_item.parts to call the existing _process_content_part helper, await it, and extend the local processed_content list with its returned list (e.g. for part_index, part in enumerate(content_item.parts): processed_content.extend(await _process_content_part(part, span, part_index))). Remove references to parts_acc and ensure the signature/arguments match _process_content_part (part, span, part_index) so the function returns a combined processed_content list for all branches.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/opentelemetry-instrumentation-google-generativeai/opentelemetry/instrumentation/google_generativeai/span_utils.py`:
- Around line 676-691: The fallback branch that handles non-candidates responses
checks isinstance(response.text, list) but incorrectly iterates "for item in
response" instead of the list; update the loop in the span_utils fallback to
iterate over response.text (e.g., "for item in response.text") and then use item
or item.text consistently when building output_messages so output_messages is
populated from the actual text list rather than the response object.
- Around line 747-754: Import the missing constants and use them instead of
hardcoded strings: add an import for _GCP_GEN_AI (and _GEN_CONTENT if used) from
opentelemetry.instrumentation.google_generativeai at the top of span_utils.py,
then replace the hardcoded "Google" and any literal token-type strings in the
token_histogram.record call (the call in span_utils.py that references
token_histogram.record, GenAIAttributes.GEN_AI_PROVIDER_NAME and
GenAIAttributes.GEN_AI_TOKEN_TYPE) with the appropriate constants (_GCP_GEN_AI
and _GEN_CONTENT) so the names exist at runtime and remain consistent.
---
Outside diff comments:
In
`@packages/opentelemetry-instrumentation-google-generativeai/opentelemetry/instrumentation/google_generativeai/span_utils.py`:
- Around line 720-735: The code block in span_utils.py references
um.total_token_count / um.candidates_token_count / um.prompt_token_count but
never assigns um from response.usage_metadata; set um = response.usage_metadata
(after confirming hasattr(response, "usage_metadata") and that it is not None)
before calling _set_span_attribute, and keep the existing guards around
_set_span_attribute for SpanAttributes.GEN_AI_USAGE_TOTAL_TOKENS and the
GenAIAttributes to avoid NameError and handle missing usage metadata.
In
`@packages/opentelemetry-instrumentation-google-generativeai/tests/test_generate_content.py`:
- Around line 1-12: The test fails because MagicMock is used but not imported;
add "from unittest.mock import MagicMock" to the imports at the top of the test
module so tests like
test_set_model_request_attributes_reads_system_instruction_from_config can
instantiate MagicMock() without a NameError.
---
Duplicate comments:
In
`@packages/opentelemetry-instrumentation-google-generativeai/opentelemetry/instrumentation/google_generativeai/span_utils.py`:
- Around line 238-261: The function _process_content_item currently references
an undefined parts_acc and calls a non-existent helper
_parts_from_genai_part_async; update the loop that handles content_item.parts to
call the existing _process_content_part helper, await it, and extend the local
processed_content list with its returned list (e.g. for part_index, part in
enumerate(content_item.parts): processed_content.extend(await
_process_content_part(part, span, part_index))). Remove references to parts_acc
and ensure the signature/arguments match _process_content_part (part, span,
part_index) so the function returns a combined processed_content list for all
branches.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 976d58b1-1da2-4744-97de-ca2deedb13be
📒 Files selected for processing (2)
packages/opentelemetry-instrumentation-google-generativeai/opentelemetry/instrumentation/google_generativeai/span_utils.pypackages/opentelemetry-instrumentation-google-generativeai/tests/test_generate_content.py
… finish_reason values
|
Fixed the Major issue flagged by CodeRabbit. The The fix routes through the existing Commit: fix(google-genai): use _map_gemini_finish_reason() for OTel-compliant finish_reason values |
There was a problem hiding this comment.
Actionable comments posted: 1
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-google-generativeai/opentelemetry/instrumentation/google_generativeai/span_utils.py (1)
592-601:⚠️ Potential issue | 🔴 CriticalCritical: Undefined function
_parts_from_genai_part_syncwill cause runtime error.Line 599 calls
_parts_from_genai_part_syncwhich was removed during this refactoring but the call site wasn't updated. This will raiseNameErrorwhen processing system instructions that contain parts.Consider implementing inline part processing similar to the sync input handling:
def _system_instruction_to_parts(si, span): """OTel: flat array of parts for gen_ai.system_instructions.""" if isinstance(si, str): return [{"type": "text", "content": si}] if hasattr(si, "parts") and si.parts: out = [] for idx, p in enumerate(si.parts): - out.extend(_parts_from_genai_part_sync(p, span, idx)) + if hasattr(p, "text") and p.text: + out.append({"type": "text", "content": p.text}) + elif _is_image_part(p): + img = _process_image_part_sync(p, span.context.trace_id, span.context.span_id, idx) + if img: + out.append(img) + else: + out.append({"type": "text", "content": str(p)}) return out return [{"type": "text", "content": str(si)}]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opentelemetry-instrumentation-google-generativeai/opentelemetry/instrumentation/google_generativeai/span_utils.py` around lines 592 - 601, _system_instruction_to_parts currently calls a removed helper `_parts_from_genai_part_sync`, causing a NameError; replace that call with the existing part-processing logic used elsewhere by either calling the correct helper (e.g., `_parts_from_genai_part`) or inlining the same logic used for sync input handling: iterate si.parts, enumerate to get idx, and for each part p call the correct part-to-span conversion routine (passing span and idx) to extend out; ensure you preserve the returned structure (list of dicts) and the fallback string conversion path.
♻️ Duplicate comments (3)
packages/opentelemetry-instrumentation-google-generativeai/opentelemetry/instrumentation/google_generativeai/span_utils.py (3)
238-261:⚠️ Potential issue | 🔴 CriticalCritical: Undefined variable
parts_accand function_parts_from_genai_part_asyncwill cause runtime errors.Line 244 references
parts_accbut the function initializesprocessed_contenton line 240. Additionally,_parts_from_genai_part_asyncis never defined in this module. This will raiseNameErrorwhen processing Content objects with parts.The fix should use the existing
_process_content_parthelper:async def _process_content_item(content_item, span): """Process a single content item, handling different types (Content objects, strings, Parts)""" processed_content = [] if hasattr(content_item, "parts"): # Content with parts (Google GenAI Content object) for part_index, part in enumerate(content_item.parts): - parts_acc.extend(await _parts_from_genai_part_async(part, span, part_index)) + processed_part = await _process_content_part(part, span, part_index) + if processed_part: + processed_content.append(processed_part) elif isinstance(content_item, str):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opentelemetry-instrumentation-google-generativeai/opentelemetry/instrumentation/google_generativeai/span_utils.py` around lines 238 - 261, In _process_content_item the code wrongly references an undefined variable parts_acc and calls a non-existent helper _parts_from_genai_part_async; replace that branch to iterate content_item.parts and call the existing _process_content_part for each part (passing span, part_index and/or trace/span ids as needed) and extend processed_content with its results, keeping the existing image handling via _process_image_part and returning processed_content; update references to span.context.trace_id and span.context.span_id only where _process_content_part requires them.
720-754:⚠️ Potential issue | 🔴 CriticalCritical: Undefined variable
umand missing constant imports will cause runtime errors.Multiple undefined names in this function:
Lines 723-734: Uses
um.total_token_count,um.candidates_token_count,um.prompt_token_countbutumis never assigned. Should beresponse.usage_metadataor assignum = response.usage_metadatafirst.Lines 740-741: Uses
_GCP_GEN_AIand_GEN_CONTENTconstants that are not imported in this module.Line 750: Uses hardcoded
"Google"while line 740 expects the constant_GCP_GEN_AI(inconsistent).def set_model_response_attributes( span, response, llm_model, token_histogram, stream_finish_reasons=None ): if not span.is_recording(): return _set_span_attribute(span, GenAIAttributes.GEN_AI_RESPONSE_MODEL, llm_model) if hasattr(response, "usage_metadata"): + um = response.usage_metadata _set_span_attribute( span, SpanAttributes.GEN_AI_USAGE_TOTAL_TOKENS, um.total_token_count, )Additionally, add the missing imports at the top of the file or use inline values consistently:
from opentelemetry.instrumentation.google_generativeai import _GCP_GEN_AI, _GEN_CONTENTOr replace with string literals consistently throughout (e.g.,
"Google"and"generate_content").
663-690:⚠️ Potential issue | 🟠 MajorMove the fallback out of the candidate loop and fix the iteration target.
Two issues in the fallback handling:
Lines 663-670: The
response.textfallback is inside the per-candidate loop. If any candidate has emptycontent.parts, it will use response-wide text, potentially causing one candidate to inherit another's content.Lines 679-684: The code checks
isinstance(response.text, list)but iteratesfor item in response:instead offor item in response.text:.if parts: msg = {"role": "assistant", "parts": parts} if _finish_reason: msg["finish_reason"] = _finish_reason output_messages.append(msg) + # Global fallback: only if no candidates produced parts + if not output_messages: + try: + text = response.text + if text: + output_messages.append({ + "role": "assistant", + "parts": [{"type": "text", "content": text}], + }) + except Exception: + pass else: # No candidates field: fall back to response.text try: if isinstance(response.text, list): - for item in response: + for item in response.text: output_messages.append({ "role": "assistant", - "parts": [{"type": "text", "content": item.text}], + "parts": [{"type": "text", "content": item if isinstance(item, str) else getattr(item, "text", str(item))}], })And remove the per-candidate fallback (lines 663-670).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opentelemetry-instrumentation-google-generativeai/opentelemetry/instrumentation/google_generativeai/span_utils.py` around lines 663 - 690, The per-candidate fallback that reads response.text (currently inside the candidate-processing loop in span_utils.py) must be removed so a candidate with empty content.parts doesn't pull in response-wide text; instead, after finishing the candidates loop, add a single response-level fallback that inspects response.text once and appends one assistant message if appropriate. Also fix the list-iteration bug: when handling the response.text fallback, check isinstance(response.text, list) and iterate for item in response.text (not for item in response), and when items are strings or objects use their appropriate .text/value to build parts; remove the per-candidate fallback block and implement the corrected post-loop response.text handling in the same function where parts, msg, and output_messages are used.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/opentelemetry-instrumentation-google-generativeai/opentelemetry/instrumentation/google_generativeai/span_utils.py`:
- Around line 14-16: Fix the broken import in span_utils.py: the import of
SpanAttributes from opentelemetry.semconv_ai has a missing opening parenthesis
and line-breaked syntax causing a SyntaxError. Replace the malformed lines with
a valid import statement for SpanAttributes (e.g., import SpanAttributes
correctly from opentelemetry.semconv_ai) so the module can load; locate the
faulty import near the top of span_utils.py where SpanAttributes is referenced.
---
Outside diff comments:
In
`@packages/opentelemetry-instrumentation-google-generativeai/opentelemetry/instrumentation/google_generativeai/span_utils.py`:
- Around line 592-601: _system_instruction_to_parts currently calls a removed
helper `_parts_from_genai_part_sync`, causing a NameError; replace that call
with the existing part-processing logic used elsewhere by either calling the
correct helper (e.g., `_parts_from_genai_part`) or inlining the same logic used
for sync input handling: iterate si.parts, enumerate to get idx, and for each
part p call the correct part-to-span conversion routine (passing span and idx)
to extend out; ensure you preserve the returned structure (list of dicts) and
the fallback string conversion path.
---
Duplicate comments:
In
`@packages/opentelemetry-instrumentation-google-generativeai/opentelemetry/instrumentation/google_generativeai/span_utils.py`:
- Around line 238-261: In _process_content_item the code wrongly references an
undefined variable parts_acc and calls a non-existent helper
_parts_from_genai_part_async; replace that branch to iterate content_item.parts
and call the existing _process_content_part for each part (passing span,
part_index and/or trace/span ids as needed) and extend processed_content with
its results, keeping the existing image handling via _process_image_part and
returning processed_content; update references to span.context.trace_id and
span.context.span_id only where _process_content_part requires them.
- Around line 663-690: The per-candidate fallback that reads response.text
(currently inside the candidate-processing loop in span_utils.py) must be
removed so a candidate with empty content.parts doesn't pull in response-wide
text; instead, after finishing the candidates loop, add a single response-level
fallback that inspects response.text once and appends one assistant message if
appropriate. Also fix the list-iteration bug: when handling the response.text
fallback, check isinstance(response.text, list) and iterate for item in
response.text (not for item in response), and when items are strings or objects
use their appropriate .text/value to build parts; remove the per-candidate
fallback block and implement the corrected post-loop response.text handling in
the same function where parts, msg, and output_messages are used.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 40aae1cd-c3ae-43da-bf8f-5721dae06482
📒 Files selected for processing (1)
packages/opentelemetry-instrumentation-google-generativeai/opentelemetry/instrumentation/google_generativeai/span_utils.py
…genai_part_async in _process_content_item Refactor content processing to use _process_content_part for better handling of parts.
… (was LLM_USAGE_TOTAL_TOKENS)
|
Addressed the two Critical issues flagged by CodeRabbit: Fix 1 — The original line called
result = await _process_content_part(part, span, part_index)
if result:
processed_content.append(result)Fix 2 — The test was asserting against |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/opentelemetry-instrumentation-google-generativeai/opentelemetry/instrumentation/google_generativeai/span_utils.py (2)
594-603:⚠️ Potential issue | 🔴 CriticalCritical:
_parts_from_genai_part_syncis undefined and will causeNameError.Line 601 calls
_parts_from_genai_part_syncwhich does not exist in this module. This will crash at runtime when processing system instructions that contain parts.🐛 Proposed fix using existing helpers
def _system_instruction_to_parts(si, span): """OTel: flat array of parts for gen_ai.system_instructions.""" if isinstance(si, str): return [{"type": "text", "content": si}] if hasattr(si, "parts") and si.parts: out = [] for idx, p in enumerate(si.parts): - out.extend(_parts_from_genai_part_sync(p, span, idx)) + if hasattr(p, "text") and p.text: + out.append({"type": "text", "content": p.text}) + elif _is_image_part(p): + img = _otel_image_part_from_genai_part(p, span, idx, sync=True) + if img: + out.append(img) + else: + out.append({"type": "text", "content": str(p)}) return out return [{"type": "text", "content": str(si)}]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opentelemetry-instrumentation-google-generativeai/opentelemetry/instrumentation/google_generativeai/span_utils.py` around lines 594 - 603, The function _system_instruction_to_parts calls an undefined helper _parts_from_genai_part_sync causing a NameError; update the call to use the existing helper (e.g., _parts_from_genai_part) or add a thin synchronous wrapper that delegates to the existing implementation so that _system_instruction_to_parts correctly iterates si.parts without referencing a nonexistent symbol; change the call site in _system_instruction_to_parts (the enumerate loop) to invoke the actual helper name used elsewhere in this module.
722-747:⚠️ Potential issue | 🔴 CriticalCritical: Variable
umis used but never defined.Lines 722-747 reference
um.total_token_count,um.candidates_token_count, andum.prompt_token_countwithout definingum, causingNameErrorfor every non-streaming response. Additionally,_GCP_GEN_AIand_GEN_CONTENTused in lines 742-743 are not imported in this module.🐛 Proposed fix
Add to imports at the top of the file:
from opentelemetry.instrumentation.google_generativeai import ( _GCP_GEN_AI, _GEN_CONTENT, )Then fix the function by assigning
umbefore use:def set_model_response_attributes( span, response, llm_model, token_histogram, stream_finish_reasons=None ): if not span.is_recording(): return _set_span_attribute(span, GenAIAttributes.GEN_AI_RESPONSE_MODEL, llm_model) if hasattr(response, "usage_metadata"): + um = response.usage_metadata _set_span_attribute( span, SpanAttributes.GEN_AI_USAGE_TOTAL_TOKENS, um.total_token_count, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opentelemetry-instrumentation-google-generativeai/opentelemetry/instrumentation/google_generativeai/span_utils.py` around lines 722 - 747, The code uses an undefined variable um and missing constants; fix by first assigning um = response.usage_metadata after the hasattr(response, "usage_metadata") check (so the spans and token_histogram use the actual usage_metadata object) and import the missing constants (_GCP_GEN_AI and _GEN_CONTENT) at the top of the module from opentelemetry.instrumentation.google_generativeai; update references in span_utils.py (the block that sets GEN_AI_USAGE_* attributes and the token_histogram.record call) to use the newly assigned um and the imported constants.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/opentelemetry-instrumentation-google-generativeai/opentelemetry/instrumentation/google_generativeai/span_utils.py`:
- Around line 338-350: When building input_messages, the role value must be
normalized using _normalize_message_role so Google "model" becomes the OTel
"assistant"; update each place where role is set (e.g., when handling a single
Content object and in the list branches using variables content_item and
contents and the _process_content_item call) to call
_normalize_message_role(getattr(..., "role", "user")) before appending to
input_messages; apply this change in both the async path (where role is assigned
from content_item or contents) and the sync path locations referenced (the
places currently using getattr(..., "role", "user") such as the spots around
input_messages construction and where role is assigned) so tests like
test_async_input.py pass.
---
Outside diff comments:
In
`@packages/opentelemetry-instrumentation-google-generativeai/opentelemetry/instrumentation/google_generativeai/span_utils.py`:
- Around line 594-603: The function _system_instruction_to_parts calls an
undefined helper _parts_from_genai_part_sync causing a NameError; update the
call to use the existing helper (e.g., _parts_from_genai_part) or add a thin
synchronous wrapper that delegates to the existing implementation so that
_system_instruction_to_parts correctly iterates si.parts without referencing a
nonexistent symbol; change the call site in _system_instruction_to_parts (the
enumerate loop) to invoke the actual helper name used elsewhere in this module.
- Around line 722-747: The code uses an undefined variable um and missing
constants; fix by first assigning um = response.usage_metadata after the
hasattr(response, "usage_metadata") check (so the spans and token_histogram use
the actual usage_metadata object) and import the missing constants (_GCP_GEN_AI
and _GEN_CONTENT) at the top of the module from
opentelemetry.instrumentation.google_generativeai; update references in
span_utils.py (the block that sets GEN_AI_USAGE_* attributes and the
token_histogram.record call) to use the newly assigned um and the imported
constants.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7591ad68-4233-45ad-ba12-06feae28c127
📒 Files selected for processing (1)
packages/opentelemetry-instrumentation-google-generativeai/opentelemetry/instrumentation/google_generativeai/span_utils.py
…e() at all input_messages append sites
There was a problem hiding this comment.
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-google-generativeai/tests/test_generate_content.py (1)
162-168:⚠️ Potential issue | 🔴 CriticalAdd missing import for
MagicMockto fix test execution.Line 167 uses
MagicMock()but the class is not imported, causing aNameErrorat runtime.Fix
import pytest +from unittest.mock import MagicMock from opentelemetry.trace import StatusCode, SpanKind🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opentelemetry-instrumentation-google-generativeai/tests/test_generate_content.py` around lines 162 - 168, The test test_set_model_request_attributes_reads_system_instruction_from_config references MagicMock but doesn't import it; add the missing import (import MagicMock from unittest.mock) at the top of the test file so MagicMock used in the span setup (span = MagicMock()) is defined and the test can run.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@packages/opentelemetry-instrumentation-google-generativeai/tests/test_generate_content.py`:
- Around line 162-168: The test
test_set_model_request_attributes_reads_system_instruction_from_config
references MagicMock but doesn't import it; add the missing import (import
MagicMock from unittest.mock) at the top of the test file so MagicMock used in
the span setup (span = MagicMock()) is defined and the test can run.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 11fb6d37-f023-41f5-a362-680f6761b0b8
📒 Files selected for processing (1)
packages/opentelemetry-instrumentation-google-generativeai/tests/test_generate_content.py
|
Hi team — just flagging that the failing |
|
Addressing all CodeRabbit inline review suggestions below. No human maintainer comments exist yet. Snyk failure on Apr 19 was a quota/rate-limit issue (not a vulnerability in this PR's code) — confirmed by CodeRabbit's pre-merge check status showing 4 pass / 1 fail which matches the known Snyk quota problem. CodeRabbit suggestion assessmentCR-1 (Major):
|
| # | Severity | Issue | Action |
|---|---|---|---|
| CR-4 | Critical | Missing ( in import on line 14 |
Fix syntax error |
| CR-3 | Critical | _GCP_GEN_AI/_GEN_CONTENT used but not imported |
Add import or inline literal consistently |
| CR-1 | Major | response.text fallback bleeds across candidates in loop |
Scope fallback outside candidate loop |
| CR-2 | Major | for item in response: should be for item in response.text: |
Fix iteration target |
CR-4 and CR-3 are blockers — the module cannot load with a syntax error, and the token histogram code will throw NameError at runtime if those constants are not in scope. CR-1 and CR-2 affect correctness of span data for streaming/multi-candidate responses.
The core gen_ai.input.messages / gen_ai.output.messages migration (the primary purpose of this PR) is correct: using a JSON-serialized list of {"role": "...", "parts": [...]} objects aligns with the OTel GenAI semantic conventions spec and is consistent with the OpenAI instrumentation in this repo. The attribute naming itself does not need changes.
Code Review: 4 Critical Bugs in
|
| ID | Line(s) | Issue | Severity |
|---|---|---|---|
| CR-4 | 14 | Missing ( in from opentelemetry.semconv_ai import |
Critical — SyntaxError, module unimportable |
| CR-3 | 19–20 (missing), 752 | _GCP_GEN_AI/_GEN_CONTENT not defined; "Google" hardcoded |
Critical — NameError at runtime |
| CR-2 | 682 | for item in response: instead of for item in response.text: |
High — TypeError or wrong output |
| CR-1 | 665–672 | response.text fallback inside candidate loop |
Medium — duplicated output messages |
…, and candidate bleed in span_utils
|
Fixed all 4 issues identified in code review:
All fixes committed. Happy to iterate if anything needs adjustment. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
packages/opentelemetry-instrumentation-google-generativeai/opentelemetry/instrumentation/google_generativeai/span_utils.py (4)
725-751:⚠️ Potential issue | 🔴 CriticalCritical:
umis undefined — every non-streaming response will raiseNameError.
hasattr(response, "usage_metadata")gates the block, butum = response.usage_metadatawas lost during the refactor. Lines 729, 734, 739, and 743 all referenceum, while line 753 inconsistently uses the fullresponse.usage_metadata.candidates_token_countpath. Whenhasattr(response, "usage_metadata")is True, all four references will fail with NameError. Because calling sites wrap this in@dont_throw, the error silently swallows the entire usage/token-histogram block, making token metrics disappear without warning.Fix: Add
um = response.usage_metadataimmediately after theif hasattr(response, "usage_metadata"):condition on line 727.Diff
if hasattr(response, "usage_metadata"): + um = response.usage_metadata _set_span_attribute( span, SpanAttributes.GEN_AI_USAGE_TOTAL_TOKENS, um.total_token_count, ) _set_span_attribute( span, GenAIAttributes.GEN_AI_USAGE_OUTPUT_TOKENS, um.candidates_token_count, ) _set_span_attribute( span, GenAIAttributes.GEN_AI_USAGE_INPUT_TOKENS, um.prompt_token_count, ) if token_histogram and hasattr(response, "usage_metadata"): token_histogram.record( um.prompt_token_count, attributes={ GenAIAttributes.GEN_AI_PROVIDER_NAME: _GCP_GEN_AI, GenAIAttributes.GEN_AI_OPERATION_NAME: _GEN_CONTENT, GenAIAttributes.GEN_AI_REQUEST_MODEL: llm_model, GenAIAttributes.GEN_AI_TOKEN_TYPE: "input", GenAIAttributes.GEN_AI_RESPONSE_MODEL: llm_model, }, ) token_histogram.record( - response.usage_metadata.candidates_token_count, + um.candidates_token_count, attributes={ GenAIAttributes.GEN_AI_PROVIDER_NAME: _GCP_GEN_AI, GenAIAttributes.GEN_AI_TOKEN_TYPE: "output", GenAIAttributes.GEN_AI_RESPONSE_MODEL: llm_model, }, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opentelemetry-instrumentation-google-generativeai/opentelemetry/instrumentation/google_generativeai/span_utils.py` around lines 725 - 751, The block in span_utils.py uses um but never defines it, causing a NameError; immediately after the if hasattr(response, "usage_metadata"): line add the assignment um = response.usage_metadata so subsequent calls to _set_span_attribute and token_histogram.record can reference um (used in the span variable handling and token_histogram recording for llm_model, GenAIAttributes, etc.); ensure this single-line assignment precedes the four _set_span_attribute calls and the token_histogram.record call so the usage metadata is consistently read from response.usage_metadata.
112-159: 🛠️ Refactor suggestion | 🟠 MajorRemove dead code:
_otel_image_part_from_genai_partand async variant are never called.These unused helpers were apparently introduced to return OTel-format image parts (
{"type":"blob"|"uri","modality":"image",...}), but the active code path (_process_content_partat line 269) instead routes through_process_image_partat line 274. The dead functions create a confusing divergence between two image shapes and should be removed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opentelemetry-instrumentation-google-generativeai/opentelemetry/instrumentation/google_generativeai/span_utils.py` around lines 112 - 159, Remove the two unused helper functions _otel_image_part_from_genai_part and _otel_image_part_from_genai_part_async from the module; delete their bodies and definitions and also remove any imports they solely rely on (e.g., base64 or other symbols not used elsewhere) so there is no leftover dead code or unused-import warnings, and verify no remaining references to these function names exist (they are not called by _process_content_part/_process_image_part).
597-606:⚠️ Potential issue | 🔴 CriticalCritical:
_parts_from_genai_part_syncis not defined in this module.
_system_instruction_to_partscalls_parts_from_genai_part_sync(p, span, idx)at line 604, but this function is not defined anywhere inspan_utils.py. When a Content-likesystem_instructionwith.partsis passed, this will raiseNameErrorat request time. The only mitigation is the surroundingtry/except: passin the caller, which silently drops the system instruction attribute.The tests (test_finish_reasons.py) expect this function to:
- Return a list of part dicts (used with
extend(), notappend())- Emit
{"type": "reasoning", "content": text}whenpart.thought == True- Emit
{"type": "text", "content": text}whenpart.thoughtis False/NoneThe proposed fix using
_serialize_response_partis inadequate—that function handles response parts only, returns a single dict instead of a list, and does not support thethoughtattribute required for reasoning parts. The function_parts_from_genai_part_syncmust be properly implemented with full thinking/reasoning support.
725-740: 🛠️ Refactor suggestion | 🟠 MajorUse consistent attribute source for all usage token attributes.
GEN_AI_USAGE_TOTAL_TOKENSis read fromSpanAttributeswhileGEN_AI_USAGE_OUTPUT_TOKENSandGEN_AI_USAGE_INPUT_TOKENSare read fromGenAIAttributes. UseGenAIAttributesfor all three to maintain consistency, asGEN_AI_USAGE_INPUT_TOKENSandGEN_AI_USAGE_OUTPUT_TOKENSare not currently defined inSpanAttributes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opentelemetry-instrumentation-google-generativeai/opentelemetry/instrumentation/google_generativeai/span_utils.py` around lines 725 - 740, The code uses SpanAttributes.GEN_AI_USAGE_TOTAL_TOKENS for total tokens while using GenAIAttributes for input/output, so make all three attributes use GenAIAttributes for consistency: update the _set_span_attribute calls that currently reference SpanAttributes.GEN_AI_USAGE_TOTAL_TOKENS to use GenAIAttributes.GEN_AI_USAGE_TOTAL_TOKENS instead, keeping the other calls that set GEN_AI_USAGE_OUTPUT_TOKENS and GEN_AI_USAGE_INPUT_TOKENS (using um.total_token_count, um.candidates_token_count, um.prompt_token_count) and leaving the surrounding check on response and the _set_span_attribute/span variables unchanged.
🧹 Nitpick comments (2)
packages/opentelemetry-instrumentation-google-generativeai/opentelemetry/instrumentation/google_generativeai/span_utils.py (2)
396-514: Significant duplication between sync and async input builders.
set_input_attributes_syncreimplements verbatim the per-part / per-argument logic that_process_content_item,_process_content_part, and_process_argumentalready encode for the async path — only the image upload helper differs (_process_image_part_syncvs_process_image_part). Extracting_process_content_item_sync/_process_content_part_sync/_process_argument_sync(or parameterising the existing helpers with a sync/async image callback) would shrink this function considerably and make future format changes a one-place edit. Not a correctness issue, but the surface area as written makes it easy for the two paths to drift (e.g., role normalization or new part types getting added on only one side).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opentelemetry-instrumentation-google-generativeai/opentelemetry/instrumentation/google_generativeai/span_utils.py` around lines 396 - 514, The sync path in set_input_attributes_sync duplicates logic present in the async helpers (_process_content_item, _process_content_part, _process_argument), causing maintenance drift; refactor by extracting sync variants or parameterizing the existing helpers to accept an image-processing callback so both paths share the same traversal/normalization logic: create _process_content_item_sync/_process_content_part_sync/_process_argument_sync (or update _process_content_item/_process_content_part/_process_argument to accept a process_image callback) and have set_input_attributes_sync delegate to those, passing _process_image_part_sync (instead of _process_image_part) to handle images and keeping role normalization and part handling consistent.
630-641: Multimodal/structured response parts collapse tostr(part).
_serialize_response_partonly branches ontextandfunction_call. Anything else —inline_data(image bytes),file_data(URI),executable_code,code_execution_result,function_response— falls through to{"type":"text","content": str(part)}, which produces the protobuf repr rather than a usable representation. Given that_is_image_partand the OpenAI-styleimage_urlshape already exist for input messages, mirroring them on the output side would keepgen_ai.output.messageslossless for the common image-out and tool-output cases.♻️ Suggested extension
def _serialize_response_part(part): """Serialize a single response Part to a dict for gen_ai.output.messages.""" if hasattr(part, "text") and part.text: return {"type": "text", "content": part.text} if hasattr(part, "function_call") and part.function_call: fc = part.function_call return { "type": "function_call", "name": fc.name, - "arguments": dict(fc.args) if hasattr(fc, "args") else {}, + "arguments": _parse_function_call_arguments(getattr(fc, "args", None)), } + if hasattr(part, "function_response") and part.function_response: + fr = part.function_response + return { + "type": "function_response", + "name": getattr(fr, "name", None), + "response": _function_response_to_str(getattr(fr, "response", None)), + } + if _is_image_part(part): + mime = part.inline_data.mime_type or "application/octet-stream" + b64 = base64.b64encode(part.inline_data.data).decode("utf-8") + return {"type": "image", "mime_type": mime, "content": b64} return {"type": "text", "content": str(part)}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opentelemetry-instrumentation-google-generativeai/opentelemetry/instrumentation/google_generativeai/span_utils.py` around lines 630 - 641, The current _serialize_response_part only handles text and function_call and falls back to str(part), causing multimodal parts (inline_data, file_data, executable_code, code_execution_result, function_response) to lose structure; update _serialize_response_part to detect these attributes (or reuse _is_image_part for images) and return structured dicts instead—for example map inline_data to {"type":"image","image_url": "<data:...;base64,...">} or similar image_url shape, map file_data to {"type":"file","uri": part.file_data}, executable_code to {"type":"executable_code","content": part.executable_code}, code_execution_result to {"type":"code_execution_result","result": part.code_execution_result}, and function_response to {"type":"function_response","content": part.function_response} so gen_ai.output.messages remains lossless for images and tool outputs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/opentelemetry-instrumentation-google-generativeai/opentelemetry/instrumentation/google_generativeai/span_utils.py`:
- Around line 681-696: In the else branch handling response.text in
span_utils.py, the loop assumes each item has a .text attribute which breaks
when response.text is a list[str]; update the iteration to handle both strings
and objects by checking each item’s type (e.g., if isinstance(item, str) use
item directly, else use item.text) before appending to output_messages so string
items are not attribute-accessed and lost; keep the existing structure that
appends {"role":"assistant","parts":[{"type":"text","content": ... }]} and
preserve the outer try/except logic.
- Around line 326-379: The span assembly is emitting image parts in OpenAI shape
(via _process_image_part) while text parts use OTel shape, breaking
gen_ai.input.messages uniformity; either convert image parts to OTel-compliant
shape before adding to input_messages or remove/align the unused helper
_otel_image_part_from_genai_part and document the intentional OpenAI format.
Fix: update _process_image_part (and/or the call sites in _process_content_item
/ the block that appends to input_messages) to return OTel image parts
({"type":"blob" or "file_data", "mime_type":..., "data"/"file_uri":...})
consistent with text parts, or if you keep OpenAI format, delete
_otel_image_part_from_genai_part and add a clear comment at the
gen_ai.input.messages assembly explaining the deliberate provider-specific shape
choice so consumers aren’t confused.
---
Outside diff comments:
In
`@packages/opentelemetry-instrumentation-google-generativeai/opentelemetry/instrumentation/google_generativeai/span_utils.py`:
- Around line 725-751: The block in span_utils.py uses um but never defines it,
causing a NameError; immediately after the if hasattr(response,
"usage_metadata"): line add the assignment um = response.usage_metadata so
subsequent calls to _set_span_attribute and token_histogram.record can reference
um (used in the span variable handling and token_histogram recording for
llm_model, GenAIAttributes, etc.); ensure this single-line assignment precedes
the four _set_span_attribute calls and the token_histogram.record call so the
usage metadata is consistently read from response.usage_metadata.
- Around line 112-159: Remove the two unused helper functions
_otel_image_part_from_genai_part and _otel_image_part_from_genai_part_async from
the module; delete their bodies and definitions and also remove any imports they
solely rely on (e.g., base64 or other symbols not used elsewhere) so there is no
leftover dead code or unused-import warnings, and verify no remaining references
to these function names exist (they are not called by
_process_content_part/_process_image_part).
- Around line 725-740: The code uses SpanAttributes.GEN_AI_USAGE_TOTAL_TOKENS
for total tokens while using GenAIAttributes for input/output, so make all three
attributes use GenAIAttributes for consistency: update the _set_span_attribute
calls that currently reference SpanAttributes.GEN_AI_USAGE_TOTAL_TOKENS to use
GenAIAttributes.GEN_AI_USAGE_TOTAL_TOKENS instead, keeping the other calls that
set GEN_AI_USAGE_OUTPUT_TOKENS and GEN_AI_USAGE_INPUT_TOKENS (using
um.total_token_count, um.candidates_token_count, um.prompt_token_count) and
leaving the surrounding check on response and the _set_span_attribute/span
variables unchanged.
---
Nitpick comments:
In
`@packages/opentelemetry-instrumentation-google-generativeai/opentelemetry/instrumentation/google_generativeai/span_utils.py`:
- Around line 396-514: The sync path in set_input_attributes_sync duplicates
logic present in the async helpers (_process_content_item,
_process_content_part, _process_argument), causing maintenance drift; refactor
by extracting sync variants or parameterizing the existing helpers to accept an
image-processing callback so both paths share the same traversal/normalization
logic: create
_process_content_item_sync/_process_content_part_sync/_process_argument_sync (or
update _process_content_item/_process_content_part/_process_argument to accept a
process_image callback) and have set_input_attributes_sync delegate to those,
passing _process_image_part_sync (instead of _process_image_part) to handle
images and keeping role normalization and part handling consistent.
- Around line 630-641: The current _serialize_response_part only handles text
and function_call and falls back to str(part), causing multimodal parts
(inline_data, file_data, executable_code, code_execution_result,
function_response) to lose structure; update _serialize_response_part to detect
these attributes (or reuse _is_image_part for images) and return structured
dicts instead—for example map inline_data to {"type":"image","image_url":
"<data:...;base64,...">} or similar image_url shape, map file_data to
{"type":"file","uri": part.file_data}, executable_code to
{"type":"executable_code","content": part.executable_code},
code_execution_result to {"type":"code_execution_result","result":
part.code_execution_result}, and function_response to
{"type":"function_response","content": part.function_response} so
gen_ai.output.messages remains lossless for images and tool outputs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9d763143-6c92-41b4-b9f2-03f3d13f469b
📒 Files selected for processing (1)
packages/opentelemetry-instrumentation-google-generativeai/opentelemetry/instrumentation/google_generativeai/span_utils.py
…rs, and streaming finish_reason Four bugs survive the previous commit and break the module at import time or at runtime on every code path: - NameError: `um` referenced in `set_model_response_attributes` before assignment — fixes all four `um.xxx` calls by binding `um = response.usage_metadata` at the top of each branch. - ImportError: `_collect_finish_reasons_from_response` imported by `__init__` but absent from `span_utils` — implements the function (one mapped reason per candidate, empty list when no candidates attribute exists). - NameError: `_parts_from_genai_part_sync` called in `_system_instruction_to_parts` but not defined — implements the function with thought/reasoning block support per the existing test suite. - AttributeError: `item.text` on list items in the `response.text` fallback — the list branch is absorbed into `_output_messages_from_generate_response`, which iterates candidates correctly. Additional improvements required for the test suite to pass: - Add `_output_messages_from_generate_response` helper (always populates `finish_reason`, ``""`` when unknown) and use it in the non-streaming path of `set_response_attributes`. - Streaming path: add `finish_reason` to every emitted message; suppress empty-string messages when no `stream_last_chunk` context is available; derive `finish_reason` from the last-chunk candidates when present. - `set_model_response_attributes`: emit `gen_ai.response.finish_reasons` only when at least one reason is non-empty (using `stream_finish_reasons` arg or `_collect_finish_reasons_from_response` as fallback). Fixes CodeRabbit CR-1/CR-2/CR-3/CR-4 and closes remaining test failures in `test_finish_reasons.py`.
|
Hi @traceloop/opentelemetry-instrumentation-google-generativeai-maintainers — just pushed a follow-up commit (27fdf93) that resolves the remaining runtime errors flagged by CodeRabbit: Fixed:
All cases covered by the new |
|
Addressing the new CodeRabbit suggestions from the Apr 26 review (commit 27fdf93). New CodeRabbit findings assessmentInline — Major:
|
|
Hi @traceloop/maintainers — all four CodeRabbit issues (CR-4 SyntaxError, CR-3 missing constants, CR-2 wrong loop variable, CR-1 candidate bleed) are now fixed and committed. The branch is out of date with main but changes can be cleanly merged. Would appreciate a first human review when you get a chance. Thanks! |
|
Friendly ping @traceloop/maintainers — this PR has been open for a while and CodeRabbit has completed its review. All inline review threads have been addressed. Would love a human review when you get a chance. Happy to make any further changes needed. |
|
Rebased this branch onto the latest Conflict was isolated to While verifying with the local test suite, also caught and fixed a real regression from this branch's own history: the output-token histogram record call had lost its Branch is up to date with |
|
cc @nirga @galkleinman @dinmukhamedm — rebased this onto current main since it had drifted (conflict resolution + two regressions the merge would've silently introduced are detailed above). It's conflict-free and green now. No rush, just flagging it's current whenever you get a chance to take a first look. |
2 participants
Summary
Fixes #3515.
Migrates
google_generativeai/span_utils.pyfrom deprecated indexed span attributes to the stable OTel GenAI Semantic Conventions:gen_ai.prompt.{N}.content/gen_ai.prompt.{N}.role->gen_ai.input.messages(JSON array)gen_ai.completion.{N}.content/gen_ai.completion.{N}.role->gen_ai.output.messages(JSON array)The new attributes store a JSON-serialised list of
{"role": "...", "parts": [...]}objects, consistent with the format used by the OpenAI instrumentation in this repo.Changes
span_utils.py: Removed_set_prompt_attributes;set_input_attributesandset_input_attributes_syncnow build aninput_messageslist and write it togen_ai.input.messages;set_response_attributesbuilds anoutput_messageslist and writes it togen_ai.output.messages.tests/test_generate_content.py: Updated assertions to parse the new JSON blob attributes instead of the old indexed ones.Checklist
fix(instrumentation): ...Summary by CodeRabbit
Refactor
Tests