Conversation
|
|
Contributor
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds governance-scoped identifiers to logs, extends database schema and matviews with governance dimensions, introduces dimension-grouped histogram and user-ranking APIs (with matview acceleration), updates logging pipelines to populate governance fields, extends UI/filtering and OpenAPI for team/user management, and bumps several module dependencies. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Handler as HTTP Handler
participant LogMgr as Log Manager
participant Store as RDB Log Store
participant MatView as DB (MatView)
participant RawTbl as DB (Raw Table)
rect rgba(76, 175, 80, 0.5)
note over Client,RawTbl: Dimension Histogram Flow — MatView Enabled (PostgreSQL)
Client->>Handler: GET /api/logs/histogram/cost/by-dimension?dimension=team
Handler->>LogMgr: GetDimensionCostHistogram(filters, bucketSize, dimension)
LogMgr->>Store: GetDimensionCostHistogram(filters, bucketSize, dimension)
activate Store
Store->>Store: Validate dimension, normalize bucketSize
Store->>Store: Check s.matViewsReady.Load() && canUseMatViewFilters(filters)
alt MatView Ready
Store->>MatView: SELECT SUM(cost) ... FROM mv_logs_hourly GROUP BY (dimension_field, bucket_time)
MatView-->>Store: Dimension-grouped results
else MatView Not Ready
Store->>RawTbl: SELECT SUM(cost) ... FROM logs GROUP BY (dimension_field, bucket_time)
RawTbl-->>Store: Raw grouped results
end
deactivate Store
Store-->>LogMgr: DimensionCostHistogramResult
LogMgr-->>Handler: DimensionCostHistogramResult
Handler-->>Client: {buckets, dimension, values}
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 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)
Comment |
Contributor
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
akshaydeo
force-pushed
the
04-06-adds_support_for_tracking_userid_teamid_customerid_and_businessunitid_in_logging
branch
from
2485fa5 to
c7f46fb
Compare
Contributor
Confidence Score: 4/5Not safe to merge as-is due to a SQL injection guard being bypassed on the production code path. A P1 finding exists: the allowedKeyPairColumns whitelist in GetDistinctKeyPairs is only evaluated on the non-matview (SQLite / pre-startup) path; on postgres with matviews ready (the production hot path), column names are interpolated directly into SQL with no validation. All current callers use hardcoded strings so there is no immediate exploit, but the security guard is effectively dead code on the primary path and any future caller could bypass it. Score is 4/5 pending that fix. framework/logstore/rdb.go (GetDistinctKeyPairs) and framework/logstore/matviews.go (getDistinctKeyPairsFromMatView) Important Files Changed
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@framework/logstore/matviews.go`:
- Around line 733-746: The code currently interpolates the variable dimension
directly into SQL via dimCol (used in the SELECT and GROUP BY in the matview
queries built around mv_logs_hourly), which is unsafe; update the logic that
builds dimCol (and any similar uses around
applyMatViewFilters/GetDistinctKeyPairs) to first map the HistogramDimension
enum values to a fixed set of allowed column names (e.g., a switch or map from
HistogramDimension -> "service", "region", etc.) and return an error for any
unknown/unsupported enum, then use that whitelisted column name in the
fmt.Sprintf calls; ensure all occurrences that turn dimension into a raw
identifier (the dimCol usage in the SELECT/GROUP BY blocks) adopt this whitelist
check.
- Around line 74-81: The governance dropdown columns (routing_engines_used,
user_id, team_id, team_name, customer_id, customer_name, business_unit_id,
business_unit_name) are never populated for rows filtered out by the existing
"model is non-empty" predicate; adjust the matview selection so
governance-tagged rows are not excluded by the model check. Locate the
materialized view or function that builds these COALESCE columns (the code
around the COALESCE(...) list) and either remove/relax the predicate that
excludes empty/null model values (e.g., allow model = ''/NULL) or split the
governance fields into a separate subquery/join that does not apply the model !=
'' filter so governance-tagged logs without a model still surface in the
dropdowns.
In `@framework/logstore/migrations.go`:
- Around line 2277-2305: The migration function
migrationRecreateMatViewsWithGovernanceColumns drops matviews synchronously but
ensureMatViews (called from newPostgresLogStore) recreates them asynchronously,
causing a race where canUseMatView may allow a query that then fails with
"relation does not exist"; fix by adding explicit synchronization so queries
never attempt matviews until ensureMatViews completes (or add automatic fallback
on missing-relation errors). Specifically, introduce a shared readiness signal
(e.g., a sync.Once/WaitGroup or an atomic/boolean like matViewsReady with proper
locking) that ensureMatViews sets when finished and have canUseMatView and any
query entrypoints check this readiness before using matviews (or alternatively
catch "relation does not exist" errors in query methods and transparently retry
against raw tables); update newPostgresLogStore to initialize and wait on this
signal as needed to guarantee safe behavior.
In `@framework/logstore/rdb.go`:
- Around line 1972-1998: The code currently interpolates the variable
`dimension` (used to build `dimCol`) directly into SQL which allows identifier
injection; instead, implement a local allowlist mapping valid HistogramDimension
values to safe column identifiers and replace the raw use of `dimCol` with the
looked-up, pre-validated column name (returning an error if the provided
`dimension` is not in the allowlist). Update the logic around
`dimCol`/`dimension` in the `rdb.go` methods that build queries (the block using
`baseQuery.Select(... %s AS bucket_timestamp, COALESCE(%s, '') AS dim_value,
...` and the other occurrences referenced) to use the validated column name, and
ensure the function returns an error early when the dimension is invalid so no
SQL is constructed with unvalidated input.
- Around line 2020-2032: The code assumes generateBucketTimestamps(...) always
returns timestamps, so when it returns nil (no start_time/end_time) the loop
never materializes buckets and the grouped query results are lost; update the
block that builds buckets (using DimensionCostHistogramBucket, grouped,
bucketSizeSeconds, dimension, dimSet and sortedStringKeys) to fall back to
constructing buckets from the keys in grouped when allTimestamps is nil/empty
(e.g., iterate grouped map keys, convert to time.Unix(...).UTC(), set TotalCost
and ByDimension from grouped[ts]) so the response shape matches the histogram
endpoints; apply the same fallback pattern to the other similar handlers (the
token and latency variants that use generateBucketTimestamps and grouped).
- Around line 2126-2195: GetDimensionLatencyHistogram’s raw-SQL path only
selects AVG(latency) and COUNT(*) so percentile fields (p90/p95/p99) remain
zero; update the aggregation to calculate percentiles like the other
non-dimension latency helpers and populate DimensionLatencyStats accordingly.
Concretely, change the SELECT in GetDimensionLatencyHistogram to include
percentile expressions (e.g. for Postgres use percentile_cont(0.90) WITHIN GROUP
(ORDER BY latency) AS p90_lat, percentile_cont(0.95) AS p95_lat,
percentile_cont(0.99) AS p99_lat or use the same fallback logic used elsewhere
for SQLite/other dialects), extend the anonymous result struct to capture those
columns, and set the Percentile fields on DimensionLatencyStats when building
grouped/by-dimension results so DimensionLatencyHistogramResult returns correct
p90/p95/p99 values.
In `@plugins/logging/main.go`:
- Around line 667-673: The user_id is being read from the wrong context key:
change the call that sets userID in plugins/logging/main.go to use
schemas.BifrostContextKeyGovernanceUserID (matching the governance producer)
instead of schemas.BifrostContextKeyUserID so the governance flow populates
userID correctly; specifically update the bifrost.GetStringFromContext(ctx, ...)
invocation that assigns userID to reference BifrostContextKeyGovernanceUserID to
match the other governance keys (e.g., teamID, customerID, businessUnitID).
In `@plugins/logging/operations.go`:
- Around line 845-852: GetAvailableTeams and the other governance lookup callers
call store.GetDistinctKeyPairs with governance columns (e.g.,
"team_id","team_name") but the raw-store implementation only supports the
existing key/routing columns, causing errors and empty slices; update the store
layer so GetDistinctKeyPairs (or a new whitelist-aware variant) accepts and
handles these governance column names in the raw query path (or maps them to the
matview-safe equivalents), and then keep the callers (GetAvailableTeams, the
similar functions that call GetDistinctKeyPairs and keyPairResultsToKeyPairs)
unchanged; ensure you add unit tests for the raw-store path to cover
"team_id"/"team_name" (and the other governance column pairs mentioned) so the
raw query returns correct results instead of degrading to empty slices.
🪄 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: 9a7b0cfd-ddee-4f66-8f5b-5ab46e2fa40f
📒 Files selected for processing (11)
core/schemas/bifrost.goframework/logstore/matviews.goframework/logstore/migrations.goframework/logstore/rdb.goframework/logstore/store.goframework/logstore/tables.goplugins/logging/main.goplugins/logging/operations.goplugins/logging/utils.goplugins/logging/writer.gotransports/bifrost-http/handlers/logging.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
plugins/logging/writer.go (1)
349-359: Consider replacing this positional argument list with a typed struct.This API now takes a long run of same-typed strings, so an ID/name mix-up will still compile. A small parameter struct would make the call at Line 743 in
plugins/logging/main.gosafer to extend.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/logging/writer.go` around lines 349 - 359, Replace the long positional argument list of applyOutputFieldsToEntry with a single typed parameter: define a small struct (e.g., OutputFieldsParams) that groups related fields (preferably nested small structs for ID/Name pairs like Key{ID, Name}, Team{ID, Name}, Customer{ID, Name}, etc.) plus the numeric fields (NumberOfRetries, Latency); change the signature of applyOutputFieldsToEntry to accept this struct and update all callers (including the caller in plugins/logging/main.go) to construct and pass the struct, ensuring existing field names make the callsites safer and easier to extend without ID/name mixups.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@framework/logstore/migrations.go`:
- Around line 2277-2293: The migration
migrationRecreateMatViewsWithGovernanceColumns currently drops mv_logs_hourly
and mv_logs_filterdata before their replacements exist, which can create a
downtime window; instead, modify this migration to recreate/swap the views
atomically within the same migration by: for each view (mv_logs_hourly,
mv_logs_filterdata) create the new materialized view under a temporary name
(e.g., mv_logs_hourly_new) using the new definition (or call ensureMatViews
logic to build it WITH NO DATA if appropriate), then DROP the old view and
immediately RENAME the new temp view to the canonical name in the same
transaction, and finally drop any leftover temp names — ensure this uses the
same tx (tx.WithContext(ctx)) so old pods never observe a missing view and
reference ensureMatViews if it contains the new view SQL.
In `@framework/logstore/rdb.go`:
- Around line 1972-1999: The code currently casts the HistogramDimension
(dimension) to a string (dimCol) and directly interpolates it into raw SQL in
Select, COALESCE and Group clauses (seen where dimCol, bucketExpr and
baseQuery.Select / Group are used), which allows SQL injection; fix this by
implementing a closed allowlist (map or switch) of permitted dimension
identifiers to their exact column names or safe expressions and validate the
incoming dimension against it before any interpolation, return an error if not
allowed, and use the validated mapped column name (not the raw input) in the
Select/COALESCE/Group statements and in the matview path branches referenced
around the same areas (the other blocks at lines noted in the review) so all
uses of dimension only ever use trusted, canonical column names.
- Around line 2020-2032: generateBucketTimestamps returns nil for open-ended
ranges, so the loop building buckets from allTimestamps produces an empty
Buckets slice; add a fallback when len(allTimestamps)==0 to build timestamps
from the query result keys (the grouped map) instead of returning no buckets.
Specifically, in the routine that builds DimensionCostHistogramBucket
(referencing allTimestamps, grouped, DimensionCostHistogramBucket, and
DimensionCostHistogramResult) detect when allTimestamps is empty and construct a
timestamp slice from the timestamps present in grouped (sort them), then proceed
to populate buckets as before; apply the same len(allTimestamps)==0 fallback
logic to the analogous matview helper functions in
framework/logstore/matviews.go so open-ended or omitted ranges return the rows
produced by the query rather than an empty slice.
- Around line 1979-1985: The three histogram functions
GetDimensionCostHistogram, GetDimensionTokenHistogram, and
GetDimensionLatencyHistogram currently only handle "sqlite" and default
(Postgres) SQL for bucketExpr; add an explicit "mysql" case in each function's
switch on dialect to set bucketExpr using UNIX_TIMESTAMP(timestamp) (e.g.,
fmt.Sprintf("CAST(FLOOR(UNIX_TIMESTAMP(timestamp) / %d) * %d AS SIGNED)",
bucketSizeSeconds, bucketSizeSeconds) or equivalent) so MySQL uses
UNIX_TIMESTAMP instead of EXTRACT(EPOCH FROM ...); update the switch in each
function (the bucketExpr assignment block) to include that mysql branch
mirroring other histogram functions in the file.
- Around line 2123-2196: GetDimensionLatencyHistogram currently only selects
AVG(latency) for non-Postgres dialects, so P90/P95/P99 in DimensionLatencyStats
remain zero; mirror the approach used in
GetLatencyHistogram/GetProviderLatencyHistogram by querying raw latency values
per (bucket, dimension) instead of AVG, then compute percentiles in Go with the
existing computePercentile function and populate P90Latency, P95Latency,
P99Latency along with AvgLatency and TotalRequests. Locate the non-Postgres
branch in GetDimensionLatencyHistogram, change the SELECT to retrieve latency
rows (or SELECT array_agg/JSON aggregation where supported) grouped by bucket
and dimension, iterate the raw latencies into the grouped[bucket].byDimension
buckets, call computePercentile for .P90Latency/.P95Latency/.P99Latency and also
compute AvgLatency and TotalRequests, and keep the existing use of
generateBucketTimestamps and sortedStringKeys.
In `@framework/logstore/tables.go`:
- Around line 102-108: Search results are missing the new governance fields
because listSelectColumns() in framework/logstore/rdb.go projects a fixed set of
columns; update that projection to include the new Log model fields (UserID,
TeamID, TeamName, CustomerID, CustomerName, BusinessUnitID, BusinessUnitName) so
SearchLogs() returns them (GetLog already populates them for single fetches).
Modify listSelectColumns() to add the corresponding column names used by the Log
struct so list/search queries include these fields in their SELECT clause.
In `@plugins/logging/main.go`:
- Around line 672-673: businessUnitID and businessUnitName are read via
bifrost.GetStringFromContext using
schemas.BifrostContextKeyGovernanceBusinessUnitID and
schemas.BifrostContextKeyGovernanceBusinessUnitName but those keys are never
written in the request pipeline where the team/customer context is set, so
populate those keys at the same point that the team/customer values are injected
(the same place that currently writes the team/customer context values), i.e.,
add setting of context values for
schemas.BifrostContextKeyGovernanceBusinessUnitID and
schemas.BifrostContextKeyGovernanceBusinessUnitName before this logging code
runs so businessUnitID/businessUnitName read here are non-empty.
In `@plugins/logging/operations.go`:
- Around line 845-884: GetAvailableTeams, GetAvailableCustomers,
GetAvailableUsers and GetAvailableBusinessUnits call p.store.GetDistinctKeyPairs
with new columns (team_id, team_name, customer_id, customer_name, user_id,
business_unit_id, business_unit_name) but the whitelist in
framework/logstore/rdb.go (allowedKeyPairColumns) still only allows the old
selected/virtual/routing columns, causing failures on SQLite/MySQL; update
allowedKeyPairColumns in framework/logstore/rdb.go to include these new column
names so GetDistinctKeyPairs will accept them and return the expected results
for the functions GetAvailableTeams, GetAvailableCustomers, GetAvailableUsers
and GetAvailableBusinessUnits.
---
Nitpick comments:
In `@plugins/logging/writer.go`:
- Around line 349-359: Replace the long positional argument list of
applyOutputFieldsToEntry with a single typed parameter: define a small struct
(e.g., OutputFieldsParams) that groups related fields (preferably nested small
structs for ID/Name pairs like Key{ID, Name}, Team{ID, Name}, Customer{ID,
Name}, etc.) plus the numeric fields (NumberOfRetries, Latency); change the
signature of applyOutputFieldsToEntry to accept this struct and update all
callers (including the caller in plugins/logging/main.go) to construct and pass
the struct, ensuring existing field names make the callsites safer and easier to
extend without ID/name mixups.
🪄 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: 9682fdbd-4b34-4640-b065-668c3db47e0b
📒 Files selected for processing (11)
core/schemas/bifrost.goframework/logstore/matviews.goframework/logstore/migrations.goframework/logstore/rdb.goframework/logstore/store.goframework/logstore/tables.goplugins/logging/main.goplugins/logging/operations.goplugins/logging/utils.goplugins/logging/writer.gotransports/bifrost-http/handlers/logging.go
akshaydeo
force-pushed
the
04-06-adds_support_for_tracking_userid_teamid_customerid_and_businessunitid_in_logging
branch
from
c7f46fb to
20457bc
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
framework/logstore/rdb.go (3)
2020-2032:⚠️ Potential issue | 🟠 MajorBuild buckets from query rows when the time range is omitted.
generateBucketTimestamps()returnsnilunless bothstart_timeandend_timeare set, so these methods currently drop every grouped row and returnbuckets: []for open-ended queries. The non-dimension histograms in this file already fall back to query-derived timestamps; these variants need the same fallback.Also applies to: 2103-2120, 2184-2195
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/logstore/rdb.go` around lines 2020 - 2032, generateBucketTimestamps(...) can return nil for open-ended queries causing buckets to be empty; update the bucket construction in the DimensionCostHistogram code path to fall back to using timestamps derived from the query rows when generateBucketTimestamps returns nil. Concretely, after calling generateBucketTimestamps(filters.StartTime, filters.EndTime, bucketSizeSeconds) check for nil and if so build allTimestamps from the keys of grouped (the map used later), sort them as needed, then proceed to create DimensionCostHistogramBucket objects (using DimensionCostHistogramBucket, grouped, bucketSizeSeconds, dimension, sortedStringKeys, dimSet) and return the DimensionCostHistogramResult as before; apply the same fallback fix to the other affected blocks referenced around the other ranges.
2123-2196:⚠️ Potential issue | 🟠 MajorDon't zero out p90/p95/p99 on the raw dimension-latency path.
When the matview fast path is unavailable, this implementation only computes
AVG(latency)andCOUNT(*), so the percentile fields inDimensionLatencyStatsstay zero. That breaks the latency histogram contract and diverges from the existing raw latency helpers in this file, which already compute percentiles for SQLite/MySQL and viapercentile_contfor PostgreSQL.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/logstore/rdb.go` around lines 2123 - 2196, GetDimensionLatencyHistogram's fallback path (in GetDimensionLatencyHistogram) only selects AVG(latency) and COUNT(*) so DimensionLatencyStats' percentile fields (p90/p95/p99) stay zero; update the fallback to compute percentiles like the other raw helpers: detect dialect (sqlite/mysql vs postgres) and add percentile expressions to the Select (or run a secondary query) so p90/p95/p99 are populated into the anonymous results struct before mapping into DimensionLatencyStats; mirror the percentile SQL used by getDimensionLatencyHistogramFromMatView or the existing raw latency helpers and ensure the result fields map to DimensionLatencyStats.p90/p95/p99 rather than leaving them at zero.
1972-1999:⚠️ Potential issue | 🔴 CriticalValidate
dimensionbefore splicing it into SQL.
HistogramDimensionis still just a string alias, and these methods cast it straight back intoSELECT/COALESCE/GROUP BY. The HTTP handler validates today, but the store contract is public, so one unchecked caller turns this into identifier injection or broken SQL. Resolve the dimension through a closed allowlist at the store boundary and use the mapped column name everywhere.🔒 Suggested shape
+func histogramDimensionColumn(d HistogramDimension) (string, error) { + switch d { + case DimensionProvider: + return "provider", nil + case DimensionTeam: + return "team_id", nil + case DimensionCustomer: + return "customer_id", nil + case DimensionUser: + return "user_id", nil + case DimensionBusinessUnit: + return "business_unit_id", nil + default: + return "", fmt.Errorf("invalid histogram dimension: %s", d) + } +} + func (s *RDBLogStore) GetDimensionCostHistogram(ctx context.Context, filters SearchFilters, bucketSizeSeconds int64, dimension HistogramDimension) (*DimensionCostHistogramResult, error) { @@ - dimCol := string(dimension) + dimCol, err := histogramDimensionColumn(dimension) + if err != nil { + return nil, err + }Also applies to: 2044-2075, 2133-2163
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/logstore/rdb.go` around lines 1972 - 1999, The code blindly injects the `dimension` string into SQL (used via `dimCol`) which allows identifier injection or broken queries; replace this by resolving `dimension` against a closed allowlist map (e.g., map[HistogramDimension]string) at the start of the public store method and return an error if not allowed, then use the resolved column name variable (not the raw `dimension`) everywhere you currently reference `dimCol` (in the SELECT/COALESCE, GROUP BY and any ORDER/GROUP expressions built on `dimCol` such as in the code around the `baseQuery.Select(...).Group(...).Order(...)` and the other similar blocks noted) so only validated, pre-mapped column identifiers are spliced into SQL.plugins/logging/operations.go (1)
845-885:⚠️ Potential issue | 🟠 MajorThese governance dropdown lookups still don't have a working store path.
framework/logstore/rdb.gostill only allowlists the legacy selected/virtual/routing columns for rawGetDistinctKeyPairs(), so SQLite rejects all four of these calls. On PostgreSQL,GetDistinctKeyPairs()routes through the matview path, but the providedmv_logs_hourlydefinition only exposesteam_id,customer_id,user_id, andbusiness_unit_id, notteam_name,customer_name, orbusiness_unit_name. As written, these methods collapse to logged errors and empty filter options instead of returning the new governance data.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/logging/operations.go` around lines 845 - 885, GetAvailableTeams/GetAvailableCustomers/GetAvailableUsers/GetAvailableBusinessUnits call p.store.GetDistinctKeyPairs with name columns that aren't exposed by the current store/matview path, so these lookups fail in SQLite (allowlist in framework/logstore/rdb.go) and on Postgres (mv_logs_hourly lacks the *_name columns). Fix by either (A) extending the rdb store allowlist in framework/logstore/rdb.go to permit "team_name", "customer_name", "business_unit_name" for GetDistinctKeyPairs so SQLite queries succeed, and/or (B) update the materialized view definition mv_logs_hourly to expose the corresponding name columns (team_name, customer_name, business_unit_name) so the Postgres matview path returns them; ensure GetDistinctKeyPairs and the keyPairResultsToKeyPairs consumers match the final column names.
🧹 Nitpick comments (3)
ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx (1)
534-535: Avoid unconditional dismissal blocking on outside click and Escape.Line 534 and Line 535 always call
preventDefault(), so the sheet cannot be dismissed via normal dialog affordances even when nothing is dirty.ui/app/workspace/model-limits/views/modelLimitSheet.tsx(Line 213-Line 218) already uses a safer conditional guard pattern.Suggested adjustment
<SheetContent className="flex w-full flex-col overflow-x-hidden px-4 pb-8" data-testid="vk-sheet" - onInteractOutside={(e) => e.preventDefault()} - onEscapeKeyDown={(e) => e.preventDefault()} + onInteractOutside={(e) => { + if (form.formState.isDirty || isLoading) e.preventDefault(); + }} + onEscapeKeyDown={(e) => { + if (form.formState.isDirty || isLoading) e.preventDefault(); + }} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx` around lines 534 - 535, The onInteractOutside and onEscapeKeyDown handlers in virtualKeySheet.tsx currently call e.preventDefault() unconditionally, preventing normal dismissal; change them to only call e.preventDefault() when there are unsaved changes — mirror the safer pattern used in modelLimitSheet.tsx by checking the component's dirty flag (e.g., isDirty / hasUnsavedChanges / a shouldPreventDismiss predicate) inside the handlers for onInteractOutside and onEscapeKeyDown so normal dismissal works when nothing is dirty and prevention still occurs when there are pending changes.ui/app/workspace/logs/sheets/logDetailsSheet.tsx (1)
313-318: Render governance fields with*_idfallback when*_nameis missing.On Line 318 (and similarly for team/customer/business unit), the field is hidden unless a name exists. If only IDs are populated, governance context disappears in the details view.
💡 Suggested fallback rendering
- {displayLog.team_name && <LogEntryDetailsView className="w-full" label="Team" value={displayLog.team_name} />} - {displayLog.customer_name && <LogEntryDetailsView className="w-full" label="Customer" value={displayLog.customer_name} />} - {displayLog.business_unit_name && ( - <LogEntryDetailsView className="w-full" label="Business Unit" value={displayLog.business_unit_name} /> - )} - {displayLog.user_name && <LogEntryDetailsView className="w-full" label="User" value={displayLog.user_name} />} + {(displayLog.team_name || displayLog.team_id) && ( + <LogEntryDetailsView className="w-full" label="Team" value={displayLog.team_name ?? displayLog.team_id} /> + )} + {(displayLog.customer_name || displayLog.customer_id) && ( + <LogEntryDetailsView + className="w-full" + label="Customer" + value={displayLog.customer_name ?? displayLog.customer_id} + /> + )} + {(displayLog.business_unit_name || displayLog.business_unit_id) && ( + <LogEntryDetailsView + className="w-full" + label="Business Unit" + value={displayLog.business_unit_name ?? displayLog.business_unit_id} + /> + )} + {(displayLog.user_name || displayLog.user_id) && ( + <LogEntryDetailsView className="w-full" label="User" value={displayLog.user_name ?? displayLog.user_id} /> + )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/logs/sheets/logDetailsSheet.tsx` around lines 313 - 318, The details view currently only renders governance fields when displayLog.team_name / customer_name / business_unit_name / user_name exist; update the rendering logic in logDetailsSheet.tsx (where LogEntryDetailsView is used) to fall back to the corresponding ID properties (team_id, customer_id, business_unit_id, user_id) when the name is missing—i.e., for each field (team, customer, business unit, user) render <LogEntryDetailsView label="X" value={displayLog.X_name ?? displayLog.X_id} /> and keep the conditional only if either name or id exists so the governance context still shows when only IDs are present.plugins/governance/main.go (1)
338-338: Usebifrost.Ptr()to create the pointer for consistency with repository patterns.This function uses
new()directly; prefer the repository'sbifrost.Ptr()helper for pointer creation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/governance/main.go` at line 338, Replace the direct new() pointer creation with the repository helper: instead of assigning p.isVkMandatory = new(enforceAuthOnInference), use bifrost.Ptr(...) to construct the pointer (referencing p.isVkMandatory and the enforceAuthOnInference type) so the code follows the project pattern; import/use bifrost.Ptr where necessary.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugins/governance/main.go`:
- Around line 394-404: The current block stores pointer values and dereferences
nested structs without nil checks; update both Team and Customer sections (where
virtualKey, virtualKey.TeamID, virtualKey.CustomerID, ctx.SetValue and the
schemas.BifrostContextKeyGovernance* keys are used) to mirror the pattern in
resolver.go: check for non-nil ID pointers and set ctx values to the
dereferenced string IDs (not pointer types), and also guard virtualKey.Team and
virtualKey.Customer for nil before reading Name and set the Name as a string
only when the nested struct is non-nil.
In `@ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx`:
- Line 533: The data-testid on the new sheet element is 2-part ("vk-sheet") and
must be changed to a 3-part selector to follow the repo convention; locate the
JSX in virtualKeySheet.tsx where data-testid="vk-sheet" is set (inside the
VirtualKeySheet / sheet element) and rename it to a three-part value such as
"vk-sheet-content" or "virtualKey-sheet-content" (pattern:
<entity>-<element>-<qualifier>), then update any tests or references that expect
the old "vk-sheet" id.
In `@ui/lib/types/logs.ts`:
- Around line 454-461: The type field user_name in ui/lib/types/logs.ts is
unused by the backend and prevents UI user context from rendering; either remove
user_name from the Logs type and update any UI checks (e.g., the conditional in
logDetailsSheet.tsx that uses displayLog.user_name) to rely on user_id (or a
derived display label), or modify the backend to emit a user_name (set it to
user_id as described in plugins/logging/operations.go) by updating the log
emission code so the emitted payload includes user_name (see
framework/logstore/tables.go and migrations.go for schema constraints); pick one
approach and make consistent changes to the Logs type, logDetailsSheet.tsx
references, and the backend emitter (plugins/logging/operations.go) so the UI
conditional can render correctly.
---
Duplicate comments:
In `@framework/logstore/rdb.go`:
- Around line 2020-2032: generateBucketTimestamps(...) can return nil for
open-ended queries causing buckets to be empty; update the bucket construction
in the DimensionCostHistogram code path to fall back to using timestamps derived
from the query rows when generateBucketTimestamps returns nil. Concretely, after
calling generateBucketTimestamps(filters.StartTime, filters.EndTime,
bucketSizeSeconds) check for nil and if so build allTimestamps from the keys of
grouped (the map used later), sort them as needed, then proceed to create
DimensionCostHistogramBucket objects (using DimensionCostHistogramBucket,
grouped, bucketSizeSeconds, dimension, sortedStringKeys, dimSet) and return the
DimensionCostHistogramResult as before; apply the same fallback fix to the other
affected blocks referenced around the other ranges.
- Around line 2123-2196: GetDimensionLatencyHistogram's fallback path (in
GetDimensionLatencyHistogram) only selects AVG(latency) and COUNT(*) so
DimensionLatencyStats' percentile fields (p90/p95/p99) stay zero; update the
fallback to compute percentiles like the other raw helpers: detect dialect
(sqlite/mysql vs postgres) and add percentile expressions to the Select (or run
a secondary query) so p90/p95/p99 are populated into the anonymous results
struct before mapping into DimensionLatencyStats; mirror the percentile SQL used
by getDimensionLatencyHistogramFromMatView or the existing raw latency helpers
and ensure the result fields map to DimensionLatencyStats.p90/p95/p99 rather
than leaving them at zero.
- Around line 1972-1999: The code blindly injects the `dimension` string into
SQL (used via `dimCol`) which allows identifier injection or broken queries;
replace this by resolving `dimension` against a closed allowlist map (e.g.,
map[HistogramDimension]string) at the start of the public store method and
return an error if not allowed, then use the resolved column name variable (not
the raw `dimension`) everywhere you currently reference `dimCol` (in the
SELECT/COALESCE, GROUP BY and any ORDER/GROUP expressions built on `dimCol` such
as in the code around the `baseQuery.Select(...).Group(...).Order(...)` and the
other similar blocks noted) so only validated, pre-mapped column identifiers are
spliced into SQL.
In `@plugins/logging/operations.go`:
- Around line 845-885:
GetAvailableTeams/GetAvailableCustomers/GetAvailableUsers/GetAvailableBusinessUnits
call p.store.GetDistinctKeyPairs with name columns that aren't exposed by the
current store/matview path, so these lookups fail in SQLite (allowlist in
framework/logstore/rdb.go) and on Postgres (mv_logs_hourly lacks the *_name
columns). Fix by either (A) extending the rdb store allowlist in
framework/logstore/rdb.go to permit "team_name", "customer_name",
"business_unit_name" for GetDistinctKeyPairs so SQLite queries succeed, and/or
(B) update the materialized view definition mv_logs_hourly to expose the
corresponding name columns (team_name, customer_name, business_unit_name) so the
Postgres matview path returns them; ensure GetDistinctKeyPairs and the
keyPairResultsToKeyPairs consumers match the final column names.
---
Nitpick comments:
In `@plugins/governance/main.go`:
- Line 338: Replace the direct new() pointer creation with the repository
helper: instead of assigning p.isVkMandatory = new(enforceAuthOnInference), use
bifrost.Ptr(...) to construct the pointer (referencing p.isVkMandatory and the
enforceAuthOnInference type) so the code follows the project pattern; import/use
bifrost.Ptr where necessary.
In `@ui/app/workspace/logs/sheets/logDetailsSheet.tsx`:
- Around line 313-318: The details view currently only renders governance fields
when displayLog.team_name / customer_name / business_unit_name / user_name
exist; update the rendering logic in logDetailsSheet.tsx (where
LogEntryDetailsView is used) to fall back to the corresponding ID properties
(team_id, customer_id, business_unit_id, user_id) when the name is missing—i.e.,
for each field (team, customer, business unit, user) render <LogEntryDetailsView
label="X" value={displayLog.X_name ?? displayLog.X_id} /> and keep the
conditional only if either name or id exists so the governance context still
shows when only IDs are present.
In `@ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx`:
- Around line 534-535: The onInteractOutside and onEscapeKeyDown handlers in
virtualKeySheet.tsx currently call e.preventDefault() unconditionally,
preventing normal dismissal; change them to only call e.preventDefault() when
there are unsaved changes — mirror the safer pattern used in modelLimitSheet.tsx
by checking the component's dirty flag (e.g., isDirty / hasUnsavedChanges / a
shouldPreventDismiss predicate) inside the handlers for onInteractOutside and
onEscapeKeyDown so normal dismissal works when nothing is dirty and prevention
still occurs when there are pending changes.
🪄 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: b32e5ec9-5c8a-408e-a4ca-b5d63a12d9e4
📒 Files selected for processing (16)
core/schemas/bifrost.goframework/logstore/matviews.goframework/logstore/migrations.goframework/logstore/rdb.goframework/logstore/store.goframework/logstore/tables.goplugins/governance/main.goplugins/governance/store.goplugins/logging/main.goplugins/logging/operations.goplugins/logging/utils.goplugins/logging/writer.gotransports/bifrost-http/handlers/logging.goui/app/workspace/logs/sheets/logDetailsSheet.tsxui/app/workspace/virtual-keys/views/virtualKeySheet.tsxui/lib/types/logs.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- plugins/logging/writer.go
- framework/logstore/store.go
- plugins/logging/main.go
- framework/logstore/tables.go
- framework/logstore/matviews.go
- core/schemas/bifrost.go
- plugins/logging/utils.go
- framework/logstore/migrations.go
akshaydeo
force-pushed
the
04-06-adds_support_for_tracking_userid_teamid_customerid_and_businessunitid_in_logging
branch
from
20457bc to
55503d5
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
plugins/governance/main.go (1)
338-338: Consider usingbifrost.Ptr()for consistency with codebase patterns.In Go 1.26,
new(expr)initializes the allocated value toexpr's value, so the current codep.isVkMandatory = new(enforceAuthOnInference)correctly creates a pointer to theenforceAuthOnInferencevalue. However,bifrost.Ptr(enforceAuthOnInference)is clearer and aligns with existing codebase style (seecore/utils.go).Suggested refactor
func (p *GovernancePlugin) UpdateEnforceAuthOnInference(enforceAuthOnInference bool) { p.cfgMutex.Lock() defer p.cfgMutex.Unlock() - p.isVkMandatory = new(enforceAuthOnInference) + p.isVkMandatory = bifrost.Ptr(enforceAuthOnInference) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/governance/main.go` at line 338, Replace the use of new(enforceAuthOnInference) with bifrost.Ptr(enforceAuthOnInference) to match codebase conventions; update the assignment to p.isVkMandatory to call bifrost.Ptr(enforceAuthOnInference), ensure the bifrost package is imported where p.isVkMandatory is set, and confirm the pointer type of p.isVkMandatory matches the return type of bifrost.Ptr.plugins/logging/writer.go (1)
349-357: Reduce positional-argument fragility inapplyOutputFieldsToEntry.This signature now has many adjacent
stringarguments, which makes call-site swaps easy to miss. Consider passing a typed context struct instead.♻️ Suggested refactor shape
+type OutputGovernanceContext struct { + SelectedKeyID string + SelectedKeyName string + VirtualKeyID string + VirtualKeyName string + RoutingRuleID string + RoutingRuleName string + TeamID string + TeamName string + CustomerID string + CustomerName string + UserID string + BusinessUnitID string + BusinessUnitName string +} + func applyOutputFieldsToEntry( entry *logstore.Log, - selectedKeyID, selectedKeyName string, - virtualKeyID, virtualKeyName string, - routingRuleID, routingRuleName string, - teamID, teamName string, - customerID, customerName string, - userID string, - businessUnitID, businessUnitName string, + gov OutputGovernanceContext, numberOfRetries int, latency int64, ) { - entry.SelectedKeyID = selectedKeyID - entry.SelectedKeyName = selectedKeyName + entry.SelectedKeyID = gov.SelectedKeyID + entry.SelectedKeyName = gov.SelectedKeyName // ... map remaining fields from gov ... }Also applies to: 375-395
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/logging/writer.go` around lines 349 - 357, The function applyOutputFieldsToEntry has many adjacent string parameters (selectedKeyID, selectedKeyName, virtualKeyID, virtualKeyName, routingRuleID, routingRuleName, teamID, teamName, customerID, customerName, userID, businessUnitID, businessUnitName) which is fragile; define a typed struct (e.g., OutputFieldsContext or ApplyOutputFieldsOpts) containing those named fields, replace the long parameter list on applyOutputFieldsToEntry with a single struct parameter, and update all call sites (including the similar usages around the other related function blocks) to construct and pass that struct so argument order errors are eliminated and the code is clearer.transports/bifrost-http/handlers/logging.go (1)
943-943: Consider splitting the longSendJSONcall for readability.The map literal spans a single line with 11 fields, making it harder to scan. Consider formatting it as a multi-line struct.
♻️ Suggested refactor
- SendJSON(ctx, map[string]interface{}{"models": models, "aliases": aliases, "selected_keys": selectedKeysArray, "virtual_keys": virtualKeysArray, "routing_rules": routingRulesArray, "routing_engines": routingEngines, "teams": teams, "customers": customers, "users": users, "business_units": businessUnits, "metadata_keys": metadataKeys}) + SendJSON(ctx, map[string]interface{}{ + "models": models, + "aliases": aliases, + "selected_keys": selectedKeysArray, + "virtual_keys": virtualKeysArray, + "routing_rules": routingRulesArray, + "routing_engines": routingEngines, + "teams": teams, + "customers": customers, + "users": users, + "business_units": businessUnits, + "metadata_keys": metadataKeys, + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/handlers/logging.go` at line 943, The SendJSON call currently passes a single-line map literal with many keys which is hard to read; refactor the call to format the map across multiple lines so each key/value pair is on its own line (e.g., break up the argument to SendJSON in logging.go so the map starts on its own line and lists "models", "aliases", "selected_keys", "virtual_keys", "routing_rules", "routing_engines", "teams", "customers", "users", "business_units", and "metadata_keys" each on separate lines), keeping the call to SendJSON(ctx, ...) intact and preserving the same variable names and ordering.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@plugins/governance/main.go`:
- Line 338: Replace the use of new(enforceAuthOnInference) with
bifrost.Ptr(enforceAuthOnInference) to match codebase conventions; update the
assignment to p.isVkMandatory to call bifrost.Ptr(enforceAuthOnInference),
ensure the bifrost package is imported where p.isVkMandatory is set, and confirm
the pointer type of p.isVkMandatory matches the return type of bifrost.Ptr.
In `@plugins/logging/writer.go`:
- Around line 349-357: The function applyOutputFieldsToEntry has many adjacent
string parameters (selectedKeyID, selectedKeyName, virtualKeyID, virtualKeyName,
routingRuleID, routingRuleName, teamID, teamName, customerID, customerName,
userID, businessUnitID, businessUnitName) which is fragile; define a typed
struct (e.g., OutputFieldsContext or ApplyOutputFieldsOpts) containing those
named fields, replace the long parameter list on applyOutputFieldsToEntry with a
single struct parameter, and update all call sites (including the similar usages
around the other related function blocks) to construct and pass that struct so
argument order errors are eliminated and the code is clearer.
In `@transports/bifrost-http/handlers/logging.go`:
- Line 943: The SendJSON call currently passes a single-line map literal with
many keys which is hard to read; refactor the call to format the map across
multiple lines so each key/value pair is on its own line (e.g., break up the
argument to SendJSON in logging.go so the map starts on its own line and lists
"models", "aliases", "selected_keys", "virtual_keys", "routing_rules",
"routing_engines", "teams", "customers", "users", "business_units", and
"metadata_keys" each on separate lines), keeping the call to SendJSON(ctx, ...)
intact and preserving the same variable names and ordering.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 022dab05-14ee-4632-84d3-865c512208ad
📒 Files selected for processing (18)
core/schemas/bifrost.goframework/configstore/tables/virtualkey.goframework/logstore/matviews.goframework/logstore/migrations.goframework/logstore/postgres.goframework/logstore/rdb.goframework/logstore/store.goframework/logstore/tables.goplugins/governance/main.goplugins/governance/store.goplugins/logging/main.goplugins/logging/operations.goplugins/logging/utils.goplugins/logging/writer.gotransports/bifrost-http/handlers/logging.goui/app/workspace/logs/sheets/logDetailsSheet.tsxui/app/workspace/virtual-keys/views/virtualKeySheet.tsxui/lib/types/logs.ts
✅ Files skipped from review due to trivial changes (1)
- core/schemas/bifrost.go
🚧 Files skipped from review as they are similar to previous changes (5)
- ui/app/workspace/logs/sheets/logDetailsSheet.tsx
- ui/lib/types/logs.ts
- plugins/governance/store.go
- plugins/logging/main.go
- framework/logstore/matviews.go
akshaydeo
force-pushed
the
04-06-adds_support_for_tracking_userid_teamid_customerid_and_businessunitid_in_logging
branch
from
55503d5 to
bef31d2
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx (1)
531-536: Consider showing a confirmation dialog instead of completely blocking dismissal.Preventing both click-outside and Escape key dismissal breaks standard modal interaction patterns. Users expect Escape to close overlays, and blocking it entirely may frustrate keyboard users.
A more accessible pattern for protecting unsaved changes:
♻️ Suggested approach
- onInteractOutside={(e) => e.preventDefault()} - onEscapeKeyDown={(e) => e.preventDefault()} + onInteractOutside={(e) => { + if (form.formState.isDirty) { + e.preventDefault(); + // Optionally show unsaved changes warning + } + }} + onEscapeKeyDown={(e) => { + if (form.formState.isDirty) { + e.preventDefault(); + // Optionally show unsaved changes warning + } + }}This allows clean forms to dismiss normally while protecting dirty forms. Alternatively, you could prompt users with a confirmation dialog before discarding changes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx` around lines 531 - 536, The SheetContent currently prevents all outside click and Escape dismissals via onInteractOutside and onEscapeKeyDown; change these handlers to only prevent dismissal when the form is dirty by checking the component's dirty state (e.g., isDirty) and otherwise allow the event to proceed so the sheet closes normally. When isDirty is true, intercept the event, preventDefault, and open a confirmation flow (e.g., setShowConfirm / openConfirmDialog and call handleConfirmDismiss on confirm to actually close the sheet); when isDirty is false, do not call preventDefault so default dismissal behavior works. Ensure you update the handlers named onInteractOutside and onEscapeKeyDown in virtualKeySheet.tsx and wire the confirmation dialog state/handler (setShowConfirm / handleConfirmDismiss) to perform the final close.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@framework/logstore/rdb.go`:
- Around line 1969-1975: The matview fast-path check occurs before normalizing
bucketSizeSeconds, causing callers that omit/zero the bucket to bypass matviews;
update GetDimensionCostHistogram to set bucketSizeSeconds = 3600 when <= 0
before evaluating s.db.Dialector.Name() and s.canUseMatView(...) so the matview
branch uses the effective bucket; apply the same reorder/fix to the other
histogram variants (e.g. GetDimensionLatencyHistogram and the token histogram
methods) so all code uses the normalized bucketSizeSeconds when deciding whether
to use the matview.
---
Nitpick comments:
In `@ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx`:
- Around line 531-536: The SheetContent currently prevents all outside click and
Escape dismissals via onInteractOutside and onEscapeKeyDown; change these
handlers to only prevent dismissal when the form is dirty by checking the
component's dirty state (e.g., isDirty) and otherwise allow the event to proceed
so the sheet closes normally. When isDirty is true, intercept the event,
preventDefault, and open a confirmation flow (e.g., setShowConfirm /
openConfirmDialog and call handleConfirmDismiss on confirm to actually close the
sheet); when isDirty is false, do not call preventDefault so default dismissal
behavior works. Ensure you update the handlers named onInteractOutside and
onEscapeKeyDown in virtualKeySheet.tsx and wire the confirmation dialog
state/handler (setShowConfirm / handleConfirmDismiss) to perform the final
close.
🪄 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: 4dbd64a4-97b6-4fd4-b00b-1a087ab09e11
📒 Files selected for processing (20)
core/schemas/bifrost.goframework/configstore/rdb.goframework/configstore/tables/virtualkey.goframework/logstore/matviews.goframework/logstore/migrations.goframework/logstore/postgres.goframework/logstore/rdb.goframework/logstore/store.goframework/logstore/tables.goplugins/governance/main.goplugins/governance/store.goplugins/logging/main.goplugins/logging/operations.goplugins/logging/utils.goplugins/logging/writer.gotransports/bifrost-http/handlers/logging.gotransports/bifrost-http/lib/config_test.goui/app/workspace/logs/sheets/logDetailsSheet.tsxui/app/workspace/virtual-keys/views/virtualKeySheet.tsxui/lib/types/logs.ts
✅ Files skipped from review due to trivial changes (3)
- framework/configstore/tables/virtualkey.go
- ui/app/workspace/logs/sheets/logDetailsSheet.tsx
- plugins/governance/store.go
🚧 Files skipped from review as they are similar to previous changes (9)
- framework/logstore/postgres.go
- framework/logstore/migrations.go
- ui/lib/types/logs.ts
- framework/logstore/store.go
- plugins/governance/main.go
- plugins/logging/main.go
- plugins/logging/operations.go
- transports/bifrost-http/handlers/logging.go
- framework/logstore/matviews.go
akshaydeo
force-pushed
the
04-06-adds_support_for_tracking_userid_teamid_customerid_and_businessunitid_in_logging
branch
from
bef31d2 to
2ad9415
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
framework/logstore/matviews.go (1)
174-185:⚠️ Potential issue | 🟠 MajorDon't route alias-filtered analytics through the matview.
Handlers still populate
SearchFilters.Aliases, butmv_logs_hourlydoes not materializealiasandapplyMatViewFilters()never constrains it. Because this helper still returnstruewhenAliasesis set, Postgres stats, histogram, ranking, and new by-dimension queries will silently ignore alias filtering.🛠️ Minimal fix
func canUseMatViewFilters(f SearchFilters) bool { - return f.ContentSearch == "" && + return len(f.Aliases) == 0 && + f.ContentSearch == "" && len(f.MetadataFilters) == 0 && len(f.RoutingEngineUsed) == 0 && f.MinLatency == nil && f.MaxLatency == nil && f.MinTokens == nil && f.MaxTokens == nil && f.MinCost == nil && f.MaxCost == nil && !f.MissingCostOnly }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/logstore/matviews.go` around lines 174 - 185, canUseMatViewFilters currently returns true even when SearchFilters.Aliases is set, but mv_logs_hourly doesn't materialize alias and applyMatViewFilters doesn't constrain it; update canUseMatViewFilters to also require no alias filtering by checking that SearchFilters.Aliases (or the appropriate alias field on SearchFilters) is empty/zero-length so the matview is only used when alias filters are not present.framework/logstore/rdb.go (1)
2356-2385:⚠️ Potential issue | 🟠 MajorMove allowlist validation and readiness gate before the Postgres fast path in
GetDistinctKeyPairs.
getDistinctKeyPairsFromMatViewusesfmt.Sprintf()to interpolateidColandnameColwithout any allowlist validation, and the Postgres branch returns before the localallowedKeyPairColumnschecks can run. Additionally, unlike all other matview query paths in this file (e.g., lines 414, 504, 564), there is nocanUseMatView()readiness gate before accessingmv_logs_filterdata, which can cause failures or stale reads during materialized view recreation.Suggested fix:
Move the allowlist and readiness checks before the Postgres early return, or add them inside
getDistinctKeyPairsFromMatView.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/logstore/rdb.go` around lines 2356 - 2385, The Postgres fast-path in GetDistinctKeyPairs returns to call getDistinctKeyPairsFromMatView without validating idCol/nameCol or checking canUseMatView, leaving fmt.Sprintf interpolation unguarded; move the allowlist checks against allowedKeyPairColumns and the canUseMatView readiness gate to run before the s.db.Dialector.Name() == "postgres" early return, or alternatively add those two checks at the start of getDistinctKeyPairsFromMatView so idCol/nameCol are validated and canUseMatView is enforced before any fmt.Sprintf-based query construction.
♻️ Duplicate comments (1)
framework/logstore/rdb.go (1)
1969-1975:⚠️ Potential issue | 🟠 MajorNormalize the default bucket before checking the matview path.
When callers omit
bucketSizeSeconds, these methods evaluate the>= 3600fast-path guard before they apply the3600default, so the new/by-dimensionendpoints fall back to raw SQL even though their effective bucket is hourly. In the latency variant that also drops the matview percentile path.Also applies to: 2061-2067, 2174-2180
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/logstore/rdb.go` around lines 1969 - 1975, The matview fast-path check runs before the default bucket is applied, so ensure bucketSizeSeconds is normalized to the default (3600) at the start of the method; in GetDimensionCostHistogram call (and the similar methods around the other reported ranges, e.g., the latency histogram variant), set bucketSizeSeconds = 3600 when <= 0 before evaluating s.db.Dialector.Name() == "postgres" && s.canUseMatView(...) && bucketSizeSeconds >= 3600, so that getDimensionCostHistogramFromMatView (and the latency matview path) are chosen correctly when callers omit the bucket.
🧹 Nitpick comments (2)
transports/bifrost-http/handlers/logging.go (1)
118-129: Extract the governance-ID parsing once.This exact
team_ids/customer_ids/user_ids/business_unit_idsblock now exists ingetLogs,getLogsStats, andparseHistogramFilters. Pulling it into one helper would keep list, stats, and histogram behavior from drifting the next time another governance filter is added.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/handlers/logging.go` around lines 118 - 129, Extract the repeated parsing logic into a single helper (e.g., create a function like applyGovernanceFilters(ctx *fasthttp.RequestCtx, filters *LogFilters) or parseGovernanceIDs(ctx, &filters.TeamIDs, &filters.CustomerIDs, &filters.UserIDs, &filters.BusinessUnitIDs)) and replace the duplicated blocks in getLogs, getLogsStats, and parseHistogramFilters with calls to that helper; the helper should read the same query params ("team_ids","customer_ids","user_ids","business_unit_ids"), call parseCommaSeparated for each non-empty value, and assign into the corresponding fields on the provided filters struct.plugins/logging/utils.go (1)
81-92: Make the user option contract explicit.Teams, customers, and business units have a natural
ID/Namepair in this PR, but users only adduser_id. Returning[]KeyPairforGetAvailableUsersis safe only if callers are guaranteed a usable label (Name = IDor equivalent); otherwise the user filter can render blank options while the other governance filters render names. Consider documenting or enforcing that invariant, or making this one method ID-only.Also applies to: 263-281
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/logging/utils.go` around lines 81 - 92, GetAvailableUsers currently returns []KeyPair but callers may get blank labels because user entries only populate user_id; update the contract to be explicit by either (A) changing GetAvailableUsers(ctx context.Context) to return []string or []UserID (ID-only) to reflect it only provides IDs, or (B) keep []KeyPair but ensure the implementation that builds GetAvailableUsers always sets KeyPair.Name = KeyPair.ID (or a formatted label) so the UI filter shows a usable label; update the function signature and any callers (or update the implementation that constructs KeyPair in GetAvailableUsers) and add a short comment on GetAvailableUsers to document the invariant.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@framework/logstore/matviews.go`:
- Around line 187-194: The code sets d.matViewsReady true even when
refreshMatViews fails, causing canUseMatView to allow queries against
empty/recreated views; update the logic in the postgres.go routine that
currently calls d.matViewsReady.Store(true) so it only stores true after
refreshMatViews(...) returns success (no error) — move the Store(true) into the
success branch after refreshMatViews completes (or add an explicit check of the
returned error), leaving matViewsReady false on failure so canUseMatView
continues to block matview usage until a successful refresh.
In `@plugins/logging/utils.go`:
- Around line 96-103: Validate the incoming dimension against
ValidHistogramDimensions at the start of GetDimensionCostHistogram,
GetDimensionTokenHistogram, GetDimensionLatencyHistogram and their MatView
helper functions before any query construction; if the dimension is not in
ValidHistogramDimensions immediately return an error. Ensure this validation
occurs for both the fallback code path and the MatView path prior to using the
dimension in any Select() or Group() clauses so you never inject an unchecked
dimension into the SQL builder.
---
Outside diff comments:
In `@framework/logstore/matviews.go`:
- Around line 174-185: canUseMatViewFilters currently returns true even when
SearchFilters.Aliases is set, but mv_logs_hourly doesn't materialize alias and
applyMatViewFilters doesn't constrain it; update canUseMatViewFilters to also
require no alias filtering by checking that SearchFilters.Aliases (or the
appropriate alias field on SearchFilters) is empty/zero-length so the matview is
only used when alias filters are not present.
In `@framework/logstore/rdb.go`:
- Around line 2356-2385: The Postgres fast-path in GetDistinctKeyPairs returns
to call getDistinctKeyPairsFromMatView without validating idCol/nameCol or
checking canUseMatView, leaving fmt.Sprintf interpolation unguarded; move the
allowlist checks against allowedKeyPairColumns and the canUseMatView readiness
gate to run before the s.db.Dialector.Name() == "postgres" early return, or
alternatively add those two checks at the start of
getDistinctKeyPairsFromMatView so idCol/nameCol are validated and canUseMatView
is enforced before any fmt.Sprintf-based query construction.
---
Duplicate comments:
In `@framework/logstore/rdb.go`:
- Around line 1969-1975: The matview fast-path check runs before the default
bucket is applied, so ensure bucketSizeSeconds is normalized to the default
(3600) at the start of the method; in GetDimensionCostHistogram call (and the
similar methods around the other reported ranges, e.g., the latency histogram
variant), set bucketSizeSeconds = 3600 when <= 0 before evaluating
s.db.Dialector.Name() == "postgres" && s.canUseMatView(...) && bucketSizeSeconds
>= 3600, so that getDimensionCostHistogramFromMatView (and the latency matview
path) are chosen correctly when callers omit the bucket.
---
Nitpick comments:
In `@plugins/logging/utils.go`:
- Around line 81-92: GetAvailableUsers currently returns []KeyPair but callers
may get blank labels because user entries only populate user_id; update the
contract to be explicit by either (A) changing GetAvailableUsers(ctx
context.Context) to return []string or []UserID (ID-only) to reflect it only
provides IDs, or (B) keep []KeyPair but ensure the implementation that builds
GetAvailableUsers always sets KeyPair.Name = KeyPair.ID (or a formatted label)
so the UI filter shows a usable label; update the function signature and any
callers (or update the implementation that constructs KeyPair in
GetAvailableUsers) and add a short comment on GetAvailableUsers to document the
invariant.
In `@transports/bifrost-http/handlers/logging.go`:
- Around line 118-129: Extract the repeated parsing logic into a single helper
(e.g., create a function like applyGovernanceFilters(ctx *fasthttp.RequestCtx,
filters *LogFilters) or parseGovernanceIDs(ctx, &filters.TeamIDs,
&filters.CustomerIDs, &filters.UserIDs, &filters.BusinessUnitIDs)) and replace
the duplicated blocks in getLogs, getLogsStats, and parseHistogramFilters with
calls to that helper; the helper should read the same query params
("team_ids","customer_ids","user_ids","business_unit_ids"), call
parseCommaSeparated for each non-empty value, and assign into the corresponding
fields on the provided filters struct.
🪄 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: 5d686a86-bab7-42b1-90f9-b544b21b6a90
⛔ Files ignored due to path filters (12)
core/go.sumis excluded by!**/*.sumframework/go.sumis excluded by!**/*.sumplugins/governance/go.sumis excluded by!**/*.sumplugins/jsonparser/go.sumis excluded by!**/*.sumplugins/litellmcompat/go.sumis excluded by!**/*.sumplugins/logging/go.sumis excluded by!**/*.sumplugins/maxim/go.sumis excluded by!**/*.sumplugins/mocker/go.sumis excluded by!**/*.sumplugins/otel/go.sumis excluded by!**/*.sumplugins/semanticcache/go.sumis excluded by!**/*.sumplugins/telemetry/go.sumis excluded by!**/*.sumtransports/go.sumis excluded by!**/*.sum
📒 Files selected for processing (32)
core/go.modcore/schemas/bifrost.goframework/configstore/rdb.goframework/configstore/tables/virtualkey.goframework/go.modframework/logstore/matviews.goframework/logstore/migrations.goframework/logstore/postgres.goframework/logstore/rdb.goframework/logstore/store.goframework/logstore/tables.goplugins/governance/go.modplugins/governance/main.goplugins/governance/store.goplugins/jsonparser/go.modplugins/litellmcompat/go.modplugins/logging/go.modplugins/logging/main.goplugins/logging/operations.goplugins/logging/utils.goplugins/logging/writer.goplugins/maxim/go.modplugins/mocker/go.modplugins/otel/go.modplugins/semanticcache/go.modplugins/telemetry/go.modtransports/bifrost-http/handlers/logging.gotransports/bifrost-http/lib/config_test.gotransports/go.modui/app/workspace/logs/sheets/logDetailsSheet.tsxui/app/workspace/virtual-keys/views/virtualKeySheet.tsxui/lib/types/logs.ts
✅ Files skipped from review due to trivial changes (18)
- plugins/governance/go.mod
- core/go.mod
- plugins/mocker/go.mod
- plugins/logging/go.mod
- framework/go.mod
- framework/configstore/tables/virtualkey.go
- plugins/semanticcache/go.mod
- plugins/litellmcompat/go.mod
- plugins/maxim/go.mod
- plugins/otel/go.mod
- plugins/telemetry/go.mod
- ui/app/workspace/logs/sheets/logDetailsSheet.tsx
- ui/lib/types/logs.ts
- plugins/governance/store.go
- plugins/jsonparser/go.mod
- core/schemas/bifrost.go
- transports/go.mod
- transports/bifrost-http/lib/config_test.go
🚧 Files skipped from review as they are similar to previous changes (7)
- framework/logstore/postgres.go
- ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx
- plugins/logging/writer.go
- framework/logstore/migrations.go
- plugins/governance/main.go
- plugins/logging/main.go
- plugins/logging/operations.go
akshaydeo
force-pushed
the
04-06-adds_support_for_tracking_userid_teamid_customerid_and_businessunitid_in_logging
branch
from
2ad9415 to
7054e19
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
framework/logstore/matviews.go (1)
164-168:⚠️ Potential issue | 🟠 MajorThe recovery path can still flip
matViewsReadyon an advisory-lock miss.Line 166 treats any
nilfromrefreshMatViewsas a successful recovery, butrefreshMatViewsalso returnsnilwhen another replica merely owns the advisory lock. That reopens the same readiness problem through the background refresher: this instance can start routing requests to matviews without any confirmed refresh success.🔧 Suggested fix
- if err := refreshMatViews(ctx, db); err != nil { + refreshed, err := refreshMatViews(ctx, db) + if err != nil { logger.Warn(fmt.Sprintf("logstore: matview refresh failed: %s", err)) - } else if readyFlag != nil && !readyFlag.Load() { + } else if refreshed && readyFlag != nil && !readyFlag.Load() { logger.Info("logstore: materialized views are ready (recovered)") readyFlag.Store(true) }// refreshMatViews should return (refreshed bool, err error) // and return (false, nil) on advisory-lock miss.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/logstore/matviews.go` around lines 164 - 168, refreshMatViews currently returns nil on both success and advisory-lock miss, allowing the background refresher to flip readyFlag (matViewsReady) incorrectly; change refreshMatViews signature to return (refreshed bool, err error) and ensure it returns (false, nil) when the advisory lock is missed and (true, nil) only after a successful refresh; then update the caller here to check the refreshed boolean (only call readyFlag.Store(true) when refreshed == true) and propagate the new return shape to any other call sites of refreshMatViews (e.g., background refresher and tests).
🧹 Nitpick comments (2)
plugins/governance/main.go (1)
338-338: Preferbifrost.Ptr()overnew(expr)for consistency.This line deviates from the repository convention of using
bifrost.Ptr()to create pointers. Thenew(enforceAuthOnInference)syntax, while valid in Go 1.26+, is not idiomatic for this codebase.♻️ Suggested fix to maintain consistency
- p.isVkMandatory = new(enforceAuthOnInference) + p.isVkMandatory = bifrost.Ptr(enforceAuthOnInference)Based on learnings: "In the maximhq/bifrost repository, prefer using bifrost.Ptr() to create pointers instead of the address operator (&) even when & would be valid syntactically. Apply this consistently across all code paths..."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/governance/main.go` at line 338, Replace the use of new(enforceAuthOnInference) with bifrost.Ptr(enforceAuthOnInference{}) to follow repository pointer conventions; specifically update the assignment to p.isVkMandatory so it calls bifrost.Ptr with a literal enforceAuthOnInference value (using the struct literal syntax) instead of using new(), ensuring consistency with other uses of bifrost.Ptr.transports/bifrost-http/handlers/logging.go (1)
118-129: CentralizeSearchFiltersquery parsing.The same governance ID parsing was just added to
getLogs,getLogsStats, andparseHistogramFilters, and the rest of the filter matrix is already duplicated across those paths. This is a drift trap; the next filter change will likely miss one endpoint.♻️ Suggested shape
+func parseLogFilters(ctx *fasthttp.RequestCtx) *logstore.SearchFilters { + filters := &logstore.SearchFilters{} + // all shared query parsing lives here + return filters +} ... - filters := &logstore.SearchFilters{} - // duplicated parsing... + filters := parseLogFilters(ctx)Also applies to: 344-355, 488-499
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/handlers/logging.go` around lines 118 - 129, Create a single helper that centralizes QueryArgs -> SearchFilters parsing (including governance IDs) and replace the duplicated blocks in getLogs, getLogsStats, and parseHistogramFilters with that helper; specifically implement something like parseSearchFiltersFromCtx(ctx) that returns a SearchFilters with TeamIDs, CustomerIDs, UserIDs, BusinessUnitIDs (using the existing parseCommaSeparated) and any governance ID logic, then update callers (getLogs, getLogsStats, parseHistogramFilters) to call this helper and remove the repeated parsing code blocks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@framework/logstore/matviews.go`:
- Around line 193-200: The calls to matview-backed helpers
(getDistinctModelsFromMatView, getDistinctKeyPairsFromMatView,
getDistinctRoutingEnginesFromMatView) from GetDistinctModels,
GetDistinctKeyPairs, and GetDistinctRoutingEngines in rdb.go lack the startup
readiness gate and thus can hit missing mv_logs_filterdata; update each of those
three methods to check s.matViewsReady (the same boolean used in
RDBLogStore.canUseMatView) before calling the matview helper and either return
the raw-table fallback (existing non-matview path) when matViewsReady is false
or skip to the fallback branch, ensuring no direct call to matview helpers when
s.matViewsReady.Load() is false.
In `@framework/logstore/migrations.go`:
- Around line 2232-2272: migrationAddGovernanceContextColumns currently only
adds the new governance columns (columns slice) but does not create indexes,
which will cause slow scans; update the migration
(migrationAddGovernanceContextColumns -> Migrate and Rollback functions) to
create B-tree indexes for team_id, customer_id, user_id and business_unit_id
after adding the columns (use tx.Exec or tx.Migrator().CreateIndex equivalents)
and drop those indexes in Rollback; additionally, extend the histogram
covering-index rebuild path (where the covering index is reconstructed for
logs/histogram queries) to include these new governance ID columns so the
non-matview histogram fallback remains performant.
In `@framework/logstore/tables.go`:
- Around line 1223-1230: ValidHistogramDimensions is exported as a mutable
package variable which allows callers to change the allowed HistogramDimension
at runtime; make the allowlist immutable by unexporting the backing map and
exposing either a read-only function or a copy-returning accessor. Replace the
exported var ValidHistogramDimensions with an unexported map (e.g.,
validHistogramDimensions) and add a function like ValidHistogramDimensions() or
SupportedHistogramDimensions() that returns a copy or slice of allowed
HistogramDimension values; callers (and parseDimension) should use the accessor
instead of modifying the map directly so external packages cannot widen the set
at runtime.
In `@ui/app/workspace/logs/page.tsx`:
- Around line 203-206: handleLogMessage inserts/updates live websocket rows
using matchesFilters, but matchesFilters doesn't check the new governance filter
fields (user_ids, team_ids, customer_ids, business_unit_ids), so live updates
can bypass the selected scope; update matchesFilters to evaluate those four
fields (using the same semantics as server fetches) and ensure handleLogMessage
calls the updated matchesFilters for both insert and update paths so
websocket-driven rows and live stats/histogram stay in sync with urlState
filters.
In `@ui/app/workspace/logs/sheets/logDetailsSheet.tsx`:
- Around line 314-357: The four new governance Link elements inside the
LogEntryDetailsView blocks (the ones rendering Team, Customer, Business Unit,
and User using displayLog.team_id/customer_id/business_unit_id/user_id) lack
data-testid attributes; add data-testid attributes to each Link following the
project pattern data-testid="<entity>-link-navigation" (e.g.,
team-link-navigation, customer-link-navigation, businessunit-link-navigation,
user-link-navigation) so E2E tests can target them while leaving the surrounding
LogEntryDetailsView usage unchanged.
---
Duplicate comments:
In `@framework/logstore/matviews.go`:
- Around line 164-168: refreshMatViews currently returns nil on both success and
advisory-lock miss, allowing the background refresher to flip readyFlag
(matViewsReady) incorrectly; change refreshMatViews signature to return
(refreshed bool, err error) and ensure it returns (false, nil) when the advisory
lock is missed and (true, nil) only after a successful refresh; then update the
caller here to check the refreshed boolean (only call readyFlag.Store(true) when
refreshed == true) and propagate the new return shape to any other call sites of
refreshMatViews (e.g., background refresher and tests).
---
Nitpick comments:
In `@plugins/governance/main.go`:
- Line 338: Replace the use of new(enforceAuthOnInference) with
bifrost.Ptr(enforceAuthOnInference{}) to follow repository pointer conventions;
specifically update the assignment to p.isVkMandatory so it calls bifrost.Ptr
with a literal enforceAuthOnInference value (using the struct literal syntax)
instead of using new(), ensuring consistency with other uses of bifrost.Ptr.
In `@transports/bifrost-http/handlers/logging.go`:
- Around line 118-129: Create a single helper that centralizes QueryArgs ->
SearchFilters parsing (including governance IDs) and replace the duplicated
blocks in getLogs, getLogsStats, and parseHistogramFilters with that helper;
specifically implement something like parseSearchFiltersFromCtx(ctx) that
returns a SearchFilters with TeamIDs, CustomerIDs, UserIDs, BusinessUnitIDs
(using the existing parseCommaSeparated) and any governance ID logic, then
update callers (getLogs, getLogsStats, parseHistogramFilters) to call this
helper and remove the repeated parsing code blocks.
🪄 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: 3d3d2b7b-61d7-4862-99df-e905ae61551f
⛔ Files ignored due to path filters (12)
core/go.sumis excluded by!**/*.sumframework/go.sumis excluded by!**/*.sumplugins/governance/go.sumis excluded by!**/*.sumplugins/jsonparser/go.sumis excluded by!**/*.sumplugins/litellmcompat/go.sumis excluded by!**/*.sumplugins/logging/go.sumis excluded by!**/*.sumplugins/maxim/go.sumis excluded by!**/*.sumplugins/mocker/go.sumis excluded by!**/*.sumplugins/otel/go.sumis excluded by!**/*.sumplugins/semanticcache/go.sumis excluded by!**/*.sumplugins/telemetry/go.sumis excluded by!**/*.sumtransports/go.sumis excluded by!**/*.sum
📒 Files selected for processing (36)
core/go.modcore/schemas/bifrost.goframework/configstore/rdb.goframework/configstore/tables/virtualkey.goframework/go.modframework/logstore/matviews.goframework/logstore/migrations.goframework/logstore/postgres.goframework/logstore/rdb.goframework/logstore/store.goframework/logstore/tables.goplugins/governance/go.modplugins/governance/main.goplugins/governance/store.goplugins/jsonparser/go.modplugins/litellmcompat/go.modplugins/logging/go.modplugins/logging/main.goplugins/logging/operations.goplugins/logging/utils.goplugins/logging/writer.goplugins/maxim/go.modplugins/mocker/go.modplugins/otel/go.modplugins/semanticcache/go.modplugins/telemetry/go.modtransports/bifrost-http/handlers/logging.gotransports/bifrost-http/lib/config_test.gotransports/go.modui/app/_fallbacks/enterprise/components/user-rankings/userRankingsTab.tsxui/app/workspace/dashboard/page.tsxui/app/workspace/logs/page.tsxui/app/workspace/logs/sheets/logDetailsSheet.tsxui/app/workspace/virtual-keys/views/virtualKeySheet.tsxui/lib/store/apis/logsApi.tsui/lib/types/logs.ts
✅ Files skipped from review due to trivial changes (16)
- plugins/jsonparser/go.mod
- plugins/governance/go.mod
- plugins/litellmcompat/go.mod
- core/go.mod
- plugins/semanticcache/go.mod
- plugins/logging/go.mod
- plugins/mocker/go.mod
- plugins/maxim/go.mod
- plugins/otel/go.mod
- framework/configstore/tables/virtualkey.go
- framework/go.mod
- plugins/telemetry/go.mod
- transports/go.mod
- core/schemas/bifrost.go
- plugins/governance/store.go
- plugins/logging/operations.go
🚧 Files skipped from review as they are similar to previous changes (7)
- plugins/logging/writer.go
- ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx
- plugins/logging/main.go
- framework/configstore/rdb.go
- ui/lib/types/logs.ts
- framework/logstore/store.go
- framework/logstore/rdb.go
akshaydeo
force-pushed
the
04-06-adds_support_for_tracking_userid_teamid_customerid_and_businessunitid_in_logging
branch
from
7054e19 to
5ef6711
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
plugins/logging/main.go (1)
743-743: Optional: reduce positional-argument risk inapplyOutputFieldsToEntrycall.This call now has many ordered string parameters; consider grouping governance fields into a small struct to make future additions safer.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/logging/main.go` at line 743, The call to applyOutputFieldsToEntry passes many positional governance-related strings; define a small struct (e.g., GovernanceFields or OutputGovernance) containing selectedKeyID, selectedKeyName, virtualKeyID, virtualKeyName, routingRuleID, routingRuleName, teamID, teamName, customerID, customerName, userID, businessUnitID, businessUnitName, update the applyOutputFieldsToEntry signature to accept that struct plus the remaining params (numberOfRetries, latency), and replace the long positional call with a single governance struct argument to reduce future positional-argument risk; update all callers of applyOutputFieldsToEntry and any tests accordingly.docs/openapi/paths/management/users.yaml (1)
181-205: Add explicit not-found response forgetUserTeamsfor contract consistency.
PUT /users/{id}/teamsalready documents404, butGET /users/{id}/teamsdoes not. For the same resource identifier, clients benefit from a consistent not-found contract (Line [188], Line [240]).📄 Suggested OpenAPI diff
get: @@ responses: '200': description: Successful response content: application/json: schema: $ref: '../../schemas/management/users.yaml#/UserTeamsResponse' '400': $ref: '../../openapi.yaml#/components/responses/BadRequest' + '404': + description: User not found + content: + application/json: + schema: + $ref: '../../schemas/management/common.yaml#/ErrorResponse' '500': $ref: '../../openapi.yaml#/components/responses/InternalError'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/openapi/paths/management/users.yaml` around lines 181 - 205, Add an explicit 404 Not Found response to the GET /users/{id}/teams operation (operationId: getUserTeams) to match the PUT /users/{id}/teams contract; update the responses block for getUserTeams to include a '404' response that references the shared NotFound response component (e.g., the components.responses.NotFound entry) so clients get a consistent not-found contract for the user teams resource.framework/logstore/tables.go (1)
1215-1230: Consider decoupling API dimension names from SQL column names.These constants make storage names like
team_idandbusiness_unit_idpart of the new HTTP contract. Since this API is new, mapping stable semantic values such asteam,customer,user, andbusiness_unitto the underlying columns internally would keep clients insulated from future schema changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/logstore/tables.go` around lines 1215 - 1230, The constants DimensionProvider/DimensionTeam/DimensionCustomer/DimensionUser/DimensionBusinessUnit and the map ValidHistogramDimensions expose SQL column names (e.g., "team_id", "business_unit_id") as API dimension values; change them to use stable semantic API names (e.g., "provider","team","customer","user","business_unit") and add an internal mapping from those API names to concrete column names used in queries; update consumers to validate against ValidHistogramDimensions (using the new API names) and translate via the new map when building SQL (refer to DimensionTeam, DimensionBusinessUnit, DimensionProvider, ValidHistogramDimensions to locate where to rename values and where to add the api->column mapping).transports/bifrost-http/handlers/logging.go (1)
118-129: Extract the governance filter parsing once.This
team_ids/customer_ids/user_ids/business_unit_idsblock is now copied into three request parsers. A small helper here would keep/api/logs,/api/logs/stats, and the histogram endpoints from drifting on the next filter change.Also applies to: 344-355, 488-499
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/handlers/logging.go` around lines 118 - 129, Extract the repeated governance filter parsing into a shared helper (e.g., a function like parseGovernanceFilters(ctx, filters *LogFilters) or parseGovernanceFilters(ctx *fasthttp.RequestCtx) that returns a LogFilters partial) that reads "team_ids", "customer_ids", "user_ids", and "business_unit_ids" using ctx.QueryArgs().Peek and parseCommaSeparated, and sets filters.TeamIDs/CustomerIDs/UserIDs/BusinessUnitIDs accordingly; then replace the duplicated blocks in the handlers that build filters for the /api/logs, /api/logs/stats and histogram endpoints with a single call to that helper (refer to parseCommaSeparated and the filters.TeamIDs/CustomerIDs/UserIDs/BusinessUnitIDs fields to locate the code to change).plugins/logging/utils.go (1)
87-88: Clarify docstring forGetAvailableUsersreturn type.The docstring says "unique user IDs" but the method returns
[]KeyPair(ID-Name pairs). Based on the PR summary, the Log model only hasuser_idwithout auser_namefield. Consider updating the docstring to clarify what theNamefield contains (e.g., empty or same as ID) to match the pattern of otherGetAvailable*methods that document "ID-Name pairs."📝 Suggested docstring improvement
- // GetAvailableUsers returns all unique user IDs from logs + // GetAvailableUsers returns all unique user ID-Name pairs from logs (Name may be empty if not provided) GetAvailableUsers(ctx context.Context) []KeyPair🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/logging/utils.go` around lines 87 - 88, Update the docstring for GetAvailableUsers to accurately reflect that it returns []KeyPair (ID-Name pairs) rather than just "unique user IDs"; explicitly state how the Name field is populated (e.g., empty string or same as the ID) given the Log model only contains user_id, and mirror the phrasing used by the other GetAvailable* methods to document "ID-Name pairs" so callers understand the Name semantics for KeyPair.framework/logstore/rdb.go (1)
2139-2156: Consider extracting shared “no explicit time range” bucket assembly logic.The same key-sort + bucket-build pattern appears in all three dimension histogram methods; a small helper would reduce repetition and future drift.
Also applies to: 2245-2266, 2353-2369
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/logstore/rdb.go` around lines 2139 - 2156, Extract the repeated "no explicit time range" logic that sorts keys from grouped, builds []DimensionCostHistogramBucket and returns a *DimensionCostHistogramResult into a reusable helper (e.g., buildBucketsFromGrouped(grouped map[int64]..., bucketSizeSeconds int64, dimension string, dimValues []string) *DimensionCostHistogramResult or similar). Replace the duplicated blocks in the three dimension histogram methods that check len(allTimestamps) == 0 to call this helper (passing grouped, bucketSizeSeconds, dimension, DimensionValues) so the shared behavior for assembling DimensionCostHistogramBucket entries (using keys := make(...); sort.Slice(keys...); creating buckets with DimensionCostHistogramBucket{Timestamp: time.Unix(ts,0).UTC(), TotalCost: a.totalCost, ByDimension: a.byDimension}) is centralized and returns the same DimensionCostHistogramResult.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/openapi/paths/management/users.yaml`:
- Around line 349-367: The updateTeam operation (operationId: updateTeam)
currently references CreateTeamRequest which contains a required name field
despite the description saying "renaming teams is not allowed"; either update
the requestBody schema reference to a new UpdateTeamRequest that omits or makes
the name field optional/immutable, or change the description to allow renaming
and keep CreateTeamRequest. Create the new UpdateTeamRequest schema (based on
CreateTeamRequest but without required name) and point the operationBody $ref to
'../../schemas/management/users.yaml#/UpdateTeamRequest' (or alternatively edit
the description text to reflect that name is allowed).
---
Nitpick comments:
In `@docs/openapi/paths/management/users.yaml`:
- Around line 181-205: Add an explicit 404 Not Found response to the GET
/users/{id}/teams operation (operationId: getUserTeams) to match the PUT
/users/{id}/teams contract; update the responses block for getUserTeams to
include a '404' response that references the shared NotFound response component
(e.g., the components.responses.NotFound entry) so clients get a consistent
not-found contract for the user teams resource.
In `@framework/logstore/rdb.go`:
- Around line 2139-2156: Extract the repeated "no explicit time range" logic
that sorts keys from grouped, builds []DimensionCostHistogramBucket and returns
a *DimensionCostHistogramResult into a reusable helper (e.g.,
buildBucketsFromGrouped(grouped map[int64]..., bucketSizeSeconds int64,
dimension string, dimValues []string) *DimensionCostHistogramResult or similar).
Replace the duplicated blocks in the three dimension histogram methods that
check len(allTimestamps) == 0 to call this helper (passing grouped,
bucketSizeSeconds, dimension, DimensionValues) so the shared behavior for
assembling DimensionCostHistogramBucket entries (using keys := make(...);
sort.Slice(keys...); creating buckets with
DimensionCostHistogramBucket{Timestamp: time.Unix(ts,0).UTC(), TotalCost:
a.totalCost, ByDimension: a.byDimension}) is centralized and returns the same
DimensionCostHistogramResult.
In `@framework/logstore/tables.go`:
- Around line 1215-1230: The constants
DimensionProvider/DimensionTeam/DimensionCustomer/DimensionUser/DimensionBusinessUnit
and the map ValidHistogramDimensions expose SQL column names (e.g., "team_id",
"business_unit_id") as API dimension values; change them to use stable semantic
API names (e.g., "provider","team","customer","user","business_unit") and add an
internal mapping from those API names to concrete column names used in queries;
update consumers to validate against ValidHistogramDimensions (using the new API
names) and translate via the new map when building SQL (refer to DimensionTeam,
DimensionBusinessUnit, DimensionProvider, ValidHistogramDimensions to locate
where to rename values and where to add the api->column mapping).
In `@plugins/logging/main.go`:
- Line 743: The call to applyOutputFieldsToEntry passes many positional
governance-related strings; define a small struct (e.g., GovernanceFields or
OutputGovernance) containing selectedKeyID, selectedKeyName, virtualKeyID,
virtualKeyName, routingRuleID, routingRuleName, teamID, teamName, customerID,
customerName, userID, businessUnitID, businessUnitName, update the
applyOutputFieldsToEntry signature to accept that struct plus the remaining
params (numberOfRetries, latency), and replace the long positional call with a
single governance struct argument to reduce future positional-argument risk;
update all callers of applyOutputFieldsToEntry and any tests accordingly.
In `@plugins/logging/utils.go`:
- Around line 87-88: Update the docstring for GetAvailableUsers to accurately
reflect that it returns []KeyPair (ID-Name pairs) rather than just "unique user
IDs"; explicitly state how the Name field is populated (e.g., empty string or
same as the ID) given the Log model only contains user_id, and mirror the
phrasing used by the other GetAvailable* methods to document "ID-Name pairs" so
callers understand the Name semantics for KeyPair.
In `@transports/bifrost-http/handlers/logging.go`:
- Around line 118-129: Extract the repeated governance filter parsing into a
shared helper (e.g., a function like parseGovernanceFilters(ctx, filters
*LogFilters) or parseGovernanceFilters(ctx *fasthttp.RequestCtx) that returns a
LogFilters partial) that reads "team_ids", "customer_ids", "user_ids", and
"business_unit_ids" using ctx.QueryArgs().Peek and parseCommaSeparated, and sets
filters.TeamIDs/CustomerIDs/UserIDs/BusinessUnitIDs accordingly; then replace
the duplicated blocks in the handlers that build filters for the /api/logs,
/api/logs/stats and histogram endpoints with a single call to that helper (refer
to parseCommaSeparated and the
filters.TeamIDs/CustomerIDs/UserIDs/BusinessUnitIDs fields to locate the code to
change).
🪄 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: 381e4f65-6117-4d17-b25d-b2f43e53ac0b
⛔ Files ignored due to path filters (12)
core/go.sumis excluded by!**/*.sumframework/go.sumis excluded by!**/*.sumplugins/governance/go.sumis excluded by!**/*.sumplugins/jsonparser/go.sumis excluded by!**/*.sumplugins/litellmcompat/go.sumis excluded by!**/*.sumplugins/logging/go.sumis excluded by!**/*.sumplugins/maxim/go.sumis excluded by!**/*.sumplugins/mocker/go.sumis excluded by!**/*.sumplugins/otel/go.sumis excluded by!**/*.sumplugins/semanticcache/go.sumis excluded by!**/*.sumplugins/telemetry/go.sumis excluded by!**/*.sumtransports/go.sumis excluded by!**/*.sum
📒 Files selected for processing (41)
core/go.modcore/schemas/bifrost.godocs/openapi/openapi.jsondocs/openapi/openapi.yamldocs/openapi/paths/management/users.yamldocs/openapi/schemas/management/users.yamlframework/configstore/rdb.goframework/configstore/tables/virtualkey.goframework/go.modframework/logstore/matviews.goframework/logstore/migrations.goframework/logstore/postgres.goframework/logstore/rdb.goframework/logstore/store.goframework/logstore/tables.goplugins/governance/go.modplugins/governance/main.goplugins/governance/store.goplugins/jsonparser/go.modplugins/litellmcompat/go.modplugins/logging/go.modplugins/logging/main.goplugins/logging/operations.goplugins/logging/utils.goplugins/logging/writer.goplugins/maxim/go.modplugins/mocker/go.modplugins/otel/go.modplugins/semanticcache/go.modplugins/telemetry/go.modtransports/bifrost-http/handlers/logging.gotransports/bifrost-http/lib/config_test.gotransports/go.modui/app/_fallbacks/enterprise/components/user-rankings/userRankingsTab.tsxui/app/workspace/dashboard/page.tsxui/app/workspace/logs/page.tsxui/app/workspace/logs/sheets/logDetailsSheet.tsxui/app/workspace/virtual-keys/views/virtualKeySheet.tsxui/lib/store/apis/baseApi.tsui/lib/store/apis/logsApi.tsui/lib/types/logs.ts
✅ Files skipped from review due to trivial changes (19)
- plugins/jsonparser/go.mod
- core/go.mod
- plugins/governance/go.mod
- ui/lib/store/apis/baseApi.ts
- framework/go.mod
- plugins/mocker/go.mod
- plugins/otel/go.mod
- plugins/litellmcompat/go.mod
- plugins/logging/go.mod
- framework/configstore/tables/virtualkey.go
- plugins/telemetry/go.mod
- plugins/maxim/go.mod
- ui/app/workspace/dashboard/page.tsx
- transports/go.mod
- plugins/governance/main.go
- plugins/governance/store.go
- ui/app/workspace/logs/sheets/logDetailsSheet.tsx
- docs/openapi/schemas/management/users.yaml
- transports/bifrost-http/lib/config_test.go
🚧 Files skipped from review as they are similar to previous changes (11)
- plugins/semanticcache/go.mod
- ui/lib/store/apis/logsApi.ts
- framework/logstore/migrations.go
- ui/app/_fallbacks/enterprise/components/user-rankings/userRankingsTab.tsx
- ui/app/workspace/logs/page.tsx
- framework/logstore/store.go
- plugins/logging/operations.go
- framework/logstore/postgres.go
- core/schemas/bifrost.go
- plugins/logging/writer.go
- framework/logstore/matviews.go
akshaydeo
force-pushed
the
04-06-adds_support_for_tracking_userid_teamid_customerid_and_businessunitid_in_logging
branch
2 times, most recently
from
d4aa32d to
d1845e0
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx (1)
496-496:⚠️ Potential issue | 🟠 MajorUpdate e2e page object selector to match the renamed sheet test id.
Line 496 changed the selector to
vk-sheet-content, buttests/e2e/features/virtual-keys/pages/virtual-keys.page.tsstill queriesvk-sheet. This will break Virtual Keys e2e flows that rely onthis.sheet.Suggested fix (tests/e2e/features/virtual-keys/pages/virtual-keys.page.ts)
- this.sheet = page.getByTestId('vk-sheet') + this.sheet = page.getByTestId('vk-sheet-content')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx` at line 496, Update the E2E page object so its selector matches the renamed test id: replace the old query for "vk-sheet" with "vk-sheet-content" where this.sheet is defined in the VirtualKeys page object (references to this.sheet in virtual-keys.page.ts); ensure any methods that use this.sheet (e.g., getters or wait/visibility helpers) still work with the new selector string.framework/logstore/migrations.go (1)
2048-2066:⚠️ Potential issue | 🟠 MajorAdd the governance IDs to the histogram covering index as well.
These four B-tree indexes help plain filters, but the raw histogram path still relies on
idx_logs_histogram_coverand its rebuild inensureDashboardEnhancements, and both definitions still omitteam_id,customer_id,user_id, andbusiness_unit_id. On SQLite and on PostgreSQL queries that can't use matviews, histogram requests with the new governance filters will still fall back to heap lookups against the widelogsrow.Suggested follow-up
// framework/logstore/migrations.go - initial covering index - status, timestamp, - selected_key_id, virtual_key_id, routing_rule_id, provider, object_type, - model, cost, prompt_tokens, completion_tokens, total_tokens + status, timestamp, + selected_key_id, virtual_key_id, routing_rule_id, provider, object_type, + team_id, customer_id, user_id, business_unit_id, + model, cost, prompt_tokens, completion_tokens, total_tokens // framework/logstore/migrations.go - dashboard rebuild path - status, timestamp, - selected_key_id, virtual_key_id, routing_rule_id, provider, object_type, - model, cost, prompt_tokens, completion_tokens, total_tokens, cached_read_tokens + status, timestamp, + selected_key_id, virtual_key_id, routing_rule_id, provider, object_type, + team_id, customer_id, user_id, business_unit_id, + model, cost, prompt_tokens, completion_tokens, total_tokens, cached_read_tokens🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/logstore/migrations.go` around lines 2048 - 2066, The histogram-covering index idx_logs_histogram_cover is missing governance columns (team_id, customer_id, user_id, business_unit_id), so update its SQL definition in migrations.go to include those columns (so the index covers the governance filters) and also update the index rebuild logic inside ensureDashboardEnhancements to recreate the index with the new column list; locate the idx_logs_histogram_cover entry in the migrations slice and the code path in ensureDashboardEnhancements that drops/creates or rebuilds that index and add the four governance columns to both places.
🧹 Nitpick comments (3)
plugins/governance/main.go (1)
407-421: Extract duplicated governance context attachment into a helper.The Team/Customer attachment block is repeated in two paths. Pulling this into one helper will reduce drift risk when governance fields evolve.
♻️ Suggested refactor
+func attachVirtualKeyGovernanceContext(ctx *schemas.BifrostContext, virtualKey *configstoreTables.TableVirtualKey) { + if virtualKey == nil { + return + } + if virtualKey.TeamID != nil { + ctx.SetValue(schemas.BifrostContextKeyGovernanceTeamID, *virtualKey.TeamID) + } + if virtualKey.Team != nil { + ctx.SetValue(schemas.BifrostContextKeyGovernanceTeamName, virtualKey.Team.Name) + } + if virtualKey.CustomerID != nil { + ctx.SetValue(schemas.BifrostContextKeyGovernanceCustomerID, *virtualKey.CustomerID) + } + if virtualKey.Customer != nil { + ctx.SetValue(schemas.BifrostContextKeyGovernanceCustomerName, virtualKey.Customer.Name) + } +}- // Attaching team and customer based on the virtual key - if virtualKey != nil { - ... - } + attachVirtualKeyGovernanceContext(ctx, virtualKey)Also applies to: 506-520
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/governance/main.go` around lines 407 - 421, Extract the duplicated Team/Customer ctx.SetValue block into a single helper (e.g., attachGovernanceContext or attachVirtualKeyToContext) that takes the context (ctx) and the virtualKey object and returns nothing; move the nil-check for virtualKey into that helper and have it set BifrostContextKeyGovernanceTeamID, BifrostContextKeyGovernanceTeamName, BifrostContextKeyGovernanceCustomerID, and BifrostContextKeyGovernanceCustomerName exactly as the current code does; then replace both occurrences in main.go (the block around virtualKey and the second occurrence at lines ~506-520) with a call to this helper to avoid duplication and keep behavior identical.transports/bifrost-http/handlers/logging.go (1)
256-267: Keep the governance filter parsing in one helper.The same four query params are now parsed in three places. The next filter change has to touch all three paths, which is an easy way for logs, stats, and histogram behavior to drift.
Suggested cleanup
+func parseGovernanceIDFilters(args *fasthttp.Args, filters *logstore.SearchFilters) { + if v := string(args.Peek("team_ids")); v != "" { + filters.TeamIDs = parseCommaSeparated(v) + } + if v := string(args.Peek("customer_ids")); v != "" { + filters.CustomerIDs = parseCommaSeparated(v) + } + if v := string(args.Peek("user_ids")); v != "" { + filters.UserIDs = parseCommaSeparated(v) + } + if v := string(args.Peek("business_unit_ids")); v != "" { + filters.BusinessUnitIDs = parseCommaSeparated(v) + } +}Then call it from
getLogs,getLogsStats, andparseHistogramFilters.Also applies to: 485-496, 632-643
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/handlers/logging.go` around lines 256 - 267, Extract the repeated query-param parsing into a single helper (e.g., parseGovernanceFilters(ctx, filters) or parseRequestGovernanceFilters) that reads "team_ids", "customer_ids", "user_ids", and "business_unit_ids" using ctx.QueryArgs().Peek and sets filters.TeamIDs/CustomerIDs/UserIDs/BusinessUnitIDs via parseCommaSeparated; replace the duplicated blocks in getLogs, getLogsStats, and parseHistogramFilters to call this helper so future changes to governance filters are made in one place.framework/logstore/rdb.go (1)
2267-2273: Consider extracting shared bucket-expression logic.The dialect-specific
bucketExprswitch is repeated in all three dimension histogram methods. A small helper would reduce drift risk and simplify future changes.Also applies to: 2361-2367, 2478-2484
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/logstore/rdb.go` around lines 2267 - 2273, Extract the repeated dialect-specific switch that builds bucketExpr into a small helper like buildBucketExpr(dialect string, bucketSizeSeconds int) string and replace the duplicated switch blocks in the three dimension histogram methods with calls to that helper; the helper should produce the exact strings currently generated for the "sqlite" case (using strftime and CAST/INTEGER) and the default case (using EXTRACT(EPOCH), FLOOR and CAST AS BIGINT) so behavior is unchanged and callers (which currently create variable bucketExpr) simply assign bucketExpr = buildBucketExpr(dialect, bucketSizeSeconds).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/openapi/paths/management/users.yaml`:
- Around line 421-445: The getTeamMembers operation under the team-members path
is missing a 404 response for a non-existent team; update the getTeamMembers
responses to include a '404' entry (matching addTeamMember/removeTeamMember)
that references the appropriate component response (e.g.,
../../openapi.yaml#/components/responses/NotFound or the project’s standard
TeamNotFound response) so consumers know the API returns 404 when the team ID
does not exist.
- Around line 180-204: Add a 404 response to the getUserTeams operation
(operationId getUserTeams) to match the PUT counterpart: include a '404'
response entry that references the shared NotFound/OpenAPI component (e.g.
../../openapi.yaml#/components/responses/NotFound or the project's equivalent)
so callers know a missing user returns 404; update the responses block for
getUserTeams accordingly.
In `@docs/openapi/schemas/management/users.yaml`:
- Around line 182-194: ListTeamsResponse is missing the pagination fields
present in ListUsersResponse; update the ListTeamsResponse schema to include
total_pages (integer) and has_more (boolean) alongside total, page and limit so
pagination shape is consistent with ListUsersResponse; ensure the property names
and types match exactly (total_pages: integer, has_more: boolean) and document
any required ordering/description to match ListUsersResponse behavior.
In `@framework/logstore/migrations.go`:
- Around line 2067-2069: The index entry for idx_logs_parent_request_id is
missing its table name so ensurePerformanceIndexes sees idx.table as empty and
always treats it as missing; update the index definition object for
"idx_logs_parent_request_id" to include table: "logs" (alongside the existing
name and sql) so ensurePerformanceIndexes can validate and skip recreating it
unnecessarily.
---
Duplicate comments:
In `@framework/logstore/migrations.go`:
- Around line 2048-2066: The histogram-covering index idx_logs_histogram_cover
is missing governance columns (team_id, customer_id, user_id, business_unit_id),
so update its SQL definition in migrations.go to include those columns (so the
index covers the governance filters) and also update the index rebuild logic
inside ensureDashboardEnhancements to recreate the index with the new column
list; locate the idx_logs_histogram_cover entry in the migrations slice and the
code path in ensureDashboardEnhancements that drops/creates or rebuilds that
index and add the four governance columns to both places.
In `@ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx`:
- Line 496: Update the E2E page object so its selector matches the renamed test
id: replace the old query for "vk-sheet" with "vk-sheet-content" where
this.sheet is defined in the VirtualKeys page object (references to this.sheet
in virtual-keys.page.ts); ensure any methods that use this.sheet (e.g., getters
or wait/visibility helpers) still work with the new selector string.
---
Nitpick comments:
In `@framework/logstore/rdb.go`:
- Around line 2267-2273: Extract the repeated dialect-specific switch that
builds bucketExpr into a small helper like buildBucketExpr(dialect string,
bucketSizeSeconds int) string and replace the duplicated switch blocks in the
three dimension histogram methods with calls to that helper; the helper should
produce the exact strings currently generated for the "sqlite" case (using
strftime and CAST/INTEGER) and the default case (using EXTRACT(EPOCH), FLOOR and
CAST AS BIGINT) so behavior is unchanged and callers (which currently create
variable bucketExpr) simply assign bucketExpr = buildBucketExpr(dialect,
bucketSizeSeconds).
In `@plugins/governance/main.go`:
- Around line 407-421: Extract the duplicated Team/Customer ctx.SetValue block
into a single helper (e.g., attachGovernanceContext or
attachVirtualKeyToContext) that takes the context (ctx) and the virtualKey
object and returns nothing; move the nil-check for virtualKey into that helper
and have it set BifrostContextKeyGovernanceTeamID,
BifrostContextKeyGovernanceTeamName, BifrostContextKeyGovernanceCustomerID, and
BifrostContextKeyGovernanceCustomerName exactly as the current code does; then
replace both occurrences in main.go (the block around virtualKey and the second
occurrence at lines ~506-520) with a call to this helper to avoid duplication
and keep behavior identical.
In `@transports/bifrost-http/handlers/logging.go`:
- Around line 256-267: Extract the repeated query-param parsing into a single
helper (e.g., parseGovernanceFilters(ctx, filters) or
parseRequestGovernanceFilters) that reads "team_ids", "customer_ids",
"user_ids", and "business_unit_ids" using ctx.QueryArgs().Peek and sets
filters.TeamIDs/CustomerIDs/UserIDs/BusinessUnitIDs via parseCommaSeparated;
replace the duplicated blocks in getLogs, getLogsStats, and
parseHistogramFilters to call this helper so future changes to governance
filters are made in one place.
🪄 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: db3f83e0-238b-4841-9553-57a2997a768e
⛔ Files ignored due to path filters (12)
core/go.sumis excluded by!**/*.sumframework/go.sumis excluded by!**/*.sumplugins/governance/go.sumis excluded by!**/*.sumplugins/jsonparser/go.sumis excluded by!**/*.sumplugins/litellmcompat/go.sumis excluded by!**/*.sumplugins/logging/go.sumis excluded by!**/*.sumplugins/maxim/go.sumis excluded by!**/*.sumplugins/mocker/go.sumis excluded by!**/*.sumplugins/otel/go.sumis excluded by!**/*.sumplugins/semanticcache/go.sumis excluded by!**/*.sumplugins/telemetry/go.sumis excluded by!**/*.sumtransports/go.sumis excluded by!**/*.sum
📒 Files selected for processing (41)
core/go.modcore/schemas/bifrost.godocs/openapi/openapi.jsondocs/openapi/openapi.yamldocs/openapi/paths/management/users.yamldocs/openapi/schemas/management/users.yamlframework/configstore/rdb.goframework/configstore/tables/virtualkey.goframework/go.modframework/logstore/matviews.goframework/logstore/migrations.goframework/logstore/postgres.goframework/logstore/rdb.goframework/logstore/store.goframework/logstore/tables.goplugins/governance/go.modplugins/governance/main.goplugins/governance/store.goplugins/jsonparser/go.modplugins/litellmcompat/go.modplugins/logging/go.modplugins/logging/main.goplugins/logging/operations.goplugins/logging/utils.goplugins/logging/writer.goplugins/maxim/go.modplugins/mocker/go.modplugins/otel/go.modplugins/semanticcache/go.modplugins/telemetry/go.modtransports/bifrost-http/handlers/logging.gotransports/bifrost-http/lib/config_test.gotransports/go.modui/app/_fallbacks/enterprise/components/user-rankings/userRankingsTab.tsxui/app/workspace/dashboard/page.tsxui/app/workspace/logs/page.tsxui/app/workspace/logs/sheets/logDetailView.tsxui/app/workspace/virtual-keys/views/virtualKeySheet.tsxui/lib/store/apis/baseApi.tsui/lib/store/apis/logsApi.tsui/lib/types/logs.ts
✅ Files skipped from review due to trivial changes (18)
- core/go.mod
- plugins/jsonparser/go.mod
- plugins/semanticcache/go.mod
- ui/lib/store/apis/baseApi.ts
- plugins/mocker/go.mod
- framework/go.mod
- plugins/maxim/go.mod
- transports/go.mod
- plugins/governance/go.mod
- plugins/logging/go.mod
- plugins/litellmcompat/go.mod
- plugins/telemetry/go.mod
- framework/configstore/tables/virtualkey.go
- ui/app/_fallbacks/enterprise/components/user-rankings/userRankingsTab.tsx
- ui/app/workspace/dashboard/page.tsx
- plugins/governance/store.go
- core/schemas/bifrost.go
- plugins/otel/go.mod
🚧 Files skipped from review as they are similar to previous changes (7)
- framework/logstore/postgres.go
- plugins/logging/writer.go
- framework/configstore/rdb.go
- ui/app/workspace/logs/page.tsx
- docs/openapi/openapi.yaml
- transports/bifrost-http/lib/config_test.go
- framework/logstore/matviews.go
…mid_customerid_and_businessunitid_in_logging
akshaydeo
force-pushed
the
04-06-adds_support_for_tracking_userid_teamid_customerid_and_businessunitid_in_logging
branch
from
d1845e0 to
b26e343
Compare
Contributor
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 (1)
framework/logstore/matviews.go (1)
183-190:⚠️ Potential issue | 🟠 MajorKeep
parent_request_idoff the matview path.
canUseMatViewFilters()no longer excludesSearchFilters.ParentRequestID, butapplyMatViewFilters()still cannot apply that predicate andmv_logs_hourlydoes not carry the column. Requests that only addparent_request_idto otherwise matview-eligible filters will now return unfiltered aggregates.💡 Proposed fix
func canUseMatViewFilters(f SearchFilters) bool { - return f.ContentSearch == "" && + return f.ParentRequestID == "" && + f.ContentSearch == "" && len(f.MetadataFilters) == 0 && len(f.RoutingEngineUsed) == 0 && f.MinLatency == nil && f.MaxLatency == nil && f.MinTokens == nil && f.MaxTokens == nil && f.MinCost == nil && f.MaxCost == nil && !f.MissingCostOnly }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/logstore/matviews.go` around lines 183 - 190, canUseMatViewFilters currently allows filters with ParentRequestID even though applyMatViewFilters and the mv_logs_hourly materialized view can't apply that predicate; update canUseMatViewFilters to also exclude ParentRequestID by returning false when SearchFilters.ParentRequestID is set so requests that only add parent_request_id don't go down the matview path (refer to canUseMatViewFilters, applyMatViewFilters, SearchFilters.ParentRequestID, and mv_logs_hourly).
🧹 Nitpick comments (2)
framework/configstore/rdb.go (1)
3379-3381: Ensure deterministic ordering forpricingOverridesin governance snapshots.For full config snapshot/reconciliation paths, loading without
ORDER BYcan produce nondeterministic slice ordering and noisy diffs across runs/DB engines.💡 Proposed tweak
- if err := s.db.WithContext(ctx).Find(&pricingOverrides).Error; err != nil { + if err := s.db.WithContext(ctx).Order("created_at ASC, id ASC").Find(&pricingOverrides).Error; err != nil { return nil, err }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/configstore/rdb.go` around lines 3379 - 3381, The pricingOverrides query uses s.db.WithContext(ctx).Find(&pricingOverrides) which returns nondeterministic ordering; modify the query in the same function to apply a deterministic ORDER BY (e.g., Order("id ASC") or Order("name ASC") or whatever stable unique key your pricing_overrides table exposes) before Find so snapshots/reconciliations produce stable slices; update the call chain (s.db.WithContext(ctx).Order("...").Find(&pricingOverrides).Error) to enforce a consistent sort.plugins/governance/main.go (1)
339-339: Preferbifrost.Ptr(...)overnew(...)for pointer construction consistency.This keeps pointer creation aligned with repository conventions in Go code.
♻️ Suggested change
- p.isVkMandatory = new(enforceAuthOnInference) + p.isVkMandatory = bifrost.Ptr(enforceAuthOnInference)Based on learnings: In the maximhq/bifrost repository, prefer using
bifrost.Ptr()to create pointers instead of the address operator (and equivalent direct pointer constructions) for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/governance/main.go` at line 339, Replace the direct pointer construction new(enforceAuthOnInference) for p.isVkMandatory with the repository-convention helper by calling bifrost.Ptr(...) so that p.isVkMandatory = bifrost.Ptr(enforceAuthOnInference(...)) — specifically update the assignment to use bifrost.Ptr and the enforceAuthOnInference value to maintain consistency with other pointer constructions (refer to p.isVkMandatory and enforceAuthOnInference in the diff and use bifrost.Ptr as the pointer helper).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@framework/logstore/matviews.go`:
- Around line 885-893: The SELECT is incorrectly exposing weighted averages of
hourly percentiles (SUM(p90_latency * count)/SUM(count)) as p90/p95/p99; replace
those expressions with true percentile aggregation over the raw latency
distribution (e.g., use PostgreSQL percentile_disc(0.9) or percentile_cont(0.9)
WITHIN GROUP (ORDER BY latency) over the underlying rows or over a stored
histogram/quantile state), or remove/rename the p90/p95/p99 columns if raw
latencies are not available; update the q.Select SQL (the block producing
bucket_timestamp, dim_value, avg_lat, p90_lat, p95_lat, p99_lat, total_requests)
to compute percentiles via percentile_disc/percentile_cont (or an equivalent
histogram-merge function) instead of SUM(... * count)/SUM(count), keeping
avg_lat as the weighted avg if desired.
---
Outside diff comments:
In `@framework/logstore/matviews.go`:
- Around line 183-190: canUseMatViewFilters currently allows filters with
ParentRequestID even though applyMatViewFilters and the mv_logs_hourly
materialized view can't apply that predicate; update canUseMatViewFilters to
also exclude ParentRequestID by returning false when
SearchFilters.ParentRequestID is set so requests that only add parent_request_id
don't go down the matview path (refer to canUseMatViewFilters,
applyMatViewFilters, SearchFilters.ParentRequestID, and mv_logs_hourly).
---
Nitpick comments:
In `@framework/configstore/rdb.go`:
- Around line 3379-3381: The pricingOverrides query uses
s.db.WithContext(ctx).Find(&pricingOverrides) which returns nondeterministic
ordering; modify the query in the same function to apply a deterministic ORDER
BY (e.g., Order("id ASC") or Order("name ASC") or whatever stable unique key
your pricing_overrides table exposes) before Find so snapshots/reconciliations
produce stable slices; update the call chain
(s.db.WithContext(ctx).Order("...").Find(&pricingOverrides).Error) to enforce a
consistent sort.
In `@plugins/governance/main.go`:
- Line 339: Replace the direct pointer construction new(enforceAuthOnInference)
for p.isVkMandatory with the repository-convention helper by calling
bifrost.Ptr(...) so that p.isVkMandatory =
bifrost.Ptr(enforceAuthOnInference(...)) — specifically update the assignment to
use bifrost.Ptr and the enforceAuthOnInference value to maintain consistency
with other pointer constructions (refer to p.isVkMandatory and
enforceAuthOnInference in the diff and use bifrost.Ptr as the pointer helper).
🪄 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: 549b863f-602e-4796-981c-c14d53711180
⛔ Files ignored due to path filters (12)
core/go.sumis excluded by!**/*.sumframework/go.sumis excluded by!**/*.sumplugins/governance/go.sumis excluded by!**/*.sumplugins/jsonparser/go.sumis excluded by!**/*.sumplugins/litellmcompat/go.sumis excluded by!**/*.sumplugins/logging/go.sumis excluded by!**/*.sumplugins/maxim/go.sumis excluded by!**/*.sumplugins/mocker/go.sumis excluded by!**/*.sumplugins/otel/go.sumis excluded by!**/*.sumplugins/semanticcache/go.sumis excluded by!**/*.sumplugins/telemetry/go.sumis excluded by!**/*.sumtransports/go.sumis excluded by!**/*.sum
📒 Files selected for processing (41)
core/go.modcore/schemas/bifrost.godocs/openapi/openapi.jsondocs/openapi/openapi.yamldocs/openapi/paths/management/users.yamldocs/openapi/schemas/management/users.yamlframework/configstore/rdb.goframework/configstore/tables/virtualkey.goframework/go.modframework/logstore/matviews.goframework/logstore/migrations.goframework/logstore/postgres.goframework/logstore/rdb.goframework/logstore/store.goframework/logstore/tables.goplugins/governance/go.modplugins/governance/main.goplugins/governance/store.goplugins/jsonparser/go.modplugins/litellmcompat/go.modplugins/logging/go.modplugins/logging/main.goplugins/logging/operations.goplugins/logging/utils.goplugins/logging/writer.goplugins/maxim/go.modplugins/mocker/go.modplugins/otel/go.modplugins/semanticcache/go.modplugins/telemetry/go.modtransports/bifrost-http/handlers/logging.gotransports/bifrost-http/lib/config_test.gotransports/go.modui/app/_fallbacks/enterprise/components/user-rankings/userRankingsTab.tsxui/app/workspace/dashboard/page.tsxui/app/workspace/logs/page.tsxui/app/workspace/logs/sheets/logDetailView.tsxui/app/workspace/virtual-keys/views/virtualKeySheet.tsxui/lib/store/apis/baseApi.tsui/lib/store/apis/logsApi.tsui/lib/types/logs.ts
✅ Files skipped from review due to trivial changes (17)
- plugins/jsonparser/go.mod
- plugins/governance/go.mod
- core/go.mod
- plugins/semanticcache/go.mod
- ui/lib/store/apis/baseApi.ts
- framework/go.mod
- plugins/maxim/go.mod
- plugins/telemetry/go.mod
- plugins/mocker/go.mod
- transports/go.mod
- ui/app/_fallbacks/enterprise/components/user-rankings/userRankingsTab.tsx
- plugins/governance/store.go
- plugins/logging/go.mod
- framework/configstore/tables/virtualkey.go
- docs/openapi/schemas/management/users.yaml
- docs/openapi/openapi.yaml
- plugins/otel/go.mod
🚧 Files skipped from review as they are similar to previous changes (12)
- plugins/litellmcompat/go.mod
- framework/logstore/postgres.go
- ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx
- plugins/logging/main.go
- plugins/logging/operations.go
- ui/app/workspace/logs/sheets/logDetailView.tsx
- framework/logstore/migrations.go
- transports/bifrost-http/handlers/logging.go
- transports/bifrost-http/lib/config_test.go
- plugins/logging/utils.go
- ui/app/workspace/logs/page.tsx
- framework/logstore/rdb.go
Contributor
Author
Merge activity
|
akshaydeo
deleted the
04-06-adds_support_for_tracking_userid_teamid_customerid_and_businessunitid_in_logging
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds governance context tracking to the logging system by introducing business unit, team, customer, and user dimensions for enhanced analytics and filtering capabilities.
Changes
/api/logs/histogram/{cost,tokens,latency}/by-dimension)Type of change
Affected areas
How to test
Validate the governance context tracking and dimension-based analytics:
The database migrations will automatically run on startup to add the new governance columns and recreate materialized views.
Screenshots/Recordings
N/A - Backend API changes only
Breaking changes
The changes are additive and maintain backward compatibility. Existing API endpoints continue to work unchanged.
Related issues
N/A
Security considerations
The governance context data (team, customer, user, business unit IDs) may contain sensitive organizational information. Ensure proper access controls are in place when exposing this data through APIs and that governance plugins properly validate and sanitize these values before setting them in the Bifrost context.
Checklist
docs/contributing/README.mdand followed the guidelines