feat(usage-collector): BigDecimal aggregates, deterministic record ids, op-per-kind guard#4187
Open
capybutler wants to merge 1 commit into
Open
Conversation
📝 WalkthroughWalkthroughThis PR replaces the caller-supplied ChangesDeterministic Identity and Aggregation Overhaul
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Handler
participant Service
participant Plugin
Client->>Handler: POST /records (CreateUsageRecordRequest, no id)
Handler->>Service: create_usage_record(CreateUsageRecord)
Service->>Service: into_usage_record() -> derive_usage_record_id(tenant_id, gts_id, idempotency_key)
Service->>Plugin: create_usage_record(UsageRecord{id,...})
Plugin-->>Service: persisted UsageRecord
Service-->>Handler: UsageRecord
Handler-->>Client: 201 { id, ... }
sequenceDiagram
participant Client
participant Handler
participant Service
participant Query
participant Plugin
Client->>Handler: GET /records/aggregate?op=SUM
Handler->>Service: query_aggregated_usage_records(op, gts_id)
Service->>Plugin: fetch UsageType(gts_id)
Service->>Query: require_op_allowed_for_kind(op, kind)
Query-->>Service: Ok / aggregation_op_not_allowed_for_kind
Service->>Plugin: query_aggregated_usage_records()
Plugin-->>Service: AggregationBucket{value: BigDecimal}
Service-->>Handler: aggregated result
Handler-->>Client: 200 { value: "string" }
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
capybutler
force-pushed
the
usage-collector/aggregation-and-id-fixes
branch
from
db0ecc2 to
10af4df
Compare
…s, op-per-kind guard - Aggregate value is now arbitrary-precision BigDecimal end to end: widen AggregationBucket.value (SDK) and AggregationBucketDto.value (gateway), with a bigdecimal_str_option serde codec so values round-trip as strings. - Record id can no longer be caller-supplied: the create surface across the SDK trait, domain service, local client, and gateway takes a new identity-free CreateUsageRecord (mirrors UsageRecord minus id + status). CreateUsageRecord::into_usage_record is the single choke point that stamps the deterministic UUIDv5 dedup-key derivation and the initial Active status. The record-identity field is renamed uuid -> id. Adds derive_usage_record_id plus ADR-0013 describing the scheme. - Aggregation ops are restricted per record kind: AggregationOp:: is_allowed_for matrix, OP_NOT_ALLOWED_FOR_KIND validation reason and matching error, a require_op_allowed_for_kind query guard, and enforcement before plugin dispatch. - Fix aggregation orderby. Design docs, feature specs, domain model, plugin SPI, SDK trait, and the OpenAPI schema are updated to match. Signed-off-by: capybutler <capybutler@gmail.com>
capybutler
force-pushed
the
usage-collector/aggregation-and-id-fixes
branch
from
10af4df to
2562796
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
gears/system/usage-collector/docs/ADR/0013-deterministic-usage-record-id.md (1)
117-121: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winTraceability list omits two files changed by this same PR layer.
The "Amends" list names
plugin-spi.md,sdk-trait.md,domain-model.md,features/usage-emission.md, PRD, DESIGN,usage-collector-v1.yaml, butDECOMPOSITION.mdandfeatures/event-deactivation.mdare also updated as part of this cohort (per the PR's file list) and aren't listed here.📝 Proposed fix
- Amends: `plugin-spi.md`, `sdk-trait.md`, `domain-model.md`, - `features/usage-emission.md`, PRD, DESIGN, `usage-collector-v1.yaml`. + `features/usage-emission.md`, `features/event-deactivation.md`, + `DECOMPOSITION.md`, PRD, DESIGN, `usage-collector-v1.yaml`.🤖 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 `@gears/system/usage-collector/docs/ADR/0013-deterministic-usage-record-id.md` around lines 117 - 121, The Traceability “Amends” list in the ADR is incomplete because it omits two files updated in the same change set. Update the list in the Traceability section of the deterministic-usage-record-id ADR to include DECOMPOSITION.md and features/event-deactivation.md alongside the existing references so the documented traceability matches the full PR cohort.
🧹 Nitpick comments (1)
gears/system/usage-collector/usage-collector/src/domain/service.rs (1)
1201-1212: 🚀 Performance & Scalability | 🔵 TrivialNew per-request plugin round-trip for op/kind validation.
query_aggregated_usage_recordsnow always issues an extraget_usage_typeSPI call before dispatch, solely to resolvekindfor therequire_op_allowed_for_kindguard. This is a deliberate trade-off to reject invalid(op, kind)pairs before the plugin sees them, and is correctly implemented (singlepluginhandle reused, correct error lift toNotFound/400). Flagging only as an operational note: if aggregation query volume is high, consider whether the resolvedUsageTypecatalog could eventually be cached (mirroring the batch-create catalog-dedup pattern already in this file) to avoid a network round-trip per aggregate call.🤖 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 `@gears/system/usage-collector/usage-collector/src/domain/service.rs` around lines 1201 - 1212, `query_aggregated_usage_records` adds a per-request `get_usage_type` SPI round-trip before `require_op_allowed_for_kind`, which is intentional but may hurt high-volume aggregation paths. If you want to address this note, reuse the existing `plugin` flow in `service.rs` and add a small `UsageType` lookup cache for the resolved catalog entry, following the same dedup/caching approach already used elsewhere in this file for batch-create, so repeated aggregate calls can avoid the extra network call while preserving the current `NotFound`/`400` behavior.
🤖 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.
Outside diff comments:
In `@gears/system/usage-collector/docs/ADR/0013-deterministic-usage-record-id.md`:
- Around line 117-121: The Traceability “Amends” list in the ADR is incomplete
because it omits two files updated in the same change set. Update the list in
the Traceability section of the deterministic-usage-record-id ADR to include
DECOMPOSITION.md and features/event-deactivation.md alongside the existing
references so the documented traceability matches the full PR cohort.
---
Nitpick comments:
In `@gears/system/usage-collector/usage-collector/src/domain/service.rs`:
- Around line 1201-1212: `query_aggregated_usage_records` adds a per-request
`get_usage_type` SPI round-trip before `require_op_allowed_for_kind`, which is
intentional but may hurt high-volume aggregation paths. If you want to address
this note, reuse the existing `plugin` flow in `service.rs` and add a small
`UsageType` lookup cache for the resolved catalog entry, following the same
dedup/caching approach already used elsewhere in this file for batch-create, so
repeated aggregate calls can avoid the extra network call while preserving the
current `NotFound`/`400` behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1c721577-f8f7-4067-b5ec-ef333d439ef8
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (42)
Cargo.tomlgears/system/usage-collector/docs/ADR/0013-deterministic-usage-record-id.mdgears/system/usage-collector/docs/DECOMPOSITION.mdgears/system/usage-collector/docs/DESIGN.mdgears/system/usage-collector/docs/domain-model.mdgears/system/usage-collector/docs/features/event-deactivation.mdgears/system/usage-collector/docs/features/usage-emission.mdgears/system/usage-collector/docs/features/usage-query.mdgears/system/usage-collector/docs/plugin-spi.mdgears/system/usage-collector/docs/sdk-trait.mdgears/system/usage-collector/docs/usage-collector-v1.yamlgears/system/usage-collector/plugins/noop-usage-collector-plugin/src/plugin.rsgears/system/usage-collector/plugins/noop-usage-collector-plugin/src/plugin_tests.rsgears/system/usage-collector/usage-collector-sdk/Cargo.tomlgears/system/usage-collector/usage-collector-sdk/src/api.rsgears/system/usage-collector/usage-collector-sdk/src/error.rsgears/system/usage-collector/usage-collector-sdk/src/id.rsgears/system/usage-collector/usage-collector-sdk/src/id_tests.rsgears/system/usage-collector/usage-collector-sdk/src/lib.rsgears/system/usage-collector/usage-collector-sdk/src/models.rsgears/system/usage-collector/usage-collector-sdk/src/models_tests.rsgears/system/usage-collector/usage-collector-sdk/src/plugin_api.rsgears/system/usage-collector/usage-collector-sdk/src/reason.rsgears/system/usage-collector/usage-collector-sdk/src/reason_tests.rsgears/system/usage-collector/usage-collector-sdk/src/serde_helpers.rsgears/system/usage-collector/usage-collector-sdk/src/serde_helpers_tests.rsgears/system/usage-collector/usage-collector/Cargo.tomlgears/system/usage-collector/usage-collector/src/api/rest/dto.rsgears/system/usage-collector/usage-collector/src/api/rest/dto_tests.rsgears/system/usage-collector/usage-collector/src/api/rest/handlers/usage_records.rsgears/system/usage-collector/usage-collector/src/api/rest/handlers/usage_records_tests.rsgears/system/usage-collector/usage-collector/src/domain/authz_tests.rsgears/system/usage-collector/usage-collector/src/domain/error.rsgears/system/usage-collector/usage-collector/src/domain/error_tests.rsgears/system/usage-collector/usage-collector/src/domain/local_client.rsgears/system/usage-collector/usage-collector/src/domain/query.rsgears/system/usage-collector/usage-collector/src/domain/query_tests.rsgears/system/usage-collector/usage-collector/src/domain/service.rsgears/system/usage-collector/usage-collector/src/domain/service_tests.rsgears/system/usage-collector/usage-collector/src/domain/test_support.rsgears/system/usage-collector/usage-collector/src/domain/validation_tests.rsgears/system/usage-collector/usage-collector/src/infra/sdk_error_mapping_tests.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Consolidates three related changes to the usage-collector gateway and SDK, plus one fix.
Arbitrary-precision aggregate values.
AggregationBucket.value(SDK) andAggregationBucketDto.value(gateway) move from fixed-precisionOption<Decimal>to
Option<BigDecimal>, so aggregates no longer lose precision on large orhigh-scale sums. Values round-trip over the wire as plain-decimal strings (never
scientific notation, never a bare float) via a new
bigdecimal_str_optionserdecodec.
Deterministic, gateway-derived record id. The usage-record id is now derived
on the gateway as a
UUIDv5over the dedup key(tenant_id, gts_id, idempotency_key)(derive_usage_record_id) and is no longer accepted in therequest body. The record-identity field is renamed
uuid→idacross the SDK,DTOs, and OpenAPI schema. ADR-0013 documents the derivation scheme and why the
namespace must never change.
Per-kind aggregation-op restriction. Aggregation operations are constrained
by record kind: an
AggregationOp::is_allowed_formatrix, anOP_NOT_ALLOWED_FOR_KINDvalidation reason and matching SDK error, and arequire_op_allowed_for_kindquery guard that rejects a disallowed op/kindcombination before the request is dispatched to a plugin.
Fix: correct aggregation
orderbyhandling.Design docs, feature specs, domain model, plugin SPI, SDK trait, and the OpenAPI
schema (
usage-collector-v1.yaml) are updated to match.Type of Change
Testing
Documentation
usage-collector-v1.yaml+ gear design/feature docs, ADR-0013)Checklist
cargo clippy)cargo fmt)cargo test)Summary by CodeRabbit
New Features
Bug Fixes
Documentation