feat: add connector test cases and missing attributes - #5059
Pratham-Mishra04 merged 1 commit into
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 49 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis PR adds a canonical ChangesEnrichment Registry and Telemetry Parity
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Tracer as PopulateLLMResponseAttributes
participant Resp as Response
participant Ctx as Context
participant Span as Trace Span
Tracer->>Resp: GetExtraFields (model resolution)
Tracer->>Span: set AttrBifrostAlias if model differs
Tracer->>Ctx: read routing engines
Tracer->>Span: set AttrBifrostRoutingEngineUsed (joined)
sequenceDiagram
participant Hook as PostLLMHook
participant Governance
participant Labels as labelValues map
participant Prom as Prometheus Metrics
Hook->>Governance: read business_unit_id/name
Governance-->>Hook: businessUnitID, businessUnitName
Hook->>Labels: append business_unit_id/name
Labels->>Prom: emit request metrics with labels
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
1232b37 to
af5c51c
Compare
8949996 to
575d0b6
Compare
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (3): Last reviewed commit: "feat: add connector test cases and missi..." | Re-trigger Greptile |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@core/schemas/enrichment.go`:
- Around line 3-46: The type-level comment on EnrichmentDim is stale and
contradicts the registry and tracer behavior for alias and routing_engine_used.
Update the doc comment in EnrichmentDim to match the actual registry entries and
framework/tracing/tracer.go’s PopulateLLMResponseAttributes, stating that these
dimensions do have SpanAttr values and are attached to the span like other
dimensions so connectors can read them normally. Remove the obsolete guidance
about special-casing empty SpanAttr so future Datadog/BigQuery authors follow
the registry correctly.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 48200d91-bab0-4890-8e79-3a6aeb99ff6a
📒 Files selected for processing (6)
core/schemas/enrichment.gocore/schemas/enrichment_test.gocore/schemas/trace.goframework/tracing/tracer.goplugins/telemetry/main.goplugins/telemetry/parity_test.go
af5c51c to
f82483a
Compare
575d0b6 to
4453cfd
Compare
Merge activity
|
The base branch was changed.
f82483a to
70e68aa
Compare
## Summary Introduces a canonical enrichment dimension registry (`EnrichmentDims`) in `core/schemas` as the single source of truth for the identity/context dimensions (provider, model, team, customer, business unit, virtual key, etc.) that connectors attach to telemetry. This prevents curated emitters — Prometheus labels, Datadog metric tags, BigQuery columns — from silently drifting apart when dimensions are added or removed. ## Changes - Added `core/schemas/enrichment.go` defining `EnrichmentDim` and the `EnrichmentDims` registry, with helper functions (`MetricSafeEnrichmentDims`, `EnrichmentDimNames`, `MetricSafeEnrichmentDimNames`, `EnrichmentDimColumnNames`) for deriving connector-specific lists. - Added two new canonical span attribute constants, `AttrBifrostAlias` and `AttrBifrostRoutingEngineUsed`, to `core/schemas/trace.go`. - Updated `framework/tracing/tracer.go` to set `AttrBifrostAlias` and `AttrBifrostRoutingEngineUsed` on the span inside `PopulateLLMResponseAttributes`, so all connectors read these post-response dimensions from one place rather than computing them independently. - Promoted `defaultBifrostLabelNames` in `plugins/telemetry/main.go` from a local variable to a package-level variable so the conformance test can assert against it. Added `business_unit_id` and `business_unit_name` as Prometheus labels and extracted their values from context in `PostLLMHook`. - Added `core/schemas/enrichment_test.go` with structural guards: array (`Multi`) dimensions must never be `MetricSafe`, and all dimension names must be unique. - Added `plugins/telemetry/parity_test.go` as a conformance test asserting that the Prometheus label set covers every metric-safe registry dimension and contains no high-cardinality (record-tier) dimensions. ## Type of change - [ ] Bug fix - [x] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [x] Plugins - [ ] UI (React) - [ ] Docs ## How to test ```sh go test ./core/schemas/... go test ./plugins/telemetry/... go test ./framework/tracing/... go test ./... ``` The new conformance test (`TestPrometheusLabelsMatchEnrichmentRegistry`) will fail if a metric-safe dimension is added to the registry but not to the Prometheus label list, or if a high-cardinality dimension is accidentally added as a label. The structural tests (`TestArrayDimsAreNeverMetricSafe`, `TestEnrichmentDimNamesUnique`) guard registry integrity. ## Screenshots/Recordings N/A ## Breaking changes - [ ] Yes - [x] No ## Related issues N/A ## Security considerations No new auth, secrets, or PII handling introduced. The `alias` and `routing_engine_used` span attributes contain model names and routing engine identifiers, consistent with existing telemetry data already emitted. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [ ] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
## Summary Introduces a canonical enrichment dimension registry (`EnrichmentDims`) in `core/schemas` as the single source of truth for the identity/context dimensions (provider, model, team, customer, business unit, virtual key, etc.) that connectors attach to telemetry. This prevents curated emitters — Prometheus labels, Datadog metric tags, BigQuery columns — from silently drifting apart when dimensions are added or removed. ## Changes - Added `core/schemas/enrichment.go` defining `EnrichmentDim` and the `EnrichmentDims` registry, with helper functions (`MetricSafeEnrichmentDims`, `EnrichmentDimNames`, `MetricSafeEnrichmentDimNames`, `EnrichmentDimColumnNames`) for deriving connector-specific lists. - Added two new canonical span attribute constants, `AttrBifrostAlias` and `AttrBifrostRoutingEngineUsed`, to `core/schemas/trace.go`. - Updated `framework/tracing/tracer.go` to set `AttrBifrostAlias` and `AttrBifrostRoutingEngineUsed` on the span inside `PopulateLLMResponseAttributes`, so all connectors read these post-response dimensions from one place rather than computing them independently. - Promoted `defaultBifrostLabelNames` in `plugins/telemetry/main.go` from a local variable to a package-level variable so the conformance test can assert against it. Added `business_unit_id` and `business_unit_name` as Prometheus labels and extracted their values from context in `PostLLMHook`. - Added `core/schemas/enrichment_test.go` with structural guards: array (`Multi`) dimensions must never be `MetricSafe`, and all dimension names must be unique. - Added `plugins/telemetry/parity_test.go` as a conformance test asserting that the Prometheus label set covers every metric-safe registry dimension and contains no high-cardinality (record-tier) dimensions. ## Type of change - [ ] Bug fix - [x] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [x] Plugins - [ ] UI (React) - [ ] Docs ## How to test ```sh go test ./core/schemas/... go test ./plugins/telemetry/... go test ./framework/tracing/... go test ./... ``` The new conformance test (`TestPrometheusLabelsMatchEnrichmentRegistry`) will fail if a metric-safe dimension is added to the registry but not to the Prometheus label list, or if a high-cardinality dimension is accidentally added as a label. The structural tests (`TestArrayDimsAreNeverMetricSafe`, `TestEnrichmentDimNamesUnique`) guard registry integrity. ## Screenshots/Recordings N/A ## Breaking changes - [ ] Yes - [x] No ## Related issues N/A ## Security considerations No new auth, secrets, or PII handling introduced. The `alias` and `routing_engine_used` span attributes contain model names and routing engine identifiers, consistent with existing telemetry data already emitted. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [ ] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
## Summary Introduces a canonical enrichment dimension registry (`EnrichmentDims`) in `core/schemas` as the single source of truth for the identity/context dimensions (provider, model, team, customer, business unit, virtual key, etc.) that connectors attach to telemetry. This prevents curated emitters — Prometheus labels, Datadog metric tags, BigQuery columns — from silently drifting apart when dimensions are added or removed. ## Changes - Added `core/schemas/enrichment.go` defining `EnrichmentDim` and the `EnrichmentDims` registry, with helper functions (`MetricSafeEnrichmentDims`, `EnrichmentDimNames`, `MetricSafeEnrichmentDimNames`, `EnrichmentDimColumnNames`) for deriving connector-specific lists. - Added two new canonical span attribute constants, `AttrBifrostAlias` and `AttrBifrostRoutingEngineUsed`, to `core/schemas/trace.go`. - Updated `framework/tracing/tracer.go` to set `AttrBifrostAlias` and `AttrBifrostRoutingEngineUsed` on the span inside `PopulateLLMResponseAttributes`, so all connectors read these post-response dimensions from one place rather than computing them independently. - Promoted `defaultBifrostLabelNames` in `plugins/telemetry/main.go` from a local variable to a package-level variable so the conformance test can assert against it. Added `business_unit_id` and `business_unit_name` as Prometheus labels and extracted their values from context in `PostLLMHook`. - Added `core/schemas/enrichment_test.go` with structural guards: array (`Multi`) dimensions must never be `MetricSafe`, and all dimension names must be unique. - Added `plugins/telemetry/parity_test.go` as a conformance test asserting that the Prometheus label set covers every metric-safe registry dimension and contains no high-cardinality (record-tier) dimensions. ## Type of change - [ ] Bug fix - [x] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [x] Plugins - [ ] UI (React) - [ ] Docs ## How to test ```sh go test ./core/schemas/... go test ./plugins/telemetry/... go test ./framework/tracing/... go test ./... ``` The new conformance test (`TestPrometheusLabelsMatchEnrichmentRegistry`) will fail if a metric-safe dimension is added to the registry but not to the Prometheus label list, or if a high-cardinality dimension is accidentally added as a label. The structural tests (`TestArrayDimsAreNeverMetricSafe`, `TestEnrichmentDimNamesUnique`) guard registry integrity. ## Screenshots/Recordings N/A ## Breaking changes - [ ] Yes - [x] No ## Related issues N/A ## Security considerations No new auth, secrets, or PII handling introduced. The `alias` and `routing_engine_used` span attributes contain model names and routing engine identifiers, consistent with existing telemetry data already emitted. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [ ] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable

Summary
Introduces a canonical enrichment dimension registry (
EnrichmentDims) incore/schemasas the single source of truth for the identity/context dimensions (provider, model, team, customer, business unit, virtual key, etc.) that connectors attach to telemetry. This prevents curated emitters — Prometheus labels, Datadog metric tags, BigQuery columns — from silently drifting apart when dimensions are added or removed.Changes
core/schemas/enrichment.godefiningEnrichmentDimand theEnrichmentDimsregistry, with helper functions (MetricSafeEnrichmentDims,EnrichmentDimNames,MetricSafeEnrichmentDimNames,EnrichmentDimColumnNames) for deriving connector-specific lists.AttrBifrostAliasandAttrBifrostRoutingEngineUsed, tocore/schemas/trace.go.framework/tracing/tracer.goto setAttrBifrostAliasandAttrBifrostRoutingEngineUsedon the span insidePopulateLLMResponseAttributes, so all connectors read these post-response dimensions from one place rather than computing them independently.defaultBifrostLabelNamesinplugins/telemetry/main.gofrom a local variable to a package-level variable so the conformance test can assert against it. Addedbusiness_unit_idandbusiness_unit_nameas Prometheus labels and extracted their values from context inPostLLMHook.core/schemas/enrichment_test.gowith structural guards: array (Multi) dimensions must never beMetricSafe, and all dimension names must be unique.plugins/telemetry/parity_test.goas a conformance test asserting that the Prometheus label set covers every metric-safe registry dimension and contains no high-cardinality (record-tier) dimensions.Type of change
Affected areas
How to test
The new conformance test (
TestPrometheusLabelsMatchEnrichmentRegistry) will fail if a metric-safe dimension is added to the registry but not to the Prometheus label list, or if a high-cardinality dimension is accidentally added as a label. The structural tests (TestArrayDimsAreNeverMetricSafe,TestEnrichmentDimNamesUnique) guard registry integrity.Screenshots/Recordings
N/A
Breaking changes
Related issues
N/A
Security considerations
No new auth, secrets, or PII handling introduced. The
aliasandrouting_engine_usedspan attributes contain model names and routing engine identifiers, consistent with existing telemetry data already emitted.Checklist
docs/contributing/README.mdand followed the guidelines