fix: stats for cancelled requests - #4930
Conversation
📝 WalkthroughWalkthroughThis PR adds cancelled as a tracked terminal log status across matview and non-matview analytics, response types, tests, and dashboard/logs charts. Backend queries and matview aggregation now include cancelled counts, and the UI surfaces the new metric in charts, legends, tooltips, and CSV exports. ChangesCancelled status analytics and UI
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
tejas ghatte seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ui/app/workspace/logs/views/logsVolumeChart.tsx (1)
161-167: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueColor duplicated as a magic string instead of reusing the shared constant.
fill="#a1a1aa"/bg-zinc-400duplicateCHART_COLORS.cancelledfromchartUtils.ts. This mirrors the pre-existing pattern in this file (success/error are also hardcoded here rather than usingCHART_COLORS), so it's not a new inconsistency introduced by this PR, but consolidating on the shared constant would prevent future drift if the cancelled color is updated in one place but not the other.Also applies to: 225-292, 389-392, 460-469
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/app/workspace/logs/views/logsVolumeChart.tsx` around lines 161 - 167, The cancelled chart color is hardcoded in logsVolumeChart instead of reusing the shared CHART_COLORS.cancelled constant, which can drift from chartUtils.ts. Update the cancelled indicator/legend and any related cancelled series rendering in logsVolumeChart to reference CHART_COLORS.cancelled rather than inline color values, following the existing chart color usage patterns in this file.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@ui/app/workspace/logs/views/logsVolumeChart.tsx`:
- Around line 161-167: The cancelled chart color is hardcoded in logsVolumeChart
instead of reusing the shared CHART_COLORS.cancelled constant, which can drift
from chartUtils.ts. Update the cancelled indicator/legend and any related
cancelled series rendering in logsVolumeChart to reference
CHART_COLORS.cancelled rather than inline color values, following the existing
chart color usage patterns in this file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1705d35f-7eeb-4526-ae8b-1a3d821527dd
📒 Files selected for processing (12)
framework/logstore/matviews.goframework/logstore/multi_team_filter_test.goframework/logstore/rdb.goframework/logstore/rdb_perf_test.goframework/logstore/tables.goui/app/workspace/dashboard/components/charts/logVolumeChart.tsxui/app/workspace/dashboard/components/charts/modelUsageChart.tsxui/app/workspace/dashboard/components/overviewTab.tsxui/app/workspace/dashboard/utils/chartUtils.tsui/app/workspace/dashboard/utils/exportUtils.tsui/app/workspace/logs/views/logsVolumeChart.tsxui/lib/types/logs.ts
aa77187 to
953105a
Compare
Merge activity
|
The base branch was changed.
| if usage == nil { | ||
| return 0 | ||
| } | ||
| if usage.InputTokensDetails != nil { |
There was a problem hiding this comment.
Arent text n image handled differently?
There was a problem hiding this comment.
but the input tokens for tier would be combination of all input tokens right
## Summary Updates the stream cancellation E2E test to recognize `cancelled` as a valid terminal status for cancelled requests, introduced in #4930. Previously, only `error` (and `success` for non-streaming) were accepted as valid cancel outcomes. This aligns the test with the dedicated cancellation status while retaining backward compatibility with older builds that logged `error`. ## Changes - `statusIsCancelOutcome` now accepts `cancelled` in addition to `error` for both streaming and non-streaming cancel paths - Updated inline comments to reflect that `cancelled` is the expected status on current builds, with `error` retained as a fallback for pre-#4930 builds - Removed a duplicate `go.sum` entry for `github.com/tidwall/pretty v1.2.0` ## Type of change - [x] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [ ] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test Run the stream cancellation E2E test against a current build and verify that cancelled requests with `status=cancelled` no longer produce unexpected cost-check failures. ```sh node tests/e2e/api/runners/run-stream-cancellation.mjs ``` Expected: cost checks pass for both streaming and non-streaming cancel scenarios, with `cancelled`, `error`, or `success` (non-stream only) all accepted as valid outcomes. ## Breaking changes - [x] No ## Related issues Closes #4930 ## Security considerations None. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [ ] I added/updated tests where appropriate - [ ] I updated documentation where needed - [ ] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
## Summary
Adds `cancelled` as a first-class terminal log status alongside `success` and `error`. Previously, cancelled requests were excluded from all aggregate queries, histograms, and materialized views. This change ensures cancelled requests are tracked, counted, and surfaced in charts and exports.
## Changes
- Introduced a `terminalLogStatuses` constant (`["success", "error", "cancelled"]`) to replace all scattered inline `[]string{"success", "error"}` slices across query filters in `rdb.go` and `matviews.go`.
- Added `cancelled_count` to the `mv_logs_hourly` materialized view definition and included `cancelled` in the `WHERE status IN (...)` clause so the view captures cancelled requests.
- Added `cancelled_count` to `mvLogsHourlyRequiredColumns` to enforce schema compatibility checks on startup.
- Added `canUseMatViewStatusFilter` to gate matview usage: non-terminal statuses (e.g. `processing`) force the raw query path, while terminal statuses (including `cancelled`) remain matview-eligible.
- Extended `HistogramBucket` and `ModelUsageStats` structs with a `Cancelled` field, and propagated it through all histogram query paths (raw DB and matview) for `GetHistogram`, `GetModelHistogram`, and their matview equivalents.
- Updated CSV export helpers (`overviewVolumeToCSV`, `overviewModelUsageToCSV`) to include cancelled counts.
- Updated UI chart components (`logVolumeChart.tsx`, `modelUsageChart.tsx`, `logsVolumeChart.tsx`, `overviewTab.tsx`) to render a stacked `cancelled` segment using `zinc-400` (`#a1a1aa`), display it in tooltips and legends, and handle missing values with a `?? 0` fallback.
- Added `cancelled` to the `CHART_COLORS` palette and updated the `HistogramBucket`, `ModelUsageStats`, and `MCPHistogramBucket` TypeScript interfaces.
- Added `TestCancelledStatusIncludedInLogAggregates` to verify that cancelled rows appear in `SearchLogs`, `GetStats`, and `GetHistogram` results while `processing` rows are excluded from terminal aggregates.
## Type of change
- [ ] Bug fix
- [x] Feature
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [x] Core (Go)
- [ ] Transports (HTTP)
- [ ] Providers/Integrations
- [ ] Plugins
- [x] UI (React)
- [ ] Docs
## How to test
```sh
# Core/Transports
go test ./framework/logstore/...
# UI
cd ui
pnpm i
pnpm build
```
The new test `TestCancelledStatusIncludedInLogAggregates` covers the following scenarios:
- A `cancelled` log appears in `SearchLogs` when filtering by `status=cancelled`.
- `GetStats` with no filter counts all 4 rows as total requests but only 3 (terminal) as the cache-hit denominator.
- `GetStats` filtered to `cancelled` returns correct latency and a zero success rate.
- `GetHistogram` filtered to `cancelled` returns a bucket with `Count=1`, `Success=0`, `Error=0`, `Cancelled=1`.
Because `mv_logs_hourly` gains a new `cancelled_count` column, the materialized view must be recreated on first startup. The existing schema version/migration logic handles this via `mvLogsHourlyRequiredColumns`.
## Breaking changes
- [x] Yes
- [ ] No
The `mv_logs_hourly` materialized view schema changes (new `cancelled_count` column). On startup the store will detect the missing column and recreate the view. No manual migration is required, but there will be a brief period during view recreation where matview-backed queries fall back to raw table scans.
API consumers reading `HistogramBucket` or `ModelUsageStats` JSON will now receive an additional `cancelled` field. This is additive and backwards-compatible for consumers that ignore unknown fields.
## Related issues
## Security considerations
None. No new auth surfaces, secrets, or PII handling introduced.
## Checklist
- [ ] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [ ] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [ ] I verified the CI pipeline passes locally if applicable
## Summary
Adds `cancelled` as a first-class terminal log status alongside `success` and `error`. Previously, cancelled requests were excluded from all aggregate queries, histograms, and materialized views. This change ensures cancelled requests are tracked, counted, and surfaced in charts and exports.
## Changes
- Introduced a `terminalLogStatuses` constant (`["success", "error", "cancelled"]`) to replace all scattered inline `[]string{"success", "error"}` slices across query filters in `rdb.go` and `matviews.go`.
- Added `cancelled_count` to the `mv_logs_hourly` materialized view definition and included `cancelled` in the `WHERE status IN (...)` clause so the view captures cancelled requests.
- Added `cancelled_count` to `mvLogsHourlyRequiredColumns` to enforce schema compatibility checks on startup.
- Added `canUseMatViewStatusFilter` to gate matview usage: non-terminal statuses (e.g. `processing`) force the raw query path, while terminal statuses (including `cancelled`) remain matview-eligible.
- Extended `HistogramBucket` and `ModelUsageStats` structs with a `Cancelled` field, and propagated it through all histogram query paths (raw DB and matview) for `GetHistogram`, `GetModelHistogram`, and their matview equivalents.
- Updated CSV export helpers (`overviewVolumeToCSV`, `overviewModelUsageToCSV`) to include cancelled counts.
- Updated UI chart components (`logVolumeChart.tsx`, `modelUsageChart.tsx`, `logsVolumeChart.tsx`, `overviewTab.tsx`) to render a stacked `cancelled` segment using `zinc-400` (`#a1a1aa`), display it in tooltips and legends, and handle missing values with a `?? 0` fallback.
- Added `cancelled` to the `CHART_COLORS` palette and updated the `HistogramBucket`, `ModelUsageStats`, and `MCPHistogramBucket` TypeScript interfaces.
- Added `TestCancelledStatusIncludedInLogAggregates` to verify that cancelled rows appear in `SearchLogs`, `GetStats`, and `GetHistogram` results while `processing` rows are excluded from terminal aggregates.
## Type of change
- [ ] Bug fix
- [x] Feature
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [x] Core (Go)
- [ ] Transports (HTTP)
- [ ] Providers/Integrations
- [ ] Plugins
- [x] UI (React)
- [ ] Docs
## How to test
```sh
# Core/Transports
go test ./framework/logstore/...
# UI
cd ui
pnpm i
pnpm build
```
The new test `TestCancelledStatusIncludedInLogAggregates` covers the following scenarios:
- A `cancelled` log appears in `SearchLogs` when filtering by `status=cancelled`.
- `GetStats` with no filter counts all 4 rows as total requests but only 3 (terminal) as the cache-hit denominator.
- `GetStats` filtered to `cancelled` returns correct latency and a zero success rate.
- `GetHistogram` filtered to `cancelled` returns a bucket with `Count=1`, `Success=0`, `Error=0`, `Cancelled=1`.
Because `mv_logs_hourly` gains a new `cancelled_count` column, the materialized view must be recreated on first startup. The existing schema version/migration logic handles this via `mvLogsHourlyRequiredColumns`.
## Breaking changes
- [x] Yes
- [ ] No
The `mv_logs_hourly` materialized view schema changes (new `cancelled_count` column). On startup the store will detect the missing column and recreate the view. No manual migration is required, but there will be a brief period during view recreation where matview-backed queries fall back to raw table scans.
API consumers reading `HistogramBucket` or `ModelUsageStats` JSON will now receive an additional `cancelled` field. This is additive and backwards-compatible for consumers that ignore unknown fields.
## Related issues
## Security considerations
None. No new auth surfaces, secrets, or PII handling introduced.
## Checklist
- [ ] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [ ] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [ ] I verified the CI pipeline passes locally if applicable

Summary
Adds
cancelledas a first-class terminal log status alongsidesuccessanderror. Previously, cancelled requests were excluded from all aggregate queries, histograms, and materialized views. This change ensures cancelled requests are tracked, counted, and surfaced in charts and exports.Changes
terminalLogStatusesconstant (["success", "error", "cancelled"]) to replace all scattered inline[]string{"success", "error"}slices across query filters inrdb.goandmatviews.go.cancelled_countto themv_logs_hourlymaterialized view definition and includedcancelledin theWHERE status IN (...)clause so the view captures cancelled requests.cancelled_counttomvLogsHourlyRequiredColumnsto enforce schema compatibility checks on startup.canUseMatViewStatusFilterto gate matview usage: non-terminal statuses (e.g.processing) force the raw query path, while terminal statuses (includingcancelled) remain matview-eligible.HistogramBucketandModelUsageStatsstructs with aCancelledfield, and propagated it through all histogram query paths (raw DB and matview) forGetHistogram,GetModelHistogram, and their matview equivalents.overviewVolumeToCSV,overviewModelUsageToCSV) to include cancelled counts.logVolumeChart.tsx,modelUsageChart.tsx,logsVolumeChart.tsx,overviewTab.tsx) to render a stackedcancelledsegment usingzinc-400(#a1a1aa), display it in tooltips and legends, and handle missing values with a?? 0fallback.cancelledto theCHART_COLORSpalette and updated theHistogramBucket,ModelUsageStats, andMCPHistogramBucketTypeScript interfaces.TestCancelledStatusIncludedInLogAggregatesto verify that cancelled rows appear inSearchLogs,GetStats, andGetHistogramresults whileprocessingrows are excluded from terminal aggregates.Type of change
Affected areas
How to test
The new test
TestCancelledStatusIncludedInLogAggregatescovers the following scenarios:cancelledlog appears inSearchLogswhen filtering bystatus=cancelled.GetStatswith no filter counts all 4 rows as total requests but only 3 (terminal) as the cache-hit denominator.GetStatsfiltered tocancelledreturns correct latency and a zero success rate.GetHistogramfiltered tocancelledreturns a bucket withCount=1,Success=0,Error=0,Cancelled=1.Because
mv_logs_hourlygains a newcancelled_countcolumn, the materialized view must be recreated on first startup. The existing schema version/migration logic handles this viamvLogsHourlyRequiredColumns.Breaking changes
The
mv_logs_hourlymaterialized view schema changes (newcancelled_countcolumn). On startup the store will detect the missing column and recreate the view. No manual migration is required, but there will be a brief period during view recreation where matview-backed queries fall back to raw table scans.API consumers reading
HistogramBucketorModelUsageStatsJSON will now receive an additionalcancelledfield. This is additive and backwards-compatible for consumers that ignore unknown fields.Related issues
Security considerations
None. No new auth surfaces, secrets, or PII handling introduced.
Checklist
docs/contributing/README.mdand followed the guidelines