feat: adds business unit & user names & ids to tracing - #4562
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughSummary by CodeRabbit
WalkthroughFour new ChangesBusiness unit and user identity tracing
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
Confidence Score: 5/5The change is additive and isolated to the tracing layer; no existing behavior in the critical request path is altered. All four new attributes are guarded by independent non-empty checks in both code paths, matching the surrounding team/customer pattern. The refactoring of teamName/customerName guards in PopulateContextAttributes aligns the function with bifrost.go's long-standing independent-guard style. There are no internal callers of PopulateContextAttributes so there is no in-repo regression risk, and the bifrost.go path makes no control-flow changes. No files require special attention. Important Files Changed
Reviews (2): Last reviewed commit: "feat: adds business unit & user names & ..." | Re-trigger Greptile |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
framework/tracing/llmspan.go (1)
153-163: ⚡ Quick winAdd table-driven coverage for the new context-attribute permutations.
This change expands behavior via new parameters plus conditional emission; please add focused tests for
(id,name)present vs missing cases so regressions are caught quickly.As per coding guidelines, “Apply standard Go review practices… deterministic tests, and table-driven coverage for behavior changes.”
Also applies to: 198-205
🤖 Prompt for 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. In `@framework/tracing/llmspan.go` around lines 153 - 163, The PopulateContextAttributes function now accepts multiple new (id, name) parameter pairs but lacks table-driven test coverage for these new permutations. Create a comprehensive table-driven test that systematically covers scenarios where each parameter pair (virtualKeyID/virtualKeyName, selectedKeyID/selectedKeyName, routingRuleID/routingRuleName, teamID/teamName, customerID/customerName, businessUnitID/businessUnitName, userID/userName) is present, absent, or partially populated. Ensure the test validates that attributes are correctly conditionally emitted based on which parameters are provided to catch regressions in the conditional attribute population logic.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@framework/tracing/llmspan.go`:
- Around line 153-163: The PopulateContextAttributes function now accepts
multiple new (id, name) parameter pairs but lacks table-driven test coverage for
these new permutations. Create a comprehensive table-driven test that
systematically covers scenarios where each parameter pair
(virtualKeyID/virtualKeyName, selectedKeyID/selectedKeyName,
routingRuleID/routingRuleName, teamID/teamName, customerID/customerName,
businessUnitID/businessUnitName, userID/userName) is present, absent, or
partially populated. Ensure the test validates that attributes are correctly
conditionally emitted based on which parameters are provided to catch
regressions in the conditional attribute population logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e0436d4d-9fe3-47d5-97e3-f1bdf2be4194
📒 Files selected for processing (3)
core/bifrost.gocore/schemas/trace.goframework/tracing/llmspan.go
70ed897 to
6060b08
Compare
Merge activity
|

Summary
Adds tracing support for
business_unitandusercontext attributes, enabling these governance-related identifiers to be captured in spans alongside the existing team and customer attributes.Changes
AttrBifrostBusinessUnitID,AttrBifrostBusinessUnitName,AttrBifrostUserID, andAttrBifrostUserNametrace attribute constants tocore/schemas/trace.goPopulateContextAttributesinframework/tracing/llmspan.goto accept and emitbusinessUnitID,businessUnitName,userID, anduserNameparameters when non-emptyexecuteRequestWithRetriesincore/bifrost.goto readBifrostContextKeyGovernanceBusinessUnitID,BifrostContextKeyGovernanceBusinessUnitName,BifrostContextKeyUserID, andBifrostContextKeyUserNamefrom context and set them as span attributesType of change
Affected areas
How to test
Populate the relevant context keys (
BifrostContextKeyGovernanceBusinessUnitID,BifrostContextKeyGovernanceBusinessUnitName,BifrostContextKeyUserID,BifrostContextKeyUserName) on a request context and verify the correspondingbifrost.business_unit.id,bifrost.business_unit.name,bifrost.user.id, andbifrost.user.nameattributes appear on the resulting trace span.go test ./...Breaking changes
Security considerations
userIDanduserNamevalues sourced from request context will be written to trace spans. Ensure that any user-identifying data emitted here complies with your data retention and PII policies for your tracing backend.Checklist
docs/contributing/README.mdand followed the guidelines