fix: otel plugin fixes#2727
Conversation
📝 WalkthroughSummary by CodeRabbitRelease NotesChores
WalkthroughThe PR standardizes OTEL trace type nomenclature from "otel" to "genai_extension" through a new database migration that updates existing plugin configurations, refactored metrics recording logic using per-attempt span attributes, and corresponding updates to configuration schemas, frontend forms, and TypeScript type definitions. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
🧪 Test Suite AvailableThis PR can be tested by a repository admin. |
Confidence Score: 5/5Safe to merge — no new P0/P1 issues found; previously raised concerns are addressed or explicitly acknowledged by the team. All P0/P1 concerns from prior threads (file-config breaking change, insecure default, Zod/JSON-schema divergence) are acknowledged or fixed. The remaining findings are P2 style items (trailing newlines, SpanStatusUnset guard) that do not affect correctness or production behaviour. No files require special attention beyond the acknowledged file-based config migration caveat. Important Files Changed
Reviews (3): Last reviewed commit: "fix: otel plugin fixes" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
plugins/otel/main.go (1)
379-422:⚠️ Potential issue | 🔴 CriticalRemove
schemas.SpanKindRetryfrom the candidate pool when selectingfinalSpanfor usage metrics.Retry spans are created as control-flow wrappers (line 4778 in core/bifrost.go) and do not receive usage attributes. All usage attribute population occurs exclusively in LLM response handlers in framework/tracing/llmspan.go and framework/tracing/tracer.go, which only fire for LLM call attempts, not retry control flow. Since
finalSpanselection picks the span with the latest EndTime, a retry wrapper that ends after all LLM calls will suppress metrics recording on retried requests. Change line 360 to consider onlyschemas.SpanKindLLMCallspans.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/otel/main.go` around lines 379 - 422, The finalSpan selection currently considers retry wrapper spans and can pick a retry (schemas.SpanKindRetry) which has no usage attrs; update the selection logic in plugins/otel/main.go so that when iterating spans to choose finalSpan you only consider spans of kind schemas.SpanKindLLMCall (ignore schemas.SpanKindRetry), ensuring finalSpan points to an LLM call span that contains usage attributes (so subsequent reads of finalSpan.Attributes, buildSpanAttrs, and metricsExporter.Record* calls use the correct span).transports/config.schema.json (1)
1318-1323:⚠️ Potential issue | 🟠 MajorTest fixtures must be updated to use
"genai_extension"or schema validation will fail.The enum change at line 1322 restricts
trace_typeto["genai_extension"]only. However,ValidateConfigSchema()intransports/bifrost-http/lib/validator.govalidates input JSON directly against the schema without normalizing legacy"otel"values. This will cause immediate failures in three test functions:
TestValidateConfigSchema_OtelPlugin_Valid(line 1195)TestValidateConfigSchema_OtelPlugin_MissingCollectorUrl(line 1218)TestValidateConfigSchema_OtelPlugin_MissingProtocol(line 1264)The database migration
migrationNormalizeOtelTraceTypehandles existing rows, but does not affect validation of static config files or test fixtures. Per coding guidelines, verify stack coordination: ensure fixture updates are included in this PR or a dependent stack PR, and update the example config atexamples/configs/withobservability/config.json:32accordingly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/config.schema.json` around lines 1318 - 1323, Update the tests and example config to use the new enum value "genai_extension" so schema validation passes: change the trace_type in the fixtures used by TestValidateConfigSchema_OtelPlugin_Valid, TestValidateConfigSchema_OtelPlugin_MissingCollectorUrl, and TestValidateConfigSchema_OtelPlugin_MissingProtocol to "genai_extension" and update the example config used by examples/docs to match; alternatively, if you prefer preserving legacy "otel" during validation, add a normalization step inside ValidateConfigSchema() to map "otel" -> "genai_extension" before running JSON schema validation (note migrationNormalizeOtelTraceType only handles DB rows and does not affect static fixture validation).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ui/lib/types/schemas.ts`:
- Around line 714-717: The UI schema for trace_type in ui/lib/types/schemas.ts
currently enumerates ["genai_extension", "vercel", "open_inference"], which
diverges from the backend-authoritative transports/config.schema.json that only
allows "genai_extension"; update the enum in the trace_type schema to only
permit "genai_extension" (preserve the default "genai_extension") so the
client-side validation matches the backend schema and avoids
client-pass/server-fail mismatches.
---
Outside diff comments:
In `@plugins/otel/main.go`:
- Around line 379-422: The finalSpan selection currently considers retry wrapper
spans and can pick a retry (schemas.SpanKindRetry) which has no usage attrs;
update the selection logic in plugins/otel/main.go so that when iterating spans
to choose finalSpan you only consider spans of kind schemas.SpanKindLLMCall
(ignore schemas.SpanKindRetry), ensuring finalSpan points to an LLM call span
that contains usage attributes (so subsequent reads of finalSpan.Attributes,
buildSpanAttrs, and metricsExporter.Record* calls use the correct span).
In `@transports/config.schema.json`:
- Around line 1318-1323: Update the tests and example config to use the new enum
value "genai_extension" so schema validation passes: change the trace_type in
the fixtures used by TestValidateConfigSchema_OtelPlugin_Valid,
TestValidateConfigSchema_OtelPlugin_MissingCollectorUrl, and
TestValidateConfigSchema_OtelPlugin_MissingProtocol to "genai_extension" and
update the example config used by examples/docs to match; alternatively, if you
prefer preserving legacy "otel" during validation, add a normalization step
inside ValidateConfigSchema() to map "otel" -> "genai_extension" before running
JSON schema validation (note migrationNormalizeOtelTraceType only handles DB
rows and does not affect static fixture validation).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9e55929f-9843-4164-be0d-3174dd0f7008
📒 Files selected for processing (6)
framework/configstore/migrations.goplugins/otel/main.gotransports/config.schema.jsonui/app/workspace/observability/fragments/otelFormFragment.tsxui/components/ui/select.tsxui/lib/types/schemas.ts
sammaji
force-pushed
the
04-15-fix_otel_plugin_fixes
branch
from
6aa86b9 to
577dc33
Compare
sammaji
force-pushed
the
04-15-fix_otel_plugin_fixes
branch
from
577dc33 to
e2f70ee
Compare
2 participants
Summary
Normalizes the OTEL plugin trace type from the legacy "otel" value to "genai_extension" across the codebase. This change standardizes the naming convention and improves clarity about the trace format being used.
Changes
buildSpanAttrsfunctionType of change
Affected areas
How to test
Verify the migration runs successfully and existing OTEL configurations are updated:
Test that existing OTEL plugin configurations with
trace_type: "otel"are automatically migrated totrace_type: "genai_extension"on startup.Screenshots/Recordings
N/A
Breaking changes
The migration ensures backward compatibility by automatically updating existing configurations.
Related issues
N/A
Security considerations
The migration handles plugin configuration updates safely by preserving existing settings while only modifying the trace_type field.
Checklist
docs/contributing/README.mdand followed the guidelines