feat(usage): add metering ledger#947
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
| } | ||
|
|
||
| function hashApiKey(value) { | ||
| return crypto.createHash('sha256').update(value).digest('hex') |
|
@boxlite-agent review |
law-chain-hot
left a comment
There was a problem hiding this comment.
Reviewed PR #947. I found one ledger completeness risk before this is treated as production-ready metering. I did not duplicate the existing CodeQL SHA-256 warning on the local E2E helper.
| private readonly boxRepository: BoxRepository, | ||
| ) {} | ||
|
|
||
| @OnAsyncEvent({ event: BoxEvents.CREATED }) |
There was a problem hiding this comment.
The ledger currently starts only from future box events, and later recordLifecycleTransition() logs-and-drops listener failures. That leaves two permanent accounting gaps: boxes already STARTED/STOPPED when this deploy lands will have no open usage_period until their next lifecycle event, and any transient DB/Redis failure during a listener run loses that period transition forever. Please add a bootstrap/reconciliation path (or durable retry/outbox) that scans current non-destroyed boxes and ensures exactly one open period per box before relying on this ledger.
| to, | ||
| activePeriods, | ||
| archivedPeriods, | ||
| totals: aggregateUsagePeriods([...activeRows, ...archivedRows], from, to), |
There was a problem hiding this comment.
Totals are computed from rows already capped by take: limit, so /metering underreports whenever overlaps exceed limit/default; verified code path at lines 150-174, migration lint passed, tests blocked by yarn install ENOSPC.
📦 BoxLite review — looks good ·
|
|
@Boxlite review |
| export class Migration1782700000000 implements MigrationInterface { | ||
| name = 'Migration1782700000000' | ||
|
|
||
| public async up(queryRunner: QueryRunner): Promise<void> { |
There was a problem hiding this comment.
🛑 Existing boxes never meter
The migration creates empty usage tables and UsageService only opens periods from future events, so boxes already running/stopped at deploy have no usage_period until a later lifecycle/resource/org event.
| const [activeRows, archivedRows] = await Promise.all([ | ||
| this.periods.find({ | ||
| where: activeWhere, | ||
| order: { startAt: 'DESC' }, | ||
| take: limit, | ||
| }), | ||
| this.archives.find({ | ||
| where: archivedWhere, | ||
| order: { startAt: 'DESC' }, | ||
| take: limit, | ||
| }), |
There was a problem hiding this comment.
getOrganizationMeteringView applies take: limit to both queries, then line 174 aggregates only those rows; orgs with more than limit overlapping periods get undercounted totals.
|
@boxlite-agent review |
|
Superseded by #968, which keeps the Usage lifecycle implementation aligned with the selected upstream design and uses the final BoxLite table names. |
Summary
Reviewed architecture topology
Testing