Refactor/the entire thing - #40
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR replaces the SDK_CALL event kind with BASIC_USAGE, introduces structured metrics (including input-cache metrics and provider) and a metrics Zod schema, splits polymorphic events into per-type tables, and updates insert, query, and pricing handlers for ClickHouse and Postgres to persist and aggregate JSON metrics. ChangesEvent Storage Refactor: SDK_CALL → BASIC_USAGE with Cache Metrics
Estimated code review effort: Possibly Related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/storage/adapter/postgres/handlers/addAiTokenUsage.ts (1)
98-98:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAggregation key missing
providermay merge events incorrectly.The aggregation key is
${userId}:${model}but events with the same user and model but different providers will be merged together, with the first provider being retained. This could lead to incorrect attribution of token usage to the wrong provider.Proposed fix
- const key = `${event_data.userId}:${event_data.data.model}`; + const key = `${event_data.userId}:${event_data.data.model}:${event_data.data.provider}`;🤖 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/storage/adapter/postgres/handlers/addAiTokenUsage.ts` at line 98, The aggregation key built in addAiTokenUsage (const key = `${event_data.userId}:${event_data.data.model}`) omits the provider, causing events with the same user and model but different providers to collapse; update the key to include event_data.data.provider (e.g., `${event_data.userId}:${event_data.data.model}:${event_data.data.provider}`) wherever the key is used so provider-specific usage is tracked and attributed correctly, and adjust any lookup/merge logic that relies on the old key format.src/zod/event.ts (1)
53-89:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEnforce non-negative cache debit values before returning parsed event
Line 53 accepts negative raw amounts, and Line 85-89 can also resolve to negative values via tag/expr. This allows negative
inputCacheDebitAmountinto storage/billing aggregation.Suggested fix
- inputcacheamount: z.number(), + inputcacheamount: z.number().min(0), @@ } else { inputCacheDebitAmount = v.inputcacheamount; } + if (inputCacheDebitAmount < 0) { + throw new Error("inputCacheDebitAmount must be non-negative"); + }As per coding guidelines, "Use Zod schemas for all request validation; catch ZodError and convert to domain errors".
🤖 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/zod/event.ts` around lines 53 - 89, After computing inputCacheDebitAmount (from fetchTagAmount/parseAndEvaluateExpr or v.inputcacheamount) ensure it is non-negative by adding a check after the computation that throws a z.ZodError (construct a ZodIssue indicating path 'inputcacheamount' and message like "inputCacheDebitAmount must be non-negative") when inputCacheDebitAmount < 0 so the invalid value is rejected by the schema transform; reference the variables/functions inputCacheDebitAmount, fetchTagAmount, parseAndEvaluateExpr and the async .transform block so the validation happens before returning the parsed AITokenUsageEventData.src/storage/adapter/clickhouse/handlers/addAiTokenUsage.ts (1)
92-109:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winAggregation key drops provider, causing cross-provider data corruption.
On Lines 92/109, events are keyed by
userId:modelbut now persistprovider. If a batch includes the same model from different providers, amounts are merged and attributed to one provider incorrectly.Targeted fix
- const key = `${event_data.userId}:${event_data.data.model}`; + const key = `${event_data.userId}:${event_data.data.model}:${event_data.data.provider}`;🤖 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/storage/adapter/clickhouse/handlers/addAiTokenUsage.ts` around lines 92 - 109, The aggregation key currently built as `${event_data.userId}:${event_data.data.model}` drops provider and causes merges across providers; update the key construction (the variable key used with aggregationMap) to include provider (e.g., `${event_data.userId}:${event_data.data.model}:${event_data.data.provider}`) and ensure all lookups/sets against aggregationMap use that new key so stored objects (created in the else branch and updated in the existing branch) keep provider-specific aggregates and reported_timestamp logic remains unchanged.src/storage/adapter/clickhouse/schema.ts (1)
19-45:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftExisting ClickHouse tables are not migrated for new columns.
Using only
CREATE TABLE IF NOT EXISTSwon’t addapi_key_id(Line 38) or the newprovider/metricscolumns on already-created tables, so inserts can fail after deploy.Suggested migration direction
export async function runClickHouseMigrations(): Promise<void> { const client = getClickHouseDB(); await client.command({ query: BASIC_USAGE_EVENTS_TABLE }); logger.lifecycle("ClickHouse: basic_usage_events table ensured"); await client.command({ query: AI_TOKEN_USAGE_EVENTS_TABLE }); logger.lifecycle("ClickHouse: ai_token_usage_events table ensured"); await client.command({ query: PAYMENT_EVENTS_TABLE }); logger.lifecycle("ClickHouse: payment_events table ensured"); + + // Backward-compatible schema evolution for existing deployments + await client.command({ + query: + "ALTER TABLE ai_token_usage_events ADD COLUMN IF NOT EXISTS provider String AFTER model", + }); + await client.command({ + query: + "ALTER TABLE ai_token_usage_events ADD COLUMN IF NOT EXISTS metrics String AFTER provider", + }); + await client.command({ + query: + "ALTER TABLE payment_events ADD COLUMN IF NOT EXISTS api_key_id Nullable(String) AFTER user_id", + }); }Also applies to: 56-57
🤖 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/storage/adapter/clickhouse/schema.ts` around lines 19 - 45, The CREATE TABLE IF NOT EXISTS definitions (AI_TOKEN_USAGE_EVENTS_TABLE and PAYMENT_EVENTS_TABLE) won't add new columns to existing ClickHouse tables; add an explicit migration that runs ALTER TABLE ... ADD COLUMN IF NOT EXISTS for the missing columns (api_key_id on payment_events, and provider and metrics on ai_token_usage_events, using the same types Nullable(String)/String as in the CREATEs) or include these ALTERs in startup migration logic so existing tables gain those columns before inserts occur; reference AI_TOKEN_USAGE_EVENTS_TABLE and PAYMENT_EVENTS_TABLE and ensure column types and nullability match the CREATE definitions.
🧹 Nitpick comments (1)
src/storage/adapter/postgres/handlers/queryEvents.ts (1)
1-393: ⚖️ Poor tradeoffConsider removing dead code rather than commenting it out.
The entire query handler is commented out since queries have migrated to ClickHouse. Commented-out code adds noise and maintenance burden. Consider either deleting this file or adding a clear TODO comment explaining when/if this code should be restored.
🤖 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/storage/adapter/postgres/handlers/queryEvents.ts` around lines 1 - 393, The file contains a large block of commented-out Postgres query logic (including PG_FIELDS, handleQueryEvents, queryListForType, handleListQuery, handleAggregationQuery, resolveAggCol, buildConditions, buildSelect, getEventTypes, etc.); remove this dead code entirely (delete the file) or, if you must keep it for future restoration, replace the commented block with a short top-level TODO comment stating why it was removed and when it can be restored (e.g., "Postgres query handlers removed — ClickHouse now used; restore only if migrating back to Postgres") and ensure exported symbols like handleQueryEvents are not left commented but removed/updated to avoid stale exports.
🤖 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/routes/gRPC/events/streamEvents.ts`:
- Around line 41-47: Replace the raw console.log calls that print req.toObject()
and eventSkeleton in streamEvents.ts with the project's structured logging
helpers: call logOperationInfo to record non-sensitive, structured fields (e.g.,
request id, event type, user id) extracted from req.toObject() or the parsed
streamEventSchema result, and call logOperationError if parsing or downstream
processing fails; do not log full objects or raw payloads—select and redact
sensitive fields before passing them to logOperationInfo/logOperationError so
the logs conform to the project's contract.
In `@src/storage/adapter/clickhouse/handlers/addAiTokenUsage.ts`:
- Around line 65-79: The validation for inputCacheTokens and
inputCacheDebitAmount currently only rejects negative numbers but allows
NaN/Infinity; update the checks in addAiTokenUsage handler to ensure each value
is a finite non-negative number (use Number.isFinite or global isFinite + typeof
=== "number") before proceeding, and throw the existing
StorageError.insertFailed (preserving the user context via event_data.userId)
with a clear message when the value is not a finite non-negative number; refer
to the variables inputCacheTokens and inputCacheDebitAmount in your changes.
In `@src/storage/adapter/clickhouse/handlers/priceRequestAiTokenUsage.ts`:
- Around line 50-51: The code currently falls back to the raw lastBilled string
when DateTime.fromSQL fails, which can blow up when binding to a DateTime64
parameter; change the fallback so params.lastBilled is set to null (not the raw
string) when lastBilledDt.isValid is false, and update the query to treat a null
param as "no lower bound" (e.g. use an SQL clause like "WHERE (timestamp >=
{lastBilled:DateTime64(3,'UTC')} OR {lastBilled} IS NULL)" or COALESCE-based
logic). Specifically, in the priceRequestAiTokenUsage handler replace the
existing assignment using lastBilledDt/isValid/toClickHouseDateTime so the else
branch assigns null, and ensure the query that consumes params.lastBilled
handles null as "no lower bound".
In `@src/storage/adapter/clickhouse/handlers/priceRequestSdkCall.ts`:
- Around line 48-49: The current code binds a raw unparseable lastBilled value
into params.lastBilled (using DateTime.fromSQL -> toClickHouseDateTime) which
can break DateTime64 queries; change the logic in the handler so that after
computing lastBilledDt = DateTime.fromSQL(lastBilled, { zone: 'utc' }) you only
set params.lastBilled = toClickHouseDateTime(lastBilledDt) when
lastBilledDt.isValid is true, and when invalid remove/omit params.lastBilled (or
follow the existing no-lastBilled query path) instead of assigning the raw
lastBilled string so the SDK uses the no-`lastBilled` branch.
In `@src/storage/adapter/clickhouse/handlers/queryEvents.ts`:
- Around line 21-60: CH_FIELDS is missing an entry for ai_token_usage_events
which causes buildSelectColumns to fall back to '*' and breaks UNION ALL column
compatibility; add an ai_token_usage_events object to CH_FIELDS mirroring the
same keys used for basic_usage_events/payment_events (eventId, eventType,
userId, apiKeyId, reportedTimestamp, ingestedTimestamp, sdkCallType,
debitAmount, model, inputTokens, outputTokens, inputDebitAmount,
outputDebitAmount, inputCacheTokens, inputCacheDebitAmount, creditAmount,
provider) and provide appropriate select (and where where applicable)
expressions (e.g., eventId: "toString(id)", eventType: "'SDK_CALL'" or
"'PAYMENT'" as needed, reportedTimestamp: "toString(reported_timestamp)", etc.)
so getTablesForRequest + buildSelectColumns produce consistent column shapes for
UNIONs and preserve alias-based ordering like reportedTimestamp.
In `@src/storage/adapter/postgres/handlers/addPayment.ts`:
- Around line 77-87: The catch in addPayment.ts is re-wrapping a
previously-thrown StorageError.emptyResult as StorageError.insertFailed; update
the catch so that if the caught error is already a StorageError (e.g., e
instanceof StorageError or a type check for StorageError.emptyResult) you
rethrow it unchanged, otherwise wrap non-StorageError exceptions with
StorageError.insertFailed("Failed to insert payment event", ...). Ensure this
change is applied around the try/catch that surrounds the result check and the
return of { id: result.id } so the original emptyResult error and its context
are preserved.
In `@src/storage/adapter/postgres/handlers/addSdkCall.ts`:
- Around line 51-61: The empty-result error thrown by StorageError.emptyResult
in addSdkCall (the check that throws "SDK call insert returned no ID") is being
caught and re-wrapped by the catch that creates StorageError.insertFailed;
update the handler so StorageError.emptyResult is propagated unchanged: either
move the null/empty check outside the try block or, inside the catch for the
function handling SDK call insert, detect if the caught error is an instance of
StorageError (or specifically StorageError.emptyResult) and rethrow it
immediately before wrapping other errors with StorageError.insertFailed.
Reference addSdkCall, the result null-check that throws
StorageError.emptyResult, and the catch that currently throws
StorageError.insertFailed.
In `@src/storage/adapter/postgres/postgres.ts`:
- Around line 113-116: The current stub in PostgresAdapter.async query(request:
QueryRequest): Promise<QueryResponse> returns a string and breaks the
QueryResponse contract; replace the stub with an explicit non-implementation
error or delegate to the real handler. Specifically, in the query method remove
`return "smth" as any;` and either throw a clear Error like `throw new
Error("PostgresAdapter.query not implemented");` or call and return the real
implementation (e.g., `return await handleQueryEvents(request);`) so the method
returns a valid QueryResponse shape and avoids using `any`.
In `@src/zod/metrics.ts`:
- Around line 9-13: The debit_amount schema in src/zod/metrics.ts uses
z.number().int() for fields input, input_cache, and output which will reject
fractional values; update the debit_amount object (fields input, input_cache,
output) to accept fractional numbers (e.g., use z.number() to match the types
used in src/zod/event.ts for inputamount, inputcacheamount, and outputamount) so
metrics validation accepts non-integer debits.
---
Outside diff comments:
In `@src/storage/adapter/clickhouse/handlers/addAiTokenUsage.ts`:
- Around line 92-109: The aggregation key currently built as
`${event_data.userId}:${event_data.data.model}` drops provider and causes merges
across providers; update the key construction (the variable key used with
aggregationMap) to include provider (e.g.,
`${event_data.userId}:${event_data.data.model}:${event_data.data.provider}`) and
ensure all lookups/sets against aggregationMap use that new key so stored
objects (created in the else branch and updated in the existing branch) keep
provider-specific aggregates and reported_timestamp logic remains unchanged.
In `@src/storage/adapter/clickhouse/schema.ts`:
- Around line 19-45: The CREATE TABLE IF NOT EXISTS definitions
(AI_TOKEN_USAGE_EVENTS_TABLE and PAYMENT_EVENTS_TABLE) won't add new columns to
existing ClickHouse tables; add an explicit migration that runs ALTER TABLE ...
ADD COLUMN IF NOT EXISTS for the missing columns (api_key_id on payment_events,
and provider and metrics on ai_token_usage_events, using the same types
Nullable(String)/String as in the CREATEs) or include these ALTERs in startup
migration logic so existing tables gain those columns before inserts occur;
reference AI_TOKEN_USAGE_EVENTS_TABLE and PAYMENT_EVENTS_TABLE and ensure column
types and nullability match the CREATE definitions.
In `@src/storage/adapter/postgres/handlers/addAiTokenUsage.ts`:
- Line 98: The aggregation key built in addAiTokenUsage (const key =
`${event_data.userId}:${event_data.data.model}`) omits the provider, causing
events with the same user and model but different providers to collapse; update
the key to include event_data.data.provider (e.g.,
`${event_data.userId}:${event_data.data.model}:${event_data.data.provider}`)
wherever the key is used so provider-specific usage is tracked and attributed
correctly, and adjust any lookup/merge logic that relies on the old key format.
In `@src/zod/event.ts`:
- Around line 53-89: After computing inputCacheDebitAmount (from
fetchTagAmount/parseAndEvaluateExpr or v.inputcacheamount) ensure it is
non-negative by adding a check after the computation that throws a z.ZodError
(construct a ZodIssue indicating path 'inputcacheamount' and message like
"inputCacheDebitAmount must be non-negative") when inputCacheDebitAmount < 0 so
the invalid value is rejected by the schema transform; reference the
variables/functions inputCacheDebitAmount, fetchTagAmount, parseAndEvaluateExpr
and the async .transform block so the validation happens before returning the
parsed AITokenUsageEventData.
---
Nitpick comments:
In `@src/storage/adapter/postgres/handlers/queryEvents.ts`:
- Around line 1-393: The file contains a large block of commented-out Postgres
query logic (including PG_FIELDS, handleQueryEvents, queryListForType,
handleListQuery, handleAggregationQuery, resolveAggCol, buildConditions,
buildSelect, getEventTypes, etc.); remove this dead code entirely (delete the
file) or, if you must keep it for future restoration, replace the commented
block with a short top-level TODO comment stating why it was removed and when it
can be restored (e.g., "Postgres query handlers removed — ClickHouse now used;
restore only if migrating back to Postgres") and ensure exported symbols like
handleQueryEvents are not left commented but removed/updated to avoid stale
exports.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a0e1f267-2861-4d00-b753-955b97b81499
⛔ Files ignored due to path filters (6)
src/gen/data/v1/data_pb.d.tsis excluded by!**/gen/**src/gen/data/v1/data_pb.jsis excluded by!**/gen/**src/gen/event/v1/event_pb.d.tsis excluded by!**/gen/**src/gen/event/v1/event_pb.jsis excluded by!**/gen/**src/gen/query/v1/query_pb.d.tsis excluded by!**/gen/**src/gen/query/v1/query_pb.jsis excluded by!**/gen/**
📒 Files selected for processing (27)
protosrc/factory/EventStorageAdapterFactory.tssrc/interface/event/Event.tssrc/interface/storage/Storage.tssrc/routes/gRPC/events/streamEvents.tssrc/routes/http/registerWebhookRoutes.tssrc/storage/adapter/clickhouse/handlers/addAiTokenUsage.tssrc/storage/adapter/clickhouse/handlers/addPayment.tssrc/storage/adapter/clickhouse/handlers/addSdkCall.tssrc/storage/adapter/clickhouse/handlers/priceRequestAiTokenUsage.tssrc/storage/adapter/clickhouse/handlers/priceRequestSdkCall.tssrc/storage/adapter/clickhouse/handlers/queryEvents.tssrc/storage/adapter/clickhouse/schema.tssrc/storage/adapter/postgres/handlers/addAiTokenUsage.tssrc/storage/adapter/postgres/handlers/addEventUtils.tssrc/storage/adapter/postgres/handlers/addPayment.tssrc/storage/adapter/postgres/handlers/addSdkCall.tssrc/storage/adapter/postgres/handlers/index.tssrc/storage/adapter/postgres/handlers/priceRequest.tssrc/storage/adapter/postgres/handlers/priceRequestAiTokenUsage.tssrc/storage/adapter/postgres/handlers/priceRequestSdkCall.tssrc/storage/adapter/postgres/handlers/queryEvents.tssrc/storage/adapter/postgres/postgres.tssrc/storage/db/postgres/schema.tssrc/utils/parseExpr.tssrc/zod/event.tssrc/zod/metrics.ts
💤 Files with no reviewable changes (2)
- src/utils/parseExpr.ts
- src/storage/adapter/postgres/handlers/addEventUtils.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/storage/adapter/clickhouse/handlers/priceRequestAiTokenUsage.ts (1)
48-58: ⚡ Quick winSimplify by flattening the conditional and eliminating duplicate queries.
Lines 54 and 57 are identical, and the nested structure can be flattened. Building the
lastBilledclause conditionally removes duplication and makes the logic clearer.♻️ Proposed refactor
- if (lastBilled) { - const lastBilledDt = DateTime.fromSQL(lastBilled, { zone: 'utc' }); - if (lastBilledDt.isValid) { - query = `SELECT sum(JSONExtractInt(metrics, 'debit_amount', 'input') + JSONExtractInt(metrics, 'debit_amount', 'input_cache') + JSONExtractInt(metrics, 'debit_amount', 'output')) as total FROM ai_token_usage_events WHERE user_id = {userId:String} AND mode = {mode:String} AND reported_timestamp > {lastBilled:DateTime64(3, 'UTC')} AND reported_timestamp < {before:DateTime64(3, 'UTC')}`; - params.lastBilled = toClickHouseDateTime(lastBilledDt); - } else { - query = `SELECT sum(JSONExtractInt(metrics, 'debit_amount', 'input') + JSONExtractInt(metrics, 'debit_amount', 'input_cache') + JSONExtractInt(metrics, 'debit_amount', 'output')) as total FROM ai_token_usage_events WHERE user_id = {userId:String} AND mode = {mode:String} AND reported_timestamp < {before:DateTime64(3, 'UTC')}`; - } - } else { - query = `SELECT sum(JSONExtractInt(metrics, 'debit_amount', 'input') + JSONExtractInt(metrics, 'debit_amount', 'input_cache') + JSONExtractInt(metrics, 'debit_amount', 'output')) as total FROM ai_token_usage_events WHERE user_id = {userId:String} AND mode = {mode:String} AND reported_timestamp < {before:DateTime64(3, 'UTC')}`; - } + const lastBilledDt = lastBilled ? DateTime.fromSQL(lastBilled, { zone: 'utc' }) : null; + const hasValidLastBilled = lastBilledDt?.isValid ?? false; + + const sumExpr = `sum(JSONExtractInt(metrics, 'debit_amount', 'input') + JSONExtractInt(metrics, 'debit_amount', 'input_cache') + JSONExtractInt(metrics, 'debit_amount', 'output'))`; + const lastBilledClause = hasValidLastBilled ? `AND reported_timestamp > {lastBilled:DateTime64(3, 'UTC')}` : ''; + + query = `SELECT ${sumExpr} as total FROM ai_token_usage_events WHERE user_id = {userId:String} AND mode = {mode:String} ${lastBilledClause} AND reported_timestamp < {before:DateTime64(3, 'UTC')}`; + + if (hasValidLastBilled && lastBilledDt) { + params.lastBilled = toClickHouseDateTime(lastBilledDt); + }🤖 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/storage/adapter/clickhouse/handlers/priceRequestAiTokenUsage.ts` around lines 48 - 58, The code duplicates the same SELECT query in multiple branches; flatten by building a single base query for ai_token_usage_events and then conditionally append the reported_timestamp > {lastBilled...} clause only if lastBilled is present and lastBilledDt.isValid; set params.lastBilled = toClickHouseDateTime(lastBilledDt) only when that clause is appended and always include the common params.userId, params.mode and params.before. Update references in priceRequestAiTokenUsage.ts to use the single query variable and remove the duplicate query strings in the nested if/else.
🤖 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 `@src/storage/adapter/clickhouse/handlers/priceRequestAiTokenUsage.ts`:
- Around line 48-58: The code duplicates the same SELECT query in multiple
branches; flatten by building a single base query for ai_token_usage_events and
then conditionally append the reported_timestamp > {lastBilled...} clause only
if lastBilled is present and lastBilledDt.isValid; set params.lastBilled =
toClickHouseDateTime(lastBilledDt) only when that clause is appended and always
include the common params.userId, params.mode and params.before. Update
references in priceRequestAiTokenUsage.ts to use the single query variable and
remove the duplicate query strings in the nested if/else.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2e9ea512-e9f6-47e6-868d-0bae7e8e7979
📒 Files selected for processing (4)
src/routes/gRPC/events/streamEvents.tssrc/storage/adapter/clickhouse/handlers/priceRequestAiTokenUsage.tssrc/storage/adapter/clickhouse/handlers/priceRequestSdkCall.tssrc/storage/adapter/clickhouse/handlers/queryEvents.ts
✅ Files skipped from review due to trivial changes (1)
- src/routes/gRPC/events/streamEvents.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/storage/adapter/clickhouse/handlers/priceRequestSdkCall.ts
- src/storage/adapter/clickhouse/handlers/queryEvents.ts
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/storage/adapter/postgres/handlers/addAiTokenUsage.ts (1)
99-124:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t aggregate different providers into the same row.
Line 99 keys the batch by
userId:model, but the persisted row also includesproviderandmetadata. If the same user/model appears from multiple providers in one batch, this merges them and stores the first provider/metadata for the whole sum, which silently corrupts attribution. Includeproviderin the aggregation key at minimum, and avoid merging rows with conflicting metadata.🤖 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/storage/adapter/postgres/handlers/addAiTokenUsage.ts` around lines 99 - 124, The aggregation currently keys by `${event_data.userId}:${event_data.data.model}` and thus merges different providers/metadata into one entry; change the key to include provider (e.g., `${event_data.userId}:${event_data.data.model}:${event_data.data.provider}`) and ensure when checking/creating entries in aggregationMap you use that new key; additionally, when metadata can differ, avoid silently merging by either including a stable serialization of metadata in the key or by detecting mismatched metadata on an existing entry and creating a separate aggregation entry (do not overwrite the existing provider/metadata), and preserve the reported_timestamp update logic (compare reported_timestamp with existing.reported_timestamp) as before.
🤖 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/routes/gRPC/events/registerEvent.ts`:
- Line 36: Replace the raw console.log(req.toObject()) in the registerEvent
handler with a structured, sanitized logger call: import and use
logOperationInfo from errors/logger, pass a clear operation name (e.g.,
"registerEvent") and a minimal context object (e.g., eventId, eventType, source)
derived from req.toObject() but excluding sensitive payload fields; if an error
path exists, use logOperationError similarly. Ensure no full request object is
logged and that the log call follows the project contract (operation name +
small context).
In `@src/storage/adapter/postgres/handlers/addAiTokenUsage.ts`:
- Around line 143-183: The metricsSchema.parse call is executed outside the try
block (via aiTokenUsageValues) so a ZodError can escape; move the construction
of aiTokenUsageValues (including metricsSchema.parse(...)) inside the try before
calling txn.insert(aiTokenUsageEventsTable). Add a separate catch branch for
ZodError to wrap it into StorageError.insertFailed (with a clear message like
"Invalid metrics for AI token usage event") and rethrow, and keep your existing
catch for other errors that wraps them via StorageError.insertFailed around the
txn.insert(...) operation; retain the returning({ id: aiTokenUsageEventsTable.id
}) and the check that inserted[0].id exists.
---
Outside diff comments:
In `@src/storage/adapter/postgres/handlers/addAiTokenUsage.ts`:
- Around line 99-124: The aggregation currently keys by
`${event_data.userId}:${event_data.data.model}` and thus merges different
providers/metadata into one entry; change the key to include provider (e.g.,
`${event_data.userId}:${event_data.data.model}:${event_data.data.provider}`) and
ensure when checking/creating entries in aggregationMap you use that new key;
additionally, when metadata can differ, avoid silently merging by either
including a stable serialization of metadata in the key or by detecting
mismatched metadata on an existing entry and creating a separate aggregation
entry (do not overwrite the existing provider/metadata), and preserve the
reported_timestamp update logic (compare reported_timestamp with
existing.reported_timestamp) as before.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2abe699c-0535-4ab2-8abf-8f3c475d60fa
⛔ Files ignored due to path filters (4)
src/gen/event/v1/event_pb.d.tsis excluded by!**/gen/**src/gen/event/v1/event_pb.jsis excluded by!**/gen/**src/gen/query/v1/query_pb.d.tsis excluded by!**/gen/**src/gen/query/v1/query_pb.jsis excluded by!**/gen/**
📒 Files selected for processing (12)
src/interface/event/Event.tssrc/interface/storage/Storage.tssrc/routes/gRPC/events/registerEvent.tssrc/routes/gRPC/query/queryEvents.tssrc/storage/adapter/clickhouse/handlers/addAiTokenUsage.tssrc/storage/adapter/clickhouse/handlers/addSdkCall.tssrc/storage/adapter/clickhouse/handlers/queryEvents.tssrc/storage/adapter/clickhouse/schema.tssrc/storage/adapter/postgres/handlers/addAiTokenUsage.tssrc/storage/adapter/postgres/handlers/addSdkCall.tssrc/storage/db/postgres/schema.tssrc/zod/event.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- src/interface/event/Event.ts
- src/storage/adapter/postgres/handlers/addSdkCall.ts
- src/storage/adapter/clickhouse/handlers/addSdkCall.ts
- src/interface/storage/Storage.ts
- src/storage/adapter/clickhouse/handlers/addAiTokenUsage.ts
- src/storage/adapter/clickhouse/schema.ts
- src/storage/adapter/clickhouse/handlers/queryEvents.ts
- src/storage/db/postgres/schema.ts
- src/zod/event.ts
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/zod/event.ts (1)
30-41:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMove
metadataJSON parsing into Zod validation.Malformed
metadatacurrently throws a rawSyntaxErrorhere, so it skips the normal Zod/domain-error path and turns bad client input into an internal error. It also accepts non-object JSON despite theRecord<string, unknown>contract.Suggested direction
+const metadataSchema = z + .string() + .transform((value, ctx): Record<string, unknown> => { + try { + const parsed: unknown = JSON.parse(value); + if (parsed == null || Array.isArray(parsed) || typeof parsed !== "object") { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + message: "metadata must be a JSON object", + }); + return {} as Record<string, unknown>; + } + return parsed as Record<string, unknown>; + } catch { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + message: "metadata must be valid JSON", + }); + return {} as Record<string, unknown>; + } + }) + .optional(); + const BasicUsageDataSchema: z.ZodType<BasicUsageEventData> = z .object({ ... - metadata: z.string().optional(), + metadata: metadataSchema, }) .transform(async (v): Promise<BasicUsageEventData> => { ... - return { basicUsageType: v.sdkcalltype, debitAmount, metadata: v.metadata ? JSON.parse(v.metadata) as Record<string, unknown> : undefined }; + return { basicUsageType: v.sdkcalltype, debitAmount, metadata: v.metadata }; }); const AITokenUsageDataSchema: z.ZodType<AITokenUsageEventData> = z .object({ ... - metadata: z.string().optional(), + metadata: metadataSchema, }) ... - metadata: v.metadata ? JSON.parse(v.metadata) as Record<string, unknown> : undefined, + metadata: v.metadata,As per coding guidelines, "Use Zod schemas for all request validation; catch ZodError and convert to domain errors".
Also applies to: 60-115
🤖 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/zod/event.ts` around lines 30 - 41, Replace the runtime JSON.parse in the transform with Zod-level parsing/validation: change the metadata field definition (currently metadata: z.string().optional()) to a preprocessing schema like z.preprocess((val) => { if (typeof val === 'string') { try { return JSON.parse(val); } catch (e) { throw new Error('Invalid JSON for metadata'); } } return val; }, z.record(z.unknown()).optional()), and remove the JSON.parse(...) call inside the transform so the transform uses the already-validated metadata as a Record<string, unknown> or undefined; this ensures malformed JSON becomes a ZodError and non-object JSON is rejected according to the Record<string, unknown>() contract.
🧹 Nitpick comments (2)
src/storage/adapter/clickhouse/handlers/priceRequestSdkCall.ts (2)
10-87: 💤 Low valueConsider renaming function to match the new event type.
The function is still named
handlePriceRequestSdkCallbut now handlesBASIC_USAGEevents (queriesbasic_usage_eventstable, error message referencesBASIC_USAGE). Consider renaming tohandlePriceRequestBasicUsagefor consistency with the refactor.🤖 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/storage/adapter/clickhouse/handlers/priceRequestSdkCall.ts` around lines 10 - 87, Rename the exported function handlePriceRequestSdkCall to handlePriceRequestBasicUsage and update all internal and external references (imports/exports, tests, call sites) accordingly; keep the same signature and behavior, update any JSDoc/comments to reference BASIC_USAGE/basic_usage_events if present, and ensure error construction that mentions StorageError.priceCalculationFailed and StorageError.invalidData still use the new function name where applicable so names are consistent across the codebase.
74-81: 💤 Low valueAvoid
anyin error type checking.The coding guidelines specify avoiding
any. Consider using a type guard or more explicit typing.Suggested fix
} catch (e) { if ( e && typeof e === "object" && "type" in e && - (e as any).name === "StorageError" + "name" in e && + (e as { name: string }).name === "StorageError" ) { throw e; }🤖 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/storage/adapter/clickhouse/handlers/priceRequestSdkCall.ts` around lines 74 - 81, Replace the inline cast to any by introducing a proper type guard and using it in the existing error-check block: add a function like isStorageError(error: unknown): error is StorageError that verifies error is an object, has a string name === "StorageError" (and the expected "type" property if applicable), then replace the current check (the block that inspects e and uses (e as any).name === "StorageError") to call isStorageError(e) and throw e when it returns true; this removes the any usage and makes the intent explicit.
🤖 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/storage/adapter/common/queryEventsBase.ts`:
- Around line 45-63: collectRawEventTypeValues and getTablesForRequest are
currently only picking the first "eventType" condition per group and ignoring
the condition operator, causing NEQ/NOT IN to be treated as positive filters;
update collectRawEventTypeValues to collect all eventType conditions (not just
the first) including their operators, then change getTablesForRequest to
interpret operators: for EQ/IN aggregate the matching EventTypeLabel values and
return their mapped EVENT_TYPE_TO_TABLE entries; for NEQ/NOT IN remove the
mapped tables from ALL_TABLES (i.e., treat as exclusions) and return the
remainder; use QueryFilterGroup, collectRawEventTypeValues, getTablesForRequest
and EVENT_TYPE_TO_TABLE identifiers to locate and implement this behavior.
In `@src/storage/adapter/postgres/handlers/queryEvents.ts`:
- Around line 300-306: The SUM aggregation uses
PG_FIELDS[t]?.[agg.field]?.whereCol which is null for computed fields like
ai_token_usage_events.debitAmount, causing 0 results; add a new PGFieldDef
property (e.g., aggExpr) for fields that are computed but aggregatable, set
ai_token_usage_events.debitAmount.aggExpr to the correct SQL/JSON expression,
and update the aggregator in queryEvents.ts (the if (isSum && agg.field) block)
to prefer aggExpr if present before falling back to whereCol and the 0::bigint
default so SUM uses the computed expression.
---
Outside diff comments:
In `@src/zod/event.ts`:
- Around line 30-41: Replace the runtime JSON.parse in the transform with
Zod-level parsing/validation: change the metadata field definition (currently
metadata: z.string().optional()) to a preprocessing schema like
z.preprocess((val) => { if (typeof val === 'string') { try { return
JSON.parse(val); } catch (e) { throw new Error('Invalid JSON for metadata'); } }
return val; }, z.record(z.unknown()).optional()), and remove the JSON.parse(...)
call inside the transform so the transform uses the already-validated metadata
as a Record<string, unknown> or undefined; this ensures malformed JSON becomes a
ZodError and non-object JSON is rejected according to the Record<string,
unknown>() contract.
---
Nitpick comments:
In `@src/storage/adapter/clickhouse/handlers/priceRequestSdkCall.ts`:
- Around line 10-87: Rename the exported function handlePriceRequestSdkCall to
handlePriceRequestBasicUsage and update all internal and external references
(imports/exports, tests, call sites) accordingly; keep the same signature and
behavior, update any JSDoc/comments to reference BASIC_USAGE/basic_usage_events
if present, and ensure error construction that mentions
StorageError.priceCalculationFailed and StorageError.invalidData still use the
new function name where applicable so names are consistent across the codebase.
- Around line 74-81: Replace the inline cast to any by introducing a proper type
guard and using it in the existing error-check block: add a function like
isStorageError(error: unknown): error is StorageError that verifies error is an
object, has a string name === "StorageError" (and the expected "type" property
if applicable), then replace the current check (the block that inspects e and
uses (e as any).name === "StorageError") to call isStorageError(e) and throw e
when it returns true; this removes the any usage and makes the intent explicit.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 60a1a039-5da7-425c-9bf8-19de410e90f4
📒 Files selected for processing (19)
protosrc/events/BasicUsage.tssrc/factory/EventStorageAdapterFactory.tssrc/interface/event/Event.tssrc/interface/storage/Storage.tssrc/routes/gRPC/query/queryEvents.tssrc/storage/adapter/clickhouse/ClickHouseAdapter.tssrc/storage/adapter/clickhouse/handlers/addSdkCall.tssrc/storage/adapter/clickhouse/handlers/priceRequestSdkCall.tssrc/storage/adapter/clickhouse/handlers/queryEvents.tssrc/storage/adapter/common/priceRequestPayment.tssrc/storage/adapter/common/queryEventsBase.tssrc/storage/adapter/postgres/handlers/addAiTokenUsage.tssrc/storage/adapter/postgres/handlers/addSdkCall.tssrc/storage/adapter/postgres/handlers/priceRequestSdkCall.tssrc/storage/adapter/postgres/handlers/queryEvents.tssrc/storage/adapter/postgres/postgres.tssrc/utils/eventHelpers.tssrc/zod/event.ts
✅ Files skipped from review due to trivial changes (1)
- proto
🚧 Files skipped from review as they are similar to previous changes (2)
- src/interface/storage/Storage.ts
- src/storage/adapter/postgres/handlers/addAiTokenUsage.ts
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/storage/adapter/clickhouse/handlers/queryEvents.ts`:
- Around line 202-206: The code uses DateTime.fromISO(condition.value) which
parses zone-less ISO strings in local time; change it to parse in UTC by using
DateTime.fromISO(condition.value, { zone: 'utc' }) (or immediately call
.toUTC()/setZone('utc')) before passing dt into toClickHouseDateTime, keeping
the existing dt.isValid check and using the same variables (DateTime.fromISO,
dt, condition.value, toClickHouseDateTime).
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ef2a7cfa-d70c-4e88-ac3e-007238f26839
📒 Files selected for processing (3)
src/storage/adapter/clickhouse/handlers/queryEvents.tssrc/storage/adapter/common/queryEventsBase.tssrc/storage/adapter/postgres/handlers/queryEvents.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/storage/adapter/postgres/handlers/queryEvents.ts
- src/storage/adapter/common/queryEventsBase.ts
Merged
This was referenced May 17, 2026
Merged
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.
Summary by CodeRabbit
New Features
Refactor
Bug Fixes