feat(instrumentation-google-generativeai): add OTel 1.40 GenAI semantic conventions instrumentation#931
Conversation
|
Caution Review failedPull request was closed or merged during review 📝 Walkthrough🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 (3)
packages/instrumentation-utils/eslint.config.cjs (1)
17-28: Drop no-op override entries to keep config lean.The empty override blocks are redundant and can be removed without behavior change.
♻️ Suggested cleanup
module.exports = [ { ignores: ["!**/*", "**/node_modules", "dist/**/*"], }, ...rootConfig, - { - files: ["**/*.ts", "**/*.tsx", "**/*.js", "**/*.jsx"], - rules: {}, - }, - { - files: ["**/*.ts", "**/*.tsx"], - rules: {}, - }, - { - files: ["**/*.js", "**/*.jsx"], - rules: {}, - }, ];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/instrumentation-utils/eslint.config.cjs` around lines 17 - 28, Remove the three no-op override blocks in the ESLint config (the objects with files: ["**/*.ts","**/*.tsx","**/*.js","**/*.jsx"], files: ["**/*.ts","**/*.tsx"], and files: ["**/*.js","**/*.jsx"]) since they contain empty rules and add no behavior; delete those entire override entries to keep packages/instrumentation-utils/eslint.config.cjs lean and functionally equivalent.packages/instrumentation-google-generativeai/eslint.config.cjs (1)
17-28: Remove redundant empty override blocks.These blocks do not change lint behavior and can be dropped to reduce config noise.
♻️ Suggested cleanup
module.exports = [ { ignores: ["!**/*", "**/node_modules", "dist/**/*"], }, ...rootConfig, - { - files: ["**/*.ts", "**/*.tsx", "**/*.js", "**/*.jsx"], - rules: {}, - }, - { - files: ["**/*.ts", "**/*.tsx"], - rules: {}, - }, - { - files: ["**/*.js", "**/*.jsx"], - rules: {}, - }, ];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/instrumentation-google-generativeai/eslint.config.cjs` around lines 17 - 28, Remove the redundant empty override blocks in the ESLint config: delete the extra override objects that contain only files: ["**/*.ts", "**/*.tsx", "**/*.js", "**/*.jsx"] with rules: {} and the separate overrides for ["**/*.ts","**/*.tsx"] and ["**/*.js","**/*.jsx"]; leave only the necessary override(s) or consolidate them so no empty override objects remain in eslint.config.cjs.packages/instrumentation-utils/src/content-block-mappers.ts (1)
299-307: Harden mapper input handling before property access.
block.thoughtis dereferenced without guarding nullish/non-object values, which can throw on malformed input.🛡️ Suggested hardening
export function mapGenAIContentBlock(block: any): object { if (typeof block === "string") { return { type: GenAIOtelPartType.TEXT, content: block }; } + if (!block || typeof block !== "object") { + return { type: GenAIOtelPartType.UNKNOWN }; + } // thought: true marks a model reasoning/thinking block (Gemini thinking models). // text may be undefined on malformed parts — fall back to empty string. if (block.thought === true) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/instrumentation-utils/src/content-block-mappers.ts` around lines 299 - 307, The mapper mapGenAIContentBlock accesses block.thought without guarding against null/primitive/malformed inputs, so first ensure block is an object (e.g., check block !== null && typeof block === "object") before reading properties; if the input is not an object, treat it as a TEXT part (or return { type: GenAIOtelPartType.TEXT, content: "" } / fallback as appropriate). Update mapGenAIContentBlock to validate block is an object before the block.thought check and use optional chaining or nullish coalescing for block.text (e.g., block.text ?? "") when building REASONING or TEXT outputs to avoid runtime exceptions.
🤖 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/instrumentation-google-generativeai/README.md`:
- Line 4: The README's badges are linking incorrectly: update the license badge
so the image links to the repository license (e.g., wrap the license image
markdown in a link to your LICENSE file or the Apache license URL) instead of
linking to the image itself, and fix the npm badge target by using the correct
package slug in the shields.io URL (ensure the npm badge's href and image URL
use the actual package name used by this package). Locate the badge lines in
README.md and replace the current image-only link with a linked badge pointing
to the LICENSE or Apache URL and correct the npm badge's slug to match this
package.
---
Nitpick comments:
In `@packages/instrumentation-google-generativeai/eslint.config.cjs`:
- Around line 17-28: Remove the redundant empty override blocks in the ESLint
config: delete the extra override objects that contain only files: ["**/*.ts",
"**/*.tsx", "**/*.js", "**/*.jsx"] with rules: {} and the separate overrides for
["**/*.ts","**/*.tsx"] and ["**/*.js","**/*.jsx"]; leave only the necessary
override(s) or consolidate them so no empty override objects remain in
eslint.config.cjs.
In `@packages/instrumentation-utils/eslint.config.cjs`:
- Around line 17-28: Remove the three no-op override blocks in the ESLint config
(the objects with files: ["**/*.ts","**/*.tsx","**/*.js","**/*.jsx"], files:
["**/*.ts","**/*.tsx"], and files: ["**/*.js","**/*.jsx"]) since they contain
empty rules and add no behavior; delete those entire override entries to keep
packages/instrumentation-utils/eslint.config.cjs lean and functionally
equivalent.
In `@packages/instrumentation-utils/src/content-block-mappers.ts`:
- Around line 299-307: The mapper mapGenAIContentBlock accesses block.thought
without guarding against null/primitive/malformed inputs, so first ensure block
is an object (e.g., check block !== null && typeof block === "object") before
reading properties; if the input is not an object, treat it as a TEXT part (or
return { type: GenAIOtelPartType.TEXT, content: "" } / fallback as appropriate).
Update mapGenAIContentBlock to validate block is an object before the
block.thought check and use optional chaining or nullish coalescing for
block.text (e.g., block.text ?? "") when building REASONING or TEXT outputs to
avoid runtime exceptions.
🪄 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: d532b6f2-e25e-4d88-9f2f-606cb5c32ef9
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (22)
packages/ai-semantic-conventions/src/SemanticAttributes.tspackages/instrumentation-google-generativeai/.prettierignorepackages/instrumentation-google-generativeai/README.mdpackages/instrumentation-google-generativeai/eslint.config.cjspackages/instrumentation-google-generativeai/package.jsonpackages/instrumentation-google-generativeai/rollup.config.jspackages/instrumentation-google-generativeai/src/index.tspackages/instrumentation-google-generativeai/src/instrumentation.tspackages/instrumentation-google-generativeai/src/types.tspackages/instrumentation-google-generativeai/tests/content_parts.test.tspackages/instrumentation-google-generativeai/tests/finish_reasons.test.tspackages/instrumentation-google-generativeai/tests/instrumentation.test.tspackages/instrumentation-google-generativeai/tests/semconv.test.tspackages/instrumentation-google-generativeai/tsconfig.jsonpackages/instrumentation-google-generativeai/tsconfig.test.jsonpackages/instrumentation-utils/eslint.config.cjspackages/instrumentation-utils/src/content-block-mappers.tspackages/instrumentation-utils/src/index.tspackages/instrumentation-utils/src/message-formatters.tspackages/traceloop-sdk/package.jsonpackages/traceloop-sdk/src/lib/interfaces/initialize-options.interface.tspackages/traceloop-sdk/src/lib/tracing/index.ts
lenatraceloop
force-pushed
the
lena/TLP-2077-instrumentation-google-generativeai-otel-1.40
branch
from
8854a07 to
bdd7d47
Compare
lenatraceloop
requested review from
OzBenSimhonTraceloop,
doronkopit5,
dvirski,
galzilber and
nina-kollman
| } | ||
|
|
||
| private wrapGoogleGenAI() { | ||
| // eslint-disable-next-line @typescript-eslint/no-this-alias |
There was a problem hiding this comment.
Why is the eslint disable needed in this file?
There was a problem hiding this comment.
This pattern is used consistently across all instrumentations in this codebase — it's the standard way wrappers are written here.
| @@ -0,0 +1,6 @@ | |||
| import { InstrumentationConfig } from "@opentelemetry/instrumentation"; | |||
|
|
|||
| export interface GenAIInstrumentationConfig extends InstrumentationConfig { | |||
There was a problem hiding this comment.
Yes, it's used — _config.traceContent and _config.exceptionLogger. The type declaration is needed so TypeScript knows about these custom fields.
lenatraceloop
force-pushed
the
lena/TLP-2077-instrumentation-google-generativeai-otel-1.40
branch
3 times, most recently
from
77de256 to
6081443
Compare
OzBenSimhonTraceloop
left a comment
OzBenSimhonTraceloop
left a comment
There was a problem hiding this comment.
P1 — Must Fix
- Span-level finish_reasons uses passthrough lowercase instead of OTel mapped values
instrumentation.ts:88-91, instrumentation.ts:468-476
geminiReasonToSpanValue() lowercases vendor values (SAFETY → "safety", MAX_TOKENS → "max_tokens"). Should use genaiFinishReasonMap like Anthropic/OpenAI instrumentations do, producing OTel canonical values ("content_filter", "length"). Traceloop Python uses one mapper for both paths — no dual-path design. Delete geminiReasonToSpanValue, use genaiFinishReasonMap with ?? r fallback.
- Streaming accumulation merges thinking + regular text
instrumentation.ts:293-302
Consecutive text parts concatenated without checking thought flag. A {text: "...", thought: true} followed by {text: "..."} merges into one reasoning part. Fix: add && !!prev.thought === !!part.thought to the concat condition.
P2 — Should Fix
3. OTHER maps to "other" — should be "error" (cross-SDK drift)
instrumentation.ts:74
Traceloop Python and upstream OTel Python both map OTHER → "error". JS maps to "other" which isn't in the OTel FinishReason enum. Fix: OTHER: FinishReasons.ERROR.
- BlobPart/UriPart modality omitted when mimeType absent
content-block-mapper.ts:72-78
modality is required per OTel schema. Fall back to "document" when mimeType is unknown.
Cross-SDK Drift (Python vs JS)
5. Provider name: gcp.gen_ai (Python) vs gcp.gemini (JS)
Both valid OTel values but must agree. gcp.gemini is more specific for AI Studio API. Align across SDKs.
- Top-level finish_reasons — per-candidate alignment vs dedup
Python preserves per-candidate alignment (no dedup, "" placeholders, omits attr only when all empty). JS deduplicates via new Set() and drops nulls. Align on Python's approach — array length should match candidate count.
P3 — Nice to Have
7. Missing test: per-message finish_reason in multi-candidate output — test at line 597 checks span attr but not outputMessages[*].finish_reason.
-
Missing test: streaming + thinking parts — would catch P1 #2.
-
Missing test: streaming where final chunk has null finishReason — verify span attr omitted and output message has finish_reason: "".
-
Empty string filter on span finish_reasons — filter((r) => r != null) doesn't exclude "". Add && r !== "" defensively.
|
Thanks for the detailed review! All issues have been addressed: P1 — Fixed:
P2 — Fixed: P3 — Done: |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/instrumentation-google-generativeai/src/instrumentation.ts`:
- Around line 68-86: The genaiFinishReasonMap currently maps the provider key
"OTHER" to FinishReasons.ERROR and elsewhere falls back to returning raw vendor
reason strings; update genaiFinishReasonMap so "OTHER" maps to
FinishReasons.FINISH_REASON_UNSPECIFIED (not ERROR) and change the
unknown-reason fallback logic to return FinishReasons.FINISH_REASON_UNSPECIFIED
instead of the raw vendor value, ensuring all unmapped/unknown finish reasons
normalize to FinishReasons.FINISH_REASON_UNSPECIFIED; adjust any code that reads
from genaiFinishReasonMap or the finish-reason normalization path so it uses
FinishReasons.FINISH_REASON_UNSPECIFIED as the default.
🪄 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: 5fff7c80-1eac-4d65-976c-c0ace86a39dd
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (20)
packages/ai-semantic-conventions/src/SemanticAttributes.tspackages/instrumentation-google-generativeai/.prettierignorepackages/instrumentation-google-generativeai/README.mdpackages/instrumentation-google-generativeai/eslint.config.cjspackages/instrumentation-google-generativeai/package.jsonpackages/instrumentation-google-generativeai/rollup.config.jspackages/instrumentation-google-generativeai/src/content-block-mapper.tspackages/instrumentation-google-generativeai/src/index.tspackages/instrumentation-google-generativeai/src/instrumentation.tspackages/instrumentation-google-generativeai/src/types.tspackages/instrumentation-google-generativeai/tests/content_parts.test.tspackages/instrumentation-google-generativeai/tests/finish_reasons.test.tspackages/instrumentation-google-generativeai/tests/instrumentation.test.tspackages/instrumentation-google-generativeai/tests/semconv.test.tspackages/instrumentation-google-generativeai/tsconfig.jsonpackages/instrumentation-google-generativeai/tsconfig.test.jsonpackages/instrumentation-utils/src/message-formatters.tspackages/traceloop-sdk/package.jsonpackages/traceloop-sdk/src/lib/interfaces/initialize-options.interface.tspackages/traceloop-sdk/src/lib/tracing/index.ts
✅ Files skipped from review due to trivial changes (13)
- packages/instrumentation-google-generativeai/.prettierignore
- packages/instrumentation-google-generativeai/tsconfig.test.json
- packages/instrumentation-google-generativeai/tsconfig.json
- packages/ai-semantic-conventions/src/SemanticAttributes.ts
- packages/instrumentation-google-generativeai/README.md
- packages/instrumentation-google-generativeai/src/index.ts
- packages/instrumentation-google-generativeai/eslint.config.cjs
- packages/instrumentation-google-generativeai/package.json
- packages/instrumentation-google-generativeai/tests/finish_reasons.test.ts
- packages/instrumentation-google-generativeai/tests/semconv.test.ts
- packages/instrumentation-google-generativeai/tests/content_parts.test.ts
- packages/instrumentation-google-generativeai/src/types.ts
- packages/instrumentation-google-generativeai/tests/instrumentation.test.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/traceloop-sdk/src/lib/interfaces/initialize-options.interface.ts
- packages/traceloop-sdk/package.json
- packages/traceloop-sdk/src/lib/tracing/index.ts
- packages/instrumentation-utils/src/message-formatters.ts
- packages/instrumentation-google-generativeai/rollup.config.js
- packages/instrumentation-google-generativeai/src/content-block-mapper.ts
…ic conventions instrumentation
lenatraceloop
force-pushed
the
lena/TLP-2077-instrumentation-google-generativeai-otel-1.40
branch
from
aba3dbd to
197c84f
Compare
3 participants
Summary
Attributes captured (aligned with OTel 1.40 upstream):
Test coverage: 100% — 131 tests across 4 test files:
Fixes TLP-2077
Test plan
Summary by CodeRabbit
New Features
Documentation
Tests
Chores