feat(db): add resource_usage_logs table with repository functions - #245
funds0033-cmyk wants to merge 3 commits into
Conversation
|
@funds0033-cmyk Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
|
Warning Review limit reached
More reviews will be available in 44 minutes and 5 seconds. Learn how PR review limits work. To continue reviewing without waiting, enable usage-based billing in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a new ChangesResource Usage Logs
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/db/repositories.ts`:
- Around line 870-918: The resource usage log persistence path in
insertResourceUsageLog currently stores mixed recorded_at formats because it
falls back to SQLite CURRENT_TIMESTAMP while callers may pass ISO-8601 strings,
which breaks TEXT-based sorting and filtering in getResourceUsageLogs and
getLatestResourceUsageLog. Update insertResourceUsageLog to normalize both the
default and any supplied log.recorded_at into one fixed-width UTC format before
calling db.prepare(...).run(...), and make the same default expression
consistent in the resource_usage_logs schema and migration. Add a regression
test that inserts one row with omitted recorded_at and another with an explicit
ISO timestamp on the same day to verify ordering and since filtering.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 8dc7a50f-a3f9-47fe-9742-7f42a17d6b2a
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
docs/pr-164.mdsrc/db/migrations/001_resource_usage_logs.sqlsrc/db/repositories.tssrc/db/schema.sqltests/db/resource_usage_logs.test.ts
📜 Review details
⚠️ CI failures not shown inline (2)
GitHub Actions: CI Pipeline / build-and-test (22.x): feat(db): add resource_usage_logs table with repository functions
Conclusion: failure
##[group]Run npm run lint
�[36;1mnpm run lint�[0m
shell: /usr/bin/bash -e {0}
##[endgroup]
> sorokeep@0.1.2 lint
> eslint src/ tests/
/home/runner/work/sorokeep/sorokeep/src/alerts/slack.ts
##[warning] 66:15 warning 'resourceType' is assigned a value but never used `@typescript-eslint/no-unused-vars`
/home/runner/work/sorokeep/sorokeep/src/commands/alerts.ts
##[warning] 11:5 warning 'getResourceAlertConfigsForContract' is defined but never used `@typescript-eslint/no-unused-vars`
##[warning] 12:5 warning 'deleteResourceAlertConfig' is defined but never used `@typescript-eslint/no-unused-vars`
/home/runner/work/sorokeep/sorokeep/src/core/discovery.ts
92:9 warning Unused eslint-disable directive (no problems were reported from 'no-constant-condition')
/home/runner/work/sorokeep/sorokeep/src/core/introspection.ts
##[warning] 10:5 warning 'db' is defined but never used `@typescript-eslint/no-unused-vars`
##[warning] 11:5 warning 'network' is defined but never used `@typescript-eslint/no-unused-vars`
##[warning] 12:5 warning 'rpcUrl' is defined but never used `@typescript-eslint/no-unused-vars`
/home/runner/work/sorokeep/sorokeep/src/core/rent_projection.ts
##[error] 128:18 error An interface declaring no members is equivalent to its supertype `@typescript-eslint/no-empty-object-type`
GitHub Actions: CI Pipeline / 0_build-and-test (22.x).txt: feat(db): add resource_usage_logs table with repository functions
Conclusion: failure
##[group]Run npm run lint
�[36;1mnpm run lint�[0m
shell: /usr/bin/bash -e {0}
##[endgroup]
> sorokeep@0.1.2 lint
> eslint src/ tests/
/home/runner/work/sorokeep/sorokeep/src/alerts/slack.ts
##[warning] 66:15 warning 'resourceType' is assigned a value but never used `@typescript-eslint/no-unused-vars`
/home/runner/work/sorokeep/sorokeep/src/commands/alerts.ts
##[warning] 11:5 warning 'getResourceAlertConfigsForContract' is defined but never used `@typescript-eslint/no-unused-vars`
##[warning] 12:5 warning 'deleteResourceAlertConfig' is defined but never used `@typescript-eslint/no-unused-vars`
/home/runner/work/sorokeep/sorokeep/src/core/discovery.ts
92:9 warning Unused eslint-disable directive (no problems were reported from 'no-constant-condition')
/home/runner/work/sorokeep/sorokeep/src/core/introspection.ts
##[warning] 10:5 warning 'db' is defined but never used `@typescript-eslint/no-unused-vars`
##[warning] 11:5 warning 'network' is defined but never used `@typescript-eslint/no-unused-vars`
##[warning] 12:5 warning 'rpcUrl' is defined but never used `@typescript-eslint/no-unused-vars`
/home/runner/work/sorokeep/sorokeep/src/core/rent_projection.ts
##[error] 128:18 error An interface declaring no members is equivalent to its supertype `@typescript-eslint/no-empty-object-type`
| /** Optional ISO-8601 timestamp; defaults to CURRENT_TIMESTAMP when omitted. */ | ||
| recorded_at?: string; | ||
| }, | ||
| ): number { | ||
| const result = db.prepare(` | ||
| INSERT INTO resource_usage_logs ( | ||
| contract_id, | ||
| cpu_insns, | ||
| mem_bytes, | ||
| fee_instructions, | ||
| fee_read_ledger_entries, | ||
| fee_write_ledger_entries, | ||
| fee_read_bytes, | ||
| fee_write_bytes, | ||
| fee_transaction_size, | ||
| fee_historical_ledger, | ||
| fee_rent_ledger, | ||
| fee_refundable, | ||
| recorded_at | ||
| ) VALUES ( | ||
| @contract_id, | ||
| @cpu_insns, | ||
| @mem_bytes, | ||
| @fee_instructions, | ||
| @fee_read_ledger_entries, | ||
| @fee_write_ledger_entries, | ||
| @fee_read_bytes, | ||
| @fee_write_bytes, | ||
| @fee_transaction_size, | ||
| @fee_historical_ledger, | ||
| @fee_rent_ledger, | ||
| @fee_refundable, | ||
| COALESCE(@recorded_at, CURRENT_TIMESTAMP) | ||
| ) | ||
| `).run({ | ||
| contract_id: log.contract_id, | ||
| cpu_insns: log.cpu_insns, | ||
| mem_bytes: log.mem_bytes, | ||
| fee_instructions: log.fee_instructions ?? null, | ||
| fee_read_ledger_entries: log.fee_read_ledger_entries ?? null, | ||
| fee_write_ledger_entries: log.fee_write_ledger_entries ?? null, | ||
| fee_read_bytes: log.fee_read_bytes ?? null, | ||
| fee_write_bytes: log.fee_write_bytes ?? null, | ||
| fee_transaction_size: log.fee_transaction_size ?? null, | ||
| fee_historical_ledger: log.fee_historical_ledger ?? null, | ||
| fee_rent_ledger: log.fee_rent_ledger ?? null, | ||
| fee_refundable: log.fee_refundable ?? null, | ||
| recorded_at: log.recorded_at ?? null, | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Store recorded_at in one canonical format.
Line 902 falls back to SQLite's CURRENT_TIMESTAMP (YYYY-MM-DD HH:MM:SS), while this API also accepts ISO-8601 strings like 2025-01-15T12:00:00.000Z. getResourceUsageLogs() and getLatestResourceUsageLog() then compare and sort recorded_at as raw TEXT, so mixed formats misorder same-day rows and can make since drop later auto-stamped entries. Normalize both defaults and caller-supplied values to one fixed-width UTC format before persisting them.
Possible fix
- /** Optional ISO-8601 timestamp; defaults to CURRENT_TIMESTAMP when omitted. */
+ /** Optional canonical UTC ISO-8601 timestamp. */
recorded_at?: string;
@@
- COALESCE(`@recorded_at`, CURRENT_TIMESTAMP)
+ COALESCE(`@recorded_at`, strftime('%Y-%m-%dT%H:%M:%fZ', 'now'))Apply the same default expression in src/db/schema.sql and src/db/migrations/001_resource_usage_logs.sql, and add a regression test that mixes an omitted recorded_at with an explicit ISO timestamp on the same date.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** Optional ISO-8601 timestamp; defaults to CURRENT_TIMESTAMP when omitted. */ | |
| recorded_at?: string; | |
| }, | |
| ): number { | |
| const result = db.prepare(` | |
| INSERT INTO resource_usage_logs ( | |
| contract_id, | |
| cpu_insns, | |
| mem_bytes, | |
| fee_instructions, | |
| fee_read_ledger_entries, | |
| fee_write_ledger_entries, | |
| fee_read_bytes, | |
| fee_write_bytes, | |
| fee_transaction_size, | |
| fee_historical_ledger, | |
| fee_rent_ledger, | |
| fee_refundable, | |
| recorded_at | |
| ) VALUES ( | |
| @contract_id, | |
| @cpu_insns, | |
| @mem_bytes, | |
| @fee_instructions, | |
| @fee_read_ledger_entries, | |
| @fee_write_ledger_entries, | |
| @fee_read_bytes, | |
| @fee_write_bytes, | |
| @fee_transaction_size, | |
| @fee_historical_ledger, | |
| @fee_rent_ledger, | |
| @fee_refundable, | |
| COALESCE(@recorded_at, CURRENT_TIMESTAMP) | |
| ) | |
| `).run({ | |
| contract_id: log.contract_id, | |
| cpu_insns: log.cpu_insns, | |
| mem_bytes: log.mem_bytes, | |
| fee_instructions: log.fee_instructions ?? null, | |
| fee_read_ledger_entries: log.fee_read_ledger_entries ?? null, | |
| fee_write_ledger_entries: log.fee_write_ledger_entries ?? null, | |
| fee_read_bytes: log.fee_read_bytes ?? null, | |
| fee_write_bytes: log.fee_write_bytes ?? null, | |
| fee_transaction_size: log.fee_transaction_size ?? null, | |
| fee_historical_ledger: log.fee_historical_ledger ?? null, | |
| fee_rent_ledger: log.fee_rent_ledger ?? null, | |
| fee_refundable: log.fee_refundable ?? null, | |
| recorded_at: log.recorded_at ?? null, | |
| }); | |
| /** Optional canonical UTC ISO-8601 timestamp. */ | |
| recorded_at?: string; | |
| }, | |
| ): number { | |
| const result = db.prepare(` | |
| INSERT INTO resource_usage_logs ( | |
| contract_id, | |
| cpu_insns, | |
| mem_bytes, | |
| fee_instructions, | |
| fee_read_ledger_entries, | |
| fee_write_ledger_entries, | |
| fee_read_bytes, | |
| fee_write_bytes, | |
| fee_transaction_size, | |
| fee_historical_ledger, | |
| fee_rent_ledger, | |
| fee_refundable, | |
| recorded_at | |
| ) VALUES ( | |
| `@contract_id`, | |
| `@cpu_insns`, | |
| `@mem_bytes`, | |
| `@fee_instructions`, | |
| `@fee_read_ledger_entries`, | |
| `@fee_write_ledger_entries`, | |
| `@fee_read_bytes`, | |
| `@fee_write_bytes`, | |
| `@fee_transaction_size`, | |
| `@fee_historical_ledger`, | |
| `@fee_rent_ledger`, | |
| `@fee_refundable`, | |
| COALESCE(`@recorded_at`, strftime('%Y-%m-%dT%H:%M:%fZ', 'now')) | |
| ) | |
| `).run({ | |
| contract_id: log.contract_id, | |
| cpu_insns: log.cpu_insns, | |
| mem_bytes: log.mem_bytes, | |
| fee_instructions: log.fee_instructions ?? null, | |
| fee_read_ledger_entries: log.fee_read_ledger_entries ?? null, | |
| fee_write_ledger_entries: log.fee_write_ledger_entries ?? null, | |
| fee_read_bytes: log.fee_read_bytes ?? null, | |
| fee_write_bytes: log.fee_write_bytes ?? null, | |
| fee_transaction_size: log.fee_transaction_size ?? null, | |
| fee_historical_ledger: log.fee_historical_ledger ?? null, | |
| fee_rent_ledger: log.fee_rent_ledger ?? null, | |
| fee_refundable: log.fee_refundable ?? null, | |
| recorded_at: log.recorded_at ?? null, | |
| }); |
🤖 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 `@src/db/repositories.ts` around lines 870 - 918, The resource usage log
persistence path in insertResourceUsageLog currently stores mixed recorded_at
formats because it falls back to SQLite CURRENT_TIMESTAMP while callers may pass
ISO-8601 strings, which breaks TEXT-based sorting and filtering in
getResourceUsageLogs and getLatestResourceUsageLog. Update
insertResourceUsageLog to normalize both the default and any supplied
log.recorded_at into one fixed-width UTC format before calling
db.prepare(...).run(...), and make the same default expression consistent in the
resource_usage_logs schema and migration. Add a regression test that inserts one
row with omitted recorded_at and another with an explicit ISO timestamp on the
same day to verify ordering and since filtering.
1 participant
Closes #164