feat: add cluster-aware log metadata and per-node usage aggregation - #3590
Conversation
|
|
|
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 (7)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR adds cluster node identification and governance metadata support to logging: new context keys, Log model persistence and JSON (de)serialization for budget/rate-limit IDs, DB migration and partial index, a LogStore.GetNodeUsageSince implementation and delegation, and plugin wiring to attach context-derived fields to log entries. ChangesCluster Governance and Node Usage
Sequence Diagram(s) sequenceDiagram
participant LoggerPlugin
participant HybridLogStore
participant RDBLogStore
participant Postgres
LoggerPlugin->>HybridLogStore: persist log entry (ClusterNodeID, BudgetIDsParsed, RateLimitIDsParsed)
HybridLogStore->>RDBLogStore: delegate persistence / GetNodeUsageSince calls
RDBLogStore->>Postgres: SELECT cost,total_tokens,budget_ids,rate_limit_ids WHERE cluster_node_id=... AND timestamp>=...
Postgres->>RDBLogStore: return rows
RDBLogStore->>RDBLogStore: parse JSON, dedupe IDs, aggregate into NodeUsageAggregate
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
Confidence Score: 5/5Safe to merge — all new columns are nullable, the migration is non-blocking, and the aggregation query is additive to the interface. The change is well-scoped: columns default to NULL, no existing log entries are touched, and the new interface method is implemented consistently across RDBLogStore and HybridLogStore. The concurrency concerns raised in prior review rounds have been addressed. The only outstanding observation is a minor metadata asymmetry in the error fallback path that does not affect the core aggregation logic. No files require special attention. Important Files Changed
Reviews (3): Last reviewed commit: "feat: add cluster-aware log metadata and..." | Re-trigger Greptile |
7b9b16d to
76c4f27
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
framework/logstore/migrations.go (1)
2466-2470: ⚡ Quick winMirror this index on the SQLite migration path.
idx_logs_cluster_node_idis only being added through the PostgreSQL-onlyperformanceIndexesbackground path.GetNodeUsageSincewas added onRDBLogStorefor the shared logstore implementation, so SQLite will still full-scanlogsfor that new query unless the non-Postgres migration path also creates an equivalent index.Based on learnings only PostgreSQL and SQLite database dialects are supported in
framework/logstore.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@framework/logstore/migrations.go` around lines 2466 - 2470, Add the same index for SQLite so GetNodeUsageSince on RDBLogStore won’t full-scan: in framework/logstore/migrations.go, locate the SQLite migration path (the migrations array used for the non-Postgres path) and add an entry mirroring idx_logs_cluster_node_id (use the SQLite-compatible CREATE INDEX statement for idx_logs_cluster_node_id on logs(cluster_node_id), handling SQLite partial-index syntax if needed). Ensure the new migration entry is named "idx_logs_cluster_node_id" and aligns with the existing migration pattern so it runs for SQLite alongside performanceIndexes for Postgres.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@framework/logstore/rdb.go`:
- Around line 371-389: The loop that attributes row.Cost and row.TotalTokens
uses budgetIDs and rateLimitIDs directly after sonic.Unmarshal, which
double-counts when the decoded slices contain duplicates; update the handling in
the blocks that read row.BudgetIDs and row.RateLimitIDs to normalize each
decoded slice to unique IDs (e.g., build a temporary map[string]struct{} set
from budgetIDs and rateLimitIDs) and then iterate the set keys to increment
budgetCosts[id] += row.Cost, rateLimitRequests[id]++, and rateLimitTokens[id] +=
row.TotalTokens only once per unique id.
- Around line 346-359: The query scans nullable numeric columns into
non-nullable fields: update the SELECT in the s.db...Model(&Log{}) query (the
call that builds and executes the Find into []logRow) to use COALESCE for
numeric nullable columns, e.g. COALESCE(cost, 0) AS cost and
COALESCE(total_tokens, 0) AS total_tokens so the logRow struct can safely
receive defaults when DB values are NULL; keep budget_ids and rate_limit_ids
as-is.
In `@plugins/logging/main.go`:
- Around line 354-359: SetClusterNodeID currently writes p.clusterNodeID without
synchronization while PostLLMHook reads it concurrently, causing a race; either
(A) protect the write and all reads with the existing mutex p.mu by acquiring
p.mu.Lock()/Unlock() in SetClusterNodeID and reading p.clusterNodeID under p.mu
in PostLLMHook, or (B) change p.clusterNodeID to an atomic.Value and use
Store/Load there and in PostLLMHook; update comments to state the chosen
approach and ensure all accesses to p.clusterNodeID (SetClusterNodeID and
PostLLMHook) follow the same synchronization strategy.
---
Nitpick comments:
In `@framework/logstore/migrations.go`:
- Around line 2466-2470: Add the same index for SQLite so GetNodeUsageSince on
RDBLogStore won’t full-scan: in framework/logstore/migrations.go, locate the
SQLite migration path (the migrations array used for the non-Postgres path) and
add an entry mirroring idx_logs_cluster_node_id (use the SQLite-compatible
CREATE INDEX statement for idx_logs_cluster_node_id on logs(cluster_node_id),
handling SQLite partial-index syntax if needed). Ensure the new migration entry
is named "idx_logs_cluster_node_id" and aligns with the existing migration
pattern so it runs for SQLite alongside performanceIndexes for Postgres.
🪄 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: 9ad6fdf3-a779-4d15-80ee-26e556ed0dcd
📒 Files selected for processing (7)
core/schemas/bifrost.goframework/logstore/hybrid.goframework/logstore/migrations.goframework/logstore/rdb.goframework/logstore/store.goframework/logstore/tables.goplugins/logging/main.go
76c4f27 to
3846895
Compare
Merge activity
|
…3590) ## Summary Add optional cluster metadata columns to the logs table and a per-node usage aggregation query on the LogStore interface. These are foundational hooks for improved governance accuracy in multi-node deployments. ## Changes - 3 new context keys for passing node ID and governance resource IDs through the request pipeline - 3 new nullable columns on the `Log` struct: `cluster_node_id`, `budget_ids`, `rate_limit_ids` - `NodeUsageAggregate` struct and `GetNodeUsageSince` method on the `LogStore` interface for aggregating a node's usage since a given timestamp - Non-blocking migration: column additions are instant (`ALTER TABLE ADD COLUMN`), index built via `CREATE INDEX CONCURRENTLY` in a background goroutine - Logging plugin gains `SetClusterNodeID()` setter and stamps entries with cluster metadata from the request context when set ## Type of change - [ ] Bug fix - [x] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [x] Plugins - [ ] UI (React) - [ ] Docs ## How to test ```sh make build LOCAL=1 make test-core make test-plugins ``` Start with a fresh database and verify migration completes without errors. The new columns are nullable and unused unless explicitly wired by the caller. ## Screenshots/Recordings N/A — no UI changes. ## Breaking changes - [ ] Yes - [x] No New columns are nullable and optional. The `GetNodeUsageSince` method is additive to the `LogStore` interface. Existing log entries are unaffected. ## Related issues N/A ## Security considerations No security implications. New columns store opaque IDs only. ## Checklist - [x] I read `docs/contributing/README.md` and followed the guidelines - [ ] I added/updated tests where appropriate - [x] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
## Summary This PR cuts the `v1.5.11` / `v1.3.11` release across core, framework, and all plugins, and introduces a new Claude skill (`release-checklist`) for pre-release migration safety auditing. ## Changes - **`release-checklist` skill** — Adds `.claude/skills/release-checklist/SKILL.md`, a read-only pre-release audit tool that scans Go-defined database migrations changed in a release for high-scale deadlock/lock-contention risks and boot-time-blocking operations. It produces a structured `PASS`/`WARN`/`FAIL` report with a concrete remediation plan per finding. The skill is designed to grow via an extensible Checks Registry. - **Version bumps** — `core` → `1.5.11`, `framework` → `1.3.11`, `transports` → `1.5.3`, `plugins/governance` → `1.5.11`, `plugins/logging` → `1.5.11`, `plugins/semanticcache` → `1.5.11`, `plugins/otel` → `1.2.11`, `plugins/maxim` → `1.6.11`, `plugins/prompts` → `1.0.11`, and remaining plugins bumped accordingly. - **Changelogs populated** — All per-package changelogs updated with the full set of features and fixes shipping in this release. Key highlights in this release: - Temporary access tokens for scoped, time-limited API access - MCP per-user OAuth flow refactor - Bedrock Mantle inference engine support - Azure Realtime provider with enriched session tracking - Direct access control (DAC) and virtual key rotation - Cluster-aware log metadata and per-node usage aggregation - Feature flag framework - Config-hash-based file value override of DB on restart - Semantic cache plugin rewrite - Numerous streaming stability, Bedrock, Anthropic, and Gemini fixes - AWS SDK and dependency security updates ## Type of change - [ ] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [x] Chore/CI ## Affected areas - [x] Core (Go) - [x] Transports (HTTP) - [x] Providers/Integrations - [x] Plugins - [x] UI (React) - [ ] Docs ## How to test ```sh # Verify version files reflect the new release cat core/version # expect 1.5.11 cat framework/version # expect 1.3.11 cat transports/version # expect 1.5.3 # Core/Transports go test ./... ``` To exercise the new `release-checklist` skill, invoke it via Claude with: ``` /release-checklist origin/dev...HEAD ``` Expected output: a structured report with `PASS`/`WARN`/`FAIL` per check and a Remediation Plan table for any findings. ## Screenshots/Recordings N/A ## Breaking changes - [ ] Yes - [x] No ## Related issues #3603, #3565, #3489, #3334, #3335, #3435, #3554, #3590, #3444, #3198, #3581, #3610, #3599, #3567, #3382, #3461 and others listed in the changelogs. ## Security considerations - AWS SDK and dependency security updates are included (#3461). - `FullyRedacted()` for proxy passwords and `MarshalForStorage()` for `ProxyConfig` prevent partial secret leakage in API responses (#3445). - The `release-checklist` skill is strictly read-only and never modifies files. ## Checklist - [x] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [x] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
…aximhq#3590) ## Summary Add optional cluster metadata columns to the logs table and a per-node usage aggregation query on the LogStore interface. These are foundational hooks for improved governance accuracy in multi-node deployments. ## Changes - 3 new context keys for passing node ID and governance resource IDs through the request pipeline - 3 new nullable columns on the `Log` struct: `cluster_node_id`, `budget_ids`, `rate_limit_ids` - `NodeUsageAggregate` struct and `GetNodeUsageSince` method on the `LogStore` interface for aggregating a node's usage since a given timestamp - Non-blocking migration: column additions are instant (`ALTER TABLE ADD COLUMN`), index built via `CREATE INDEX CONCURRENTLY` in a background goroutine - Logging plugin gains `SetClusterNodeID()` setter and stamps entries with cluster metadata from the request context when set ## Type of change - [ ] Bug fix - [x] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [x] Plugins - [ ] UI (React) - [ ] Docs ## How to test ```sh make build LOCAL=1 make test-core make test-plugins ``` Start with a fresh database and verify migration completes without errors. The new columns are nullable and unused unless explicitly wired by the caller. ## Screenshots/Recordings N/A — no UI changes. ## Breaking changes - [ ] Yes - [x] No New columns are nullable and optional. The `GetNodeUsageSince` method is additive to the `LogStore` interface. Existing log entries are unaffected. ## Related issues N/A ## Security considerations No security implications. New columns store opaque IDs only. ## Checklist - [x] I read `docs/contributing/README.md` and followed the guidelines - [ ] I added/updated tests where appropriate - [x] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
## Summary This PR cuts the `v1.5.11` / `v1.3.11` release across core, framework, and all plugins, and introduces a new Claude skill (`release-checklist`) for pre-release migration safety auditing. ## Changes - **`release-checklist` skill** — Adds `.claude/skills/release-checklist/SKILL.md`, a read-only pre-release audit tool that scans Go-defined database migrations changed in a release for high-scale deadlock/lock-contention risks and boot-time-blocking operations. It produces a structured `PASS`/`WARN`/`FAIL` report with a concrete remediation plan per finding. The skill is designed to grow via an extensible Checks Registry. - **Version bumps** — `core` → `1.5.11`, `framework` → `1.3.11`, `transports` → `1.5.3`, `plugins/governance` → `1.5.11`, `plugins/logging` → `1.5.11`, `plugins/semanticcache` → `1.5.11`, `plugins/otel` → `1.2.11`, `plugins/maxim` → `1.6.11`, `plugins/prompts` → `1.0.11`, and remaining plugins bumped accordingly. - **Changelogs populated** — All per-package changelogs updated with the full set of features and fixes shipping in this release. Key highlights in this release: - Temporary access tokens for scoped, time-limited API access - MCP per-user OAuth flow refactor - Bedrock Mantle inference engine support - Azure Realtime provider with enriched session tracking - Direct access control (DAC) and virtual key rotation - Cluster-aware log metadata and per-node usage aggregation - Feature flag framework - Config-hash-based file value override of DB on restart - Semantic cache plugin rewrite - Numerous streaming stability, Bedrock, Anthropic, and Gemini fixes - AWS SDK and dependency security updates ## Type of change - [ ] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [x] Chore/CI ## Affected areas - [x] Core (Go) - [x] Transports (HTTP) - [x] Providers/Integrations - [x] Plugins - [x] UI (React) - [ ] Docs ## How to test ```sh # Verify version files reflect the new release cat core/version # expect 1.5.11 cat framework/version # expect 1.3.11 cat transports/version # expect 1.5.3 # Core/Transports go test ./... ``` To exercise the new `release-checklist` skill, invoke it via Claude with: ``` /release-checklist origin/dev...HEAD ``` Expected output: a structured report with `PASS`/`WARN`/`FAIL` per check and a Remediation Plan table for any findings. ## Screenshots/Recordings N/A ## Breaking changes - [ ] Yes - [x] No ## Related issues maximhq#3603, maximhq#3565, maximhq#3489, maximhq#3334, maximhq#3335, maximhq#3435, maximhq#3554, maximhq#3590, maximhq#3444, maximhq#3198, maximhq#3581, maximhq#3610, maximhq#3599, maximhq#3567, maximhq#3382, maximhq#3461 and others listed in the changelogs. ## Security considerations - AWS SDK and dependency security updates are included (maximhq#3461). - `FullyRedacted()` for proxy passwords and `MarshalForStorage()` for `ProxyConfig` prevent partial secret leakage in API responses (maximhq#3445). - The `release-checklist` skill is strictly read-only and never modifies files. ## Checklist - [x] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [x] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
…aximhq#3590) ## Summary Add optional cluster metadata columns to the logs table and a per-node usage aggregation query on the LogStore interface. These are foundational hooks for improved governance accuracy in multi-node deployments. ## Changes - 3 new context keys for passing node ID and governance resource IDs through the request pipeline - 3 new nullable columns on the `Log` struct: `cluster_node_id`, `budget_ids`, `rate_limit_ids` - `NodeUsageAggregate` struct and `GetNodeUsageSince` method on the `LogStore` interface for aggregating a node's usage since a given timestamp - Non-blocking migration: column additions are instant (`ALTER TABLE ADD COLUMN`), index built via `CREATE INDEX CONCURRENTLY` in a background goroutine - Logging plugin gains `SetClusterNodeID()` setter and stamps entries with cluster metadata from the request context when set ## Type of change - [ ] Bug fix - [x] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [x] Plugins - [ ] UI (React) - [ ] Docs ## How to test ```sh make build LOCAL=1 make test-core make test-plugins ``` Start with a fresh database and verify migration completes without errors. The new columns are nullable and unused unless explicitly wired by the caller. ## Screenshots/Recordings N/A — no UI changes. ## Breaking changes - [ ] Yes - [x] No New columns are nullable and optional. The `GetNodeUsageSince` method is additive to the `LogStore` interface. Existing log entries are unaffected. ## Related issues N/A ## Security considerations No security implications. New columns store opaque IDs only. ## Checklist - [x] I read `docs/contributing/README.md` and followed the guidelines - [ ] I added/updated tests where appropriate - [x] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
## Summary This PR cuts the `v1.5.11` / `v1.3.11` release across core, framework, and all plugins, and introduces a new Claude skill (`release-checklist`) for pre-release migration safety auditing. ## Changes - **`release-checklist` skill** — Adds `.claude/skills/release-checklist/SKILL.md`, a read-only pre-release audit tool that scans Go-defined database migrations changed in a release for high-scale deadlock/lock-contention risks and boot-time-blocking operations. It produces a structured `PASS`/`WARN`/`FAIL` report with a concrete remediation plan per finding. The skill is designed to grow via an extensible Checks Registry. - **Version bumps** — `core` → `1.5.11`, `framework` → `1.3.11`, `transports` → `1.5.3`, `plugins/governance` → `1.5.11`, `plugins/logging` → `1.5.11`, `plugins/semanticcache` → `1.5.11`, `plugins/otel` → `1.2.11`, `plugins/maxim` → `1.6.11`, `plugins/prompts` → `1.0.11`, and remaining plugins bumped accordingly. - **Changelogs populated** — All per-package changelogs updated with the full set of features and fixes shipping in this release. Key highlights in this release: - Temporary access tokens for scoped, time-limited API access - MCP per-user OAuth flow refactor - Bedrock Mantle inference engine support - Azure Realtime provider with enriched session tracking - Direct access control (DAC) and virtual key rotation - Cluster-aware log metadata and per-node usage aggregation - Feature flag framework - Config-hash-based file value override of DB on restart - Semantic cache plugin rewrite - Numerous streaming stability, Bedrock, Anthropic, and Gemini fixes - AWS SDK and dependency security updates ## Type of change - [ ] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [x] Chore/CI ## Affected areas - [x] Core (Go) - [x] Transports (HTTP) - [x] Providers/Integrations - [x] Plugins - [x] UI (React) - [ ] Docs ## How to test ```sh # Verify version files reflect the new release cat core/version # expect 1.5.11 cat framework/version # expect 1.3.11 cat transports/version # expect 1.5.3 # Core/Transports go test ./... ``` To exercise the new `release-checklist` skill, invoke it via Claude with: ``` /release-checklist origin/dev...HEAD ``` Expected output: a structured report with `PASS`/`WARN`/`FAIL` per check and a Remediation Plan table for any findings. ## Screenshots/Recordings N/A ## Breaking changes - [ ] Yes - [x] No ## Related issues maximhq#3603, maximhq#3565, maximhq#3489, maximhq#3334, maximhq#3335, maximhq#3435, maximhq#3554, maximhq#3590, maximhq#3444, maximhq#3198, maximhq#3581, maximhq#3610, maximhq#3599, maximhq#3567, maximhq#3382, maximhq#3461 and others listed in the changelogs. ## Security considerations - AWS SDK and dependency security updates are included (maximhq#3461). - `FullyRedacted()` for proxy passwords and `MarshalForStorage()` for `ProxyConfig` prevent partial secret leakage in API responses (maximhq#3445). - The `release-checklist` skill is strictly read-only and never modifies files. ## Checklist - [x] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [x] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable

Summary
Add optional cluster metadata columns to the logs table and a per-node usage
aggregation query on the LogStore interface. These are foundational hooks for
improved governance accuracy in multi-node deployments.
Changes
request pipeline
Logstruct:cluster_node_id,budget_ids,rate_limit_idsNodeUsageAggregatestruct andGetNodeUsageSincemethod on theLogStoreinterface for aggregating a node's usage since a given timestamp
(
ALTER TABLE ADD COLUMN), index built viaCREATE INDEX CONCURRENTLYin abackground goroutine
SetClusterNodeID()setter and stamps entries withcluster metadata from the request context when set
Type of change
Affected areas
How to test
Start with a fresh database and verify migration completes without errors. The
new columns are nullable and unused unless explicitly wired by the caller.
Screenshots/Recordings
N/A — no UI changes.
Breaking changes
New columns are nullable and optional. The
GetNodeUsageSincemethod isadditive to the
LogStoreinterface. Existing log entries are unaffected.Related issues
N/A
Security considerations
No security implications. New columns store opaque IDs only.
Checklist
docs/contributing/README.mdand followed the guidelines