Conversation
…error" in logging, telemetry, and analytics queries
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds explicit detection and handling of request-cancelled errors: new BifrostError helper, logging and telemetry emit/record Changes
Sequence DiagramsequenceDiagram
participant Client
participant LLMHandler
participant LoggerPlugin
participant LogStore
participant TelemetryPlugin
participant Prometheus
Client->>LLMHandler: Send request (may be cancelled)
LLMHandler->>LLMHandler: Process request / detect cancellation
LLMHandler->>LoggerPlugin: PostLLMHook (bifrostErr)
LoggerPlugin->>LoggerPlugin: IsRequestCancelled() / inspect Error.Type
alt cancelled
LoggerPlugin->>LogStore: Save log entry (status="cancelled")
LoggerPlugin->>TelemetryPlugin: Notify cancelled
TelemetryPlugin->>Prometheus: Increment bifrost_cancelled_requests_total
else non-cancelled error
LoggerPlugin->>LogStore: Save log entry (status="error"/"success")
LoggerPlugin->>TelemetryPlugin: Notify error or success
TelemetryPlugin->>Prometheus: Increment appropriate metric
end
LogStore->>LogStore: Persist / materialized views aggregate (hourly includes cancelled)
Prometheus->>Prometheus: Record metric
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Confidence Score: 3/5Not safe to merge as-is — two P1 regressions affect live analytics correctness and a filter used for cost backfill. The core status-classification logic is correct throughout logging and telemetry, but two concrete data-correctness regressions remain: (1) the matview-based success rate calculation is now wrong on Postgres, giving different numbers than the raw-SQL path; (2) the MissingCostOnly filter is now broken for all users with cancelled requests. framework/logstore/matviews.go (getStatsFromMatView success rate denominator) and framework/logstore/rdb.go (MissingCostOnly filter) Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
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)
34-46:⚠️ Potential issue | 🟠 MajorMaterialize
cancelled_countbefore widening the view filter.After this change,
countincludes cancelled rows, but the view still only storessuccess_countanderror_count. Every PostgreSQL fast-path query built onmv_logs_hourlynow loses the new status and can no longer reconcile the exposed breakdown with the total. Add acancelled_countaggregate here and thread it through the consumers before expanding the source filter.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/logstore/matviews.go` around lines 34 - 46, The view aggregates in mv_logs_hourly are missing cancelled_count, causing totals to mismatch after widening the WHERE filter; add SUM(CASE WHEN status = 'cancelled' THEN 1 ELSE 0 END) AS cancelled_count alongside success_count and error_count in the SELECT that builds mv_logs_hourly (the SQL block in framework/logstore/matviews.go), then update all consumers and functions that consume mv_logs_hourly (e.g., any code reading success_count, error_count, total or performing reconciliation) to accept and use cancelled_count so totals reconcile correctly before expanding the source filter.
🧹 Nitpick comments (3)
core/schemas/bifrost.go (1)
898-901: MakeIsRequestCancelled()nil-safe.As written, this helper still panics on a nil
*BifrostError, so the older call sites incore/bifrost.gostill can't collapse to a single predicate. Adding the nil check lets this become the one canonical cancellation test.♻️ Proposed fix
func (e *BifrostError) IsRequestCancelled() bool { - return e.Error != nil && e.Error.Type != nil && *e.Error.Type == RequestCancelled + return e != nil && e.Error != nil && e.Error.Type != nil && *e.Error.Type == RequestCancelled }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/schemas/bifrost.go` around lines 898 - 901, The IsRequestCancelled method can panic when called on a nil *BifrostError; update IsRequestCancelled to first check the receiver for nil (e == nil) before accessing e.Error and e.Error.Type, then return false if nil; keep the existing checks for e.Error and e.Error.Type and the final comparison to RequestCancelled so IsRequestCancelled becomes a nil-safe canonical cancellation predicate.plugins/logging/main.go (1)
642-643: UseIsRequestCancelled()instead of repeating nested error-type checks.The same cancellation predicate is duplicated in four locations. Switching to
bifrostErr.IsRequestCancelled()keeps classification logic centralized and aligned with the shared schema helper.Refactor sketch
- if bifrostErr.Error != nil && bifrostErr.Error.Type != nil && *bifrostErr.Error.Type == schemas.RequestCancelled { + if bifrostErr.IsRequestCancelled() { status = "cancelled" } ... - if bifrostErr.Error != nil && bifrostErr.Error.Type != nil && *bifrostErr.Error.Type == schemas.RequestCancelled { + if bifrostErr.IsRequestCancelled() { entry.Status = "cancelled" }Also applies to: 684-685, 730-731, 771-772
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/logging/main.go` around lines 642 - 643, Replace the repeated nested cancellation checks (e.g., if bifrostErr.Error != nil && bifrostErr.Error.Type != nil && *bifrostErr.Error.Type == schemas.RequestCancelled) with the shared predicate method bifrostErr.IsRequestCancelled(); in each block where you currently set status = "cancelled" (the occurrences around the duplicated checks) call bifrostErr.IsRequestCancelled() and set status = "cancelled" when it returns true, removing the duplicated nil/type dereferencing logic so cancellation classification is centralized.plugins/telemetry/main.go (1)
490-494: PreferbifrostErr.IsRequestCancelled()for cancellation classification.Use the shared helper here as well to keep telemetry classification logic aligned with schema-level behavior and avoid duplicated nested checks.
Refactor sketch
- if bifrostErr.Error != nil && bifrostErr.Error.Type != nil && *bifrostErr.Error.Type == schemas.RequestCancelled { + if bifrostErr.IsRequestCancelled() { p.CancelledRequestsTotal.WithLabelValues(errorPromLabelValues...).Inc() } else { p.ErrorRequestsTotal.WithLabelValues(errorPromLabelValues...).Inc() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/telemetry/main.go` around lines 490 - 494, Replace the nested nil/type checks with the shared helper by calling bifrostErr.IsRequestCancelled() to classify cancellations; if that returns true increment p.CancelledRequestsTotal.WithLabelValues(errorPromLabelValues...).Inc(), otherwise increment p.ErrorRequestsTotal.WithLabelValues(errorPromLabelValues...).Inc(); this removes the duplicated nested checks (the current bifrostErr.Error != nil && bifrostErr.Error.Type != nil && *bifrostErr.Error.Type == schemas.RequestCancelled) while preserving the existing label usage and else branch behavior.
🤖 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 26-28: The shared variable completedStatuses currently includes
"cancelled" and is being reused by analytics endpoints (SearchStats,
MCPToolLogStats, HistogramBucket, MCPHistogramBucket, ModelUsageStats) which
only return success/error buckets, causing totals to mismatch; fix by
introducing a separate filter variable (e.g., completedStatusesWithCancelled)
for queries that should include cancelled and keep the existing
analytics-specific slice as []string{"success","error"} for those functions,
then update callers: have functions that need cancelled use
completedStatusesWithCancelled and leave SearchStats, MCPToolLogStats,
HistogramBucket, MCPHistogramBucket, and ModelUsageStats using the two-status
slice so count == success + error.
In `@plugins/logging/main.go`:
- Around line 730-732: The "cancelled" classification set via entry.Status =
"cancelled" when bifrostErr.Error.Type == schemas.RequestCancelled is later
overwritten by the passthrough error branch; update the passthrough
error-setting logic so it preserves an existing cancelled status: when you set
entry.Status = "error" in the streaming passthrough path (the block that
inspects passthrough/4xx/5xx), first check if entry.Status == "cancelled" (or
re-check bifrostErr.Error.Type == schemas.RequestCancelled) and skip overwriting
if so; apply the same guard to both passthrough error sites mentioned so
cancelled remains authoritative.
---
Outside diff comments:
In `@framework/logstore/matviews.go`:
- Around line 34-46: The view aggregates in mv_logs_hourly are missing
cancelled_count, causing totals to mismatch after widening the WHERE filter; add
SUM(CASE WHEN status = 'cancelled' THEN 1 ELSE 0 END) AS cancelled_count
alongside success_count and error_count in the SELECT that builds mv_logs_hourly
(the SQL block in framework/logstore/matviews.go), then update all consumers and
functions that consume mv_logs_hourly (e.g., any code reading success_count,
error_count, total or performing reconciliation) to accept and use
cancelled_count so totals reconcile correctly before expanding the source
filter.
---
Nitpick comments:
In `@core/schemas/bifrost.go`:
- Around line 898-901: The IsRequestCancelled method can panic when called on a
nil *BifrostError; update IsRequestCancelled to first check the receiver for nil
(e == nil) before accessing e.Error and e.Error.Type, then return false if nil;
keep the existing checks for e.Error and e.Error.Type and the final comparison
to RequestCancelled so IsRequestCancelled becomes a nil-safe canonical
cancellation predicate.
In `@plugins/logging/main.go`:
- Around line 642-643: Replace the repeated nested cancellation checks (e.g., if
bifrostErr.Error != nil && bifrostErr.Error.Type != nil &&
*bifrostErr.Error.Type == schemas.RequestCancelled) with the shared predicate
method bifrostErr.IsRequestCancelled(); in each block where you currently set
status = "cancelled" (the occurrences around the duplicated checks) call
bifrostErr.IsRequestCancelled() and set status = "cancelled" when it returns
true, removing the duplicated nil/type dereferencing logic so cancellation
classification is centralized.
In `@plugins/telemetry/main.go`:
- Around line 490-494: Replace the nested nil/type checks with the shared helper
by calling bifrostErr.IsRequestCancelled() to classify cancellations; if that
returns true increment
p.CancelledRequestsTotal.WithLabelValues(errorPromLabelValues...).Inc(),
otherwise increment
p.ErrorRequestsTotal.WithLabelValues(errorPromLabelValues...).Inc(); this
removes the duplicated nested checks (the current bifrostErr.Error != nil &&
bifrostErr.Error.Type != nil && *bifrostErr.Error.Type ==
schemas.RequestCancelled) while preserving the existing label usage and else
branch behavior.
🪄 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: 31c14967-8b8a-4cc7-94ea-06104c776e37
📒 Files selected for processing (6)
core/schemas/bifrost.goframework/logstore/matviews.goframework/logstore/rdb.goframework/logstore/tables.goplugins/logging/main.goplugins/telemetry/main.go
…elled from analytics queries, and add guard for status preservation
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
|
|
The base branch was changed.
d36cd75 to
5e4bfb7
Compare
6711ce3 to
a1beab5
Compare
e389df7 to
a65fce4
Compare
fa15f50 to
ca190fc
Compare
ac30a53 to
7c66b20
Compare
3 participants
Summary
Briefly explain the purpose of this PR and the problem it solves.
Changes
Type of change
Affected areas
How to test
Describe the steps to validate this change. Include commands and expected outcomes.
If adding new configs or environment variables, document them here.
Screenshots/Recordings
If UI changes, add before/after screenshots or short clips.
Breaking changes
If yes, describe impact and migration instructions.
Related issues
Link related issues and discussions. Example: Closes #123
Security considerations
Note any security implications (auth, secrets, PII, sandboxing, etc.).
Checklist
docs/contributing/README.mdand followed the guidelines