feat(google-genai): add util-backed 2.x instrumentation - #250
Conversation
d18ea4c to
4f9a4ba
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| server_url = getattr(config, "server_url", "") | ||
| if server_url: | ||
| server_address = server_url | ||
| if "aiplatform.googleapis.com" in server_url: |
ralf0131
left a comment
ralf0131
left a comment
There was a problem hiding this comment.
Summary
This PR adds comprehensive Google GenAI Python SDK 2.x instrumentation backed by opentelemetry-util-genai. The implementation follows the established LoongSuite instrumentation patterns, supports both sync/async surfaces (generate_content, embed_content, interactions), and includes thorough fail-open error handling via the hook_advice mechanism. Test coverage is extensive (205 passed, 95 skipped) with dedicated fail-open tests for every telemetry failure mode.
Findings
- [Info]
generate_content.py:1— 1021-line file could benefit from splitting mapping helpers into a separate module - [Info]
_compat.py:31— Bridge module is well-documented; consider a greppable TODO marker for the cleanup target - [Warning]
_compat.py:218— Reasoning tokens silently added to output_tokens total; verify this matches the GenAI semantic convention intent - [Info]
_stream.py:97—_StreamWrapperMetametaclass pattern is clean and well-commented - [Info]
generate_content.py:69— MCP import guard is correct for typical install-time dependency resolution
Suggestions
- File size: Consider extracting
_map_generate_content_config,_extract_*helpers and the_MessageConverterinto a_mapping.pymodule. This would makegenerate_content.pyfocus purely on the instrumentation lifecycle. - Reasoning token accounting: Add a brief comment or assertion clarifying whether
output_tokensshould include or exclude reasoning tokens per the semantic conventions. - UPSTREAM.md: The upstream alignment documentation is excellent. Consider keeping it updated as the upstream 1.0b1 evolves.
Overall
Solid, well-tested instrumentation that aligns with both upstream OTel conventions and LoongSuite's existing patterns. The fail-open design via hook_advice is consistently applied across all surfaces. No blocking issues — the findings above are informational.
Automated review by github-manager-bot
| @@ -0,0 +1,1021 @@ | |||
| # Copyright The OpenTelemetry Authors | |||
There was a problem hiding this comment.
[Info] This file is 1021 lines — consider splitting the _GenerateContentHandler (span/metric lifecycle) from the pure mapping helpers (_map_*, _extract_*) into separate modules. The mapping functions are stateless and testable in isolation; separating them would reduce cognitive load when navigating the instrumentation lifecycle.
| import timeit | ||
| from dataclasses import asdict, dataclass | ||
| from types import TracebackType | ||
| from typing import Any |
There was a problem hiding this comment.
[Info] The bridge pattern here is well-documented — the module docstring clearly explains this is a removable impedance match. One suggestion: consider adding a # TODO(loongsuite-util-migration): remove after util migrates to 1.x marker at the module level so the cleanup target is greppable.
| (_OUTPUT_MESSAGES, self.output_messages), | ||
| (_SYSTEM_INSTRUCTIONS, self.system_instruction), | ||
| ) | ||
| for key, messages in message_attributes: |
There was a problem hiding this comment.
[Warning] In _prepare_finish, self.output_tokens = (self.output_tokens or 0) + self.thinking_tokens silently adds reasoning tokens to the total output token count. This is correct if the upstream semantic convention defines output_tokens as inclusive of reasoning, but if it should be exclusive, this would inflate the reported count. Worth confirming against the GenAI semantic conventions spec — or adding a comment clarifying the intended behavior.
| ``for chunk in wrapper`` or ``with wrapper``. The hook methods are called | ||
| internally by the wrapper lifecycle and are not part of the public API. | ||
| """ | ||
|
|
There was a problem hiding this comment.
[Info] The _StreamWrapperMeta metaclass resolving both ABCMeta and wrapt.ObjectProxy's metaclass is a clean solution. The comment explaining why this is needed is helpful for future maintainers.
| from .tool_call_wrapper import wrapped_tool | ||
|
|
||
| _is_mcp_imported = False | ||
| McpClientSession = McpTool = None |
There was a problem hiding this comment.
[Info] The MCP client import guard (_is_mcp_imported) is correctly placed at module level with try/except ImportError. One minor note: the global _is_mcp_imported flag is set to False initially and then potentially overwritten. If this module is imported before mcp is installed but mcp is installed later at runtime, the flag won't update. This is fine for the typical use case (dependencies are fixed at install time), but worth being aware of in dynamic environments.
ralf0131
left a comment
ralf0131
left a comment
There was a problem hiding this comment.
Reviewed the core instrumentation structure. The implementation follows established OTel instrumentation patterns with clean separation of concerns:
Architecture ✅
instrumentor.pyfollowsBaseInstrumentorcontract correctly_compat.pybridge layer properly adapts upstream 1.0 API to LoongSuite extensions_stream.pyhandles sync/async streams with proper context manager protocol and finalizationgenerate_content.pycovers sync/async generate_content, embed_content, and interactions
Key Observations
- AllowList utility supports regex patterns with proper escaping
- Stream wrappers use
wrapt.ObjectProxywith correct metaclass for type compatibility - Token usage accounting and TTFT tracking are properly integrated
- Test coverage is comprehensive with VCR cassettes for deterministic replay
Minor Note
- Large PR (10K+ lines) — maintainer may want to verify upstream semantic convention alignment separately
Overall the code is well-structured and follows the project's established patterns.
Automated review by github-manager-bot
|
This PR has conflicts with the git fetch origin
git checkout feat/google-genai-observability
git rebase origin/main
# resolve conflicts, then:
git push --force-with-leaseThis is a one-time reminder. Feel free to @mention me for a re-review after conflicts are resolved. Automated notification by github-manager-bot |
ralf0131
left a comment
ralf0131
left a comment
There was a problem hiding this comment.
Summary
Comprehensive Google GenAI SDK 2.x instrumentation package aligned with upstream OTel opentelemetry-instrumentation-google-genai==1.0b1. Well-structured with proper fail-open semantics, sync/async parity, streaming TTFT support, embeddings, interactions, and automatic function tool wrapping. Test coverage (205 passed, 95 skipped) is thorough including fail-open edge cases.
Findings
- [Info]
generate_content.pyis 1021 lines — consider splitting_instrument_generate_contentand_uninstrument_generate_contentinto a separate module if it grows further, to keep the file navigable. - [Info] The
_is_mcp_importedmodule-level flag withMcpClientSession/McpToolglobals is fine for optional dependency handling, but worth noting that importingmcpat module load time could trigger side effects in environments wheremcpis installed but not intended for use with this instrumentor.
Suggestions
- The UPSTREAM.md baseline documentation is excellent. Consider adding a brief note on the divergence points (e.g.,
hook_adviceintegration, multimodal extensions) to help future maintainers track what's LoongSuite-specific vs upstream.
Automated review by github-manager-bot
3815b12 to
42e416e
Compare
ralf0131
left a comment
ralf0131
left a comment
There was a problem hiding this comment.
Summary
Re-review after new commits since last approval (July 31). Latest changes include streaming output message aggregation fix and additional test coverage.
Key Changes Since Last Review
- Streaming output aggregation (
generate_content.py,message.py): IntroducesStreamOutputMessageAccumulatorto properly aggregate streaming deltas by candidate index. This ensuresgen_ai.output.messagescontains complete logical responses instead of one message per SSE chunk — a correct fix for a real data quality issue. - Test coverage: New
test_stream_accumulator.py(135 lines) with thorough unit tests for the accumulator logic. Updated streaming base tests and fail-open tests to verify the aggregation behavior. - AgentScope instrumentation (
_v2_middleware.py): Significant updates to v2 middleware with proper skill instrumentation and expanded test coverage (893 lines of new tests). - QwenPaw instrumentation (
patch.py): Enhanced patch with better session tracking and turn boundary detection. - Multimodal upload (
multimodal_upload_hook.py): Improved hook with better error handling.
All changes are well-tested and follow established patterns. The streaming aggregation fix is particularly important for data quality.
LGTM ✅
Automated review by "github-manager-bot"
5 participants
Description
This PR adds LoongSuite instrumentation for the Google Gen AI Python SDK 2.x, backed by
opentelemetry-util-genai.The provider implementation is aligned with the canonical OpenTelemetry Python GenAI Google instrumentation (1.0b1/current upstream snapshot), while retaining LoongSuite's shared GenAI util for completion hooks, content capture, multimodal extensions, and client metrics.
Supported synchronous and asynchronous surfaces include:
models.generate_contentmodels.generate_content_streammodels.embed_contentinteractions.create, including streamingThe instrumentation records LLM, EMBEDDING, and TOOL spans; request/response metadata; response IDs; finish reasons; token usage; captured messages; streaming TTFT; embedding dimensions; errors; and GenAI client metrics.
After PR #245, instrumentation-only work is isolated with
hook_advice: provider/application calls and stream iteration execute exactly once outside advice, while telemetry prepare, mapping, chunk, close/finalize, and wrapper-construction failures degrade telemetry without changing the provider result, chunk identity/order, cancellation,GeneratorExit, or original exception.Google ADK changes and Robin's commercial
_SUPPRESS_LLM_SDK_KEYoverlay are intentionally out of scope.Fixes # (N/A)
Type of change
How Has This Been Tested?
205 passed, 95 skippedon the latest package environmenttox -e precommittox -e generate-workflows--vcr-record=nonegoogle-genai 2.14.0Gemini verification with CMS readbackThe live matrix covered sync/async non-streaming, sync/async streaming, embeddings, interactions, interaction streaming, deterministic function tools, multi-turn calls, two concurrent clients, early close/aclose, a real provider error, and injected response/chunk telemetry failures. CMS read back all 20 scenario traces and all 42 exported spans, including LLM → TOOL, EMBEDDING, TTFT, token usage, and ERROR trees.
Does This PR Require a Core Repo Change?
Checklist:
See contributing.md for styleguide, changelog guidelines, and more.
Validation Evidence
Spec and Scope
UPSTREAM.md.Local Checks
Real E2E Matrix
interactions.createLLM spansClientError, ERROR telemetryUpstream comparison
Both upstream implementations contain useful local span-isolation patterns, but still mix some request/stream telemetry callbacks with the business boundary. The LoongSuite delta keeps the upstream provider mapping while applying the stricter PR #245 fail-open lifecycle.