feat(api-service): refactor usage queries to use materialized views#9824
feat(api-service): refactor usage queries to use materialized views#9824djabarovgeorge merged 30 commits intonextfrom
Conversation
…for event counts - Introduced scripts for seeding ClickHouse with realistic mock data, including organizations, workflows, and events. - Added a new materialized view `trace_event_counts_mv` to optimize query performance for step_completed event counts. - Updated package.json with new seed commands for ClickHouse. - Implemented data generation logic to simulate various organizational profiles and workflows. - Enhanced trace logging and analytics capabilities by integrating new repository methods for fetching event counts. This commit enhances the observability and testing capabilities of the API service by providing a robust data seeding mechanism.
✅ Deploy Preview for dashboard-v2-novu-staging canceled.
|
djabarovgeorge
temporarily deployed
to
Linting
GitHub Actions
Inactive
djabarovgeorge
temporarily deployed
to
Linting
GitHub Actions
Inactive
djabarovgeorge
had a problem deploying
to
Linting
GitHub Actions
Failure
|
Hey there and thank you for opening this pull request! 👋 We require pull request titles to follow specific formatting rules and it looks like your proposed title needs to be adjusted. Your PR title is: Requirements:
Expected format: Details: PR title must end with 'fixes TICKET-ID' (e.g., 'fixes NOV-123') or include ticket ID in branch name |
djabarovgeorge
temporarily deployed
to
Linting
GitHub Actions
Inactive
djabarovgeorge
had a problem deploying
to
Linting
GitHub Actions
Failure
djabarovgeorge
temporarily deployed
to
Linting
GitHub Actions
Inactive
…ountsRepository and remove unused trace-event-counts schema - Updated BuildMessagesDeliveredChart use case to utilize MessageSentCountsRepository for fetching message delivery data. - Removed the trace-event-counts schema and related code from the repository as it is no longer needed. - Cleaned up imports in the analytic-logs module to reflect the changes.
commit: |
…kflowActivityCountsRepository
…er data retrieval
…d total interactions charts - Added feature flag checks to determine the data source for interaction trend and total interactions charts. - Introduced InteractionCountsRepository for fetching interaction data based on feature flags. - Updated shared module to include InteractionCountsRepository and added new feature flag key for interaction counts.
…ed SQL migration - Deleted InteractionCountsRepository and its associated SQL migration for interaction counts. - Updated use cases to utilize WorkflowActivityCountsRepository for interaction trend and total interactions data retrieval. - Adjusted shared module to remove InteractionCountsRepository references.
…onents - Deleted WorkflowVolumeCounts table and materialized view from SQL migrations. - Removed WorkflowVolumeCountsRepository and its schema from the analytic logs service. - Updated build-workflow-by-volume-chart use case to eliminate dependency on WorkflowVolumeCountsRepository. - Adjusted shared module to remove references to WorkflowVolumeCountsRepository. - Removed feature flag key for workflow volume data retrieval.
…w and provider counts
djabarovgeorge
changed the title
There was a problem hiding this comment.
i am still not sure if i will merge this one, its purpose was to seed quickly, click house directly for testing purposes.
There was a problem hiding this comment.
same here, i am still not sure if i will merge this one, its purpose was to seed the application with data through a trigger, this one is much slower as it require to go through all of the trigger flow.
djabarovgeorge
temporarily deployed
to
Linting
GitHub Actions
Inactive
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/shared/src/types/feature-flags.ts (1)
77-85: Minor naming inconsistency between related flags.There's an inconsistency in pluralization:
- Line 79:
IS_ANALYTIC_V2_ACTIVE_SUBSCRIBER_TREND_READ_ENABLED(singular)- Line 84:
IS_ANALYTIC_V2_ACTIVE_SUBSCRIBERS_READ_ENABLED(plural)Consider aligning these for consistency, e.g., both using
SUBSCRIBERS(plural) to match the domain concept of "active subscribers."apps/api/src/app/activity/usecases/build-delivery-trend-chart/build-delivery-trend-chart.usecase.ts (1)
64-107: Consider extracting the feature-flag checking logic into a shared helper.The feature-flag checking pattern (building context, parallel flag fetching, combining with OR) is duplicated across all chart use cases. While this is acceptable and keeps each use case self-contained, you could optionally extract this into a reusable helper for consistency and reduced boilerplate.
Example approach:
// In a shared helper or base class async isAnalyticV2Enabled( organizationId: string, environmentId: string, dedicatedFlagKey: FeatureFlagsKeysEnum ): Promise<boolean> { const [isGlobalEnabled, isDedicatedEnabled] = await Promise.all([ this.featureFlagsService.getFlag({ key: FeatureFlagsKeysEnum.IS_ANALYTIC_V2_LOGS_READ_GLOBAL_ENABLED, defaultValue: false, organization: { _id: organizationId }, environment: { _id: environmentId }, }), this.featureFlagsService.getFlag({ key: dedicatedFlagKey, defaultValue: false, organization: { _id: organizationId }, environment: { _id: environmentId }, }), ]); return isGlobalEnabled || isDedicatedEnabled; }
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
apps/api/src/app/activity/usecases/build-active-subscribers-chart/build-active-subscribers-chart.usecase.tsapps/api/src/app/activity/usecases/build-active-subscribers-trend-chart/build-active-subscribers-trend-chart.usecase.tsapps/api/src/app/activity/usecases/build-avg-messages-per-subscriber-chart/build-avg-messages-per-subscriber-chart.usecase.tsapps/api/src/app/activity/usecases/build-delivery-trend-chart/build-delivery-trend-chart.usecase.tsapps/api/src/app/activity/usecases/build-interaction-trend-chart/build-interaction-trend-chart.usecase.tsapps/api/src/app/activity/usecases/build-messages-delivered-chart/build-messages-delivered-chart.usecase.tsapps/api/src/app/activity/usecases/build-provider-by-volume-chart/build-provider-by-volume-chart.usecase.tsapps/api/src/app/activity/usecases/build-total-interactions-chart/build-total-interactions-chart.usecase.tspackages/shared/src/types/feature-flags.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Write concise, technical TypeScript code with accurate examples
Use descriptive variable names with auxiliary verbs (isLoading, hasError)
Add blank lines before return statements
Import motion components from 'motion/react' instead of 'motion-react'
**/*.{ts,tsx}: Write concise, technical TypeScript code with accurate examples
Use functional and declarative programming patterns; avoid classes
Prefer iteration and modularization over code duplication, minimize code duplication as possible
Use descriptive variable names with auxiliary verbs (e.g., isLoading, hasError)
Structure files: exported component, subcomponents, helpers, static content, types
Don't leave comments in code, unless they explain something complex and not trivial
Don't use nested ternaries
Favor named exports for components
Use TypeScript for all code; prefer interfaces over types
In front end code, use types over interfaces
Use functional components with TypeScript types
Use the "function" keyword for pure functions
Avoid unnecessary curly braces in conditionals; use concise syntax for simple statements
Add blank lines before return statements
When importing "motion-react" package, import it from "motion/react"
Files:
apps/api/src/app/activity/usecases/build-provider-by-volume-chart/build-provider-by-volume-chart.usecase.tsapps/api/src/app/activity/usecases/build-messages-delivered-chart/build-messages-delivered-chart.usecase.tsapps/api/src/app/activity/usecases/build-active-subscribers-trend-chart/build-active-subscribers-trend-chart.usecase.tsapps/api/src/app/activity/usecases/build-interaction-trend-chart/build-interaction-trend-chart.usecase.tsapps/api/src/app/activity/usecases/build-avg-messages-per-subscriber-chart/build-avg-messages-per-subscriber-chart.usecase.tsapps/api/src/app/activity/usecases/build-delivery-trend-chart/build-delivery-trend-chart.usecase.tspackages/shared/src/types/feature-flags.tsapps/api/src/app/activity/usecases/build-total-interactions-chart/build-total-interactions-chart.usecase.tsapps/api/src/app/activity/usecases/build-active-subscribers-chart/build-active-subscribers-chart.usecase.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use functional and declarative programming patterns; avoid classes
Files:
apps/api/src/app/activity/usecases/build-provider-by-volume-chart/build-provider-by-volume-chart.usecase.tsapps/api/src/app/activity/usecases/build-messages-delivered-chart/build-messages-delivered-chart.usecase.tsapps/api/src/app/activity/usecases/build-active-subscribers-trend-chart/build-active-subscribers-trend-chart.usecase.tsapps/api/src/app/activity/usecases/build-interaction-trend-chart/build-interaction-trend-chart.usecase.tsapps/api/src/app/activity/usecases/build-avg-messages-per-subscriber-chart/build-avg-messages-per-subscriber-chart.usecase.tsapps/api/src/app/activity/usecases/build-delivery-trend-chart/build-delivery-trend-chart.usecase.tspackages/shared/src/types/feature-flags.tsapps/api/src/app/activity/usecases/build-total-interactions-chart/build-total-interactions-chart.usecase.tsapps/api/src/app/activity/usecases/build-active-subscribers-chart/build-active-subscribers-chart.usecase.ts
**/*.{tsx,ts}
📄 CodeRabbit inference engine (CLAUDE.md)
Favor named exports for components
Files:
apps/api/src/app/activity/usecases/build-provider-by-volume-chart/build-provider-by-volume-chart.usecase.tsapps/api/src/app/activity/usecases/build-messages-delivered-chart/build-messages-delivered-chart.usecase.tsapps/api/src/app/activity/usecases/build-active-subscribers-trend-chart/build-active-subscribers-trend-chart.usecase.tsapps/api/src/app/activity/usecases/build-interaction-trend-chart/build-interaction-trend-chart.usecase.tsapps/api/src/app/activity/usecases/build-avg-messages-per-subscriber-chart/build-avg-messages-per-subscriber-chart.usecase.tsapps/api/src/app/activity/usecases/build-delivery-trend-chart/build-delivery-trend-chart.usecase.tspackages/shared/src/types/feature-flags.tsapps/api/src/app/activity/usecases/build-total-interactions-chart/build-total-interactions-chart.usecase.tsapps/api/src/app/activity/usecases/build-active-subscribers-chart/build-active-subscribers-chart.usecase.ts
apps/api/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Prefer interfaces over types in backend code
Files:
apps/api/src/app/activity/usecases/build-provider-by-volume-chart/build-provider-by-volume-chart.usecase.tsapps/api/src/app/activity/usecases/build-messages-delivered-chart/build-messages-delivered-chart.usecase.tsapps/api/src/app/activity/usecases/build-active-subscribers-trend-chart/build-active-subscribers-trend-chart.usecase.tsapps/api/src/app/activity/usecases/build-interaction-trend-chart/build-interaction-trend-chart.usecase.tsapps/api/src/app/activity/usecases/build-avg-messages-per-subscriber-chart/build-avg-messages-per-subscriber-chart.usecase.tsapps/api/src/app/activity/usecases/build-delivery-trend-chart/build-delivery-trend-chart.usecase.tsapps/api/src/app/activity/usecases/build-total-interactions-chart/build-total-interactions-chart.usecase.tsapps/api/src/app/activity/usecases/build-active-subscribers-chart/build-active-subscribers-chart.usecase.ts
**
📄 CodeRabbit inference engine (.cursor/rules/novu.mdc)
Use lowercase with dashes for directories and files (e.g., components/auth-wizard)
Files:
apps/api/src/app/activity/usecases/build-provider-by-volume-chart/build-provider-by-volume-chart.usecase.tsapps/api/src/app/activity/usecases/build-messages-delivered-chart/build-messages-delivered-chart.usecase.tsapps/api/src/app/activity/usecases/build-active-subscribers-trend-chart/build-active-subscribers-trend-chart.usecase.tsapps/api/src/app/activity/usecases/build-interaction-trend-chart/build-interaction-trend-chart.usecase.tsapps/api/src/app/activity/usecases/build-avg-messages-per-subscriber-chart/build-avg-messages-per-subscriber-chart.usecase.tsapps/api/src/app/activity/usecases/build-delivery-trend-chart/build-delivery-trend-chart.usecase.tspackages/shared/src/types/feature-flags.tsapps/api/src/app/activity/usecases/build-total-interactions-chart/build-total-interactions-chart.usecase.tsapps/api/src/app/activity/usecases/build-active-subscribers-chart/build-active-subscribers-chart.usecase.ts
🧬 Code graph analysis (3)
apps/api/src/app/activity/usecases/build-interaction-trend-chart/build-interaction-trend-chart.usecase.ts (6)
apps/api/src/app/activity/usecases/build-active-subscribers-trend-chart/build-active-subscribers-trend-chart.usecase.ts (1)
Injectable(13-89)apps/api/src/app/activity/usecases/build-delivery-trend-chart/build-delivery-trend-chart.usecase.ts (1)
Injectable(13-108)apps/api/src/app/activity/usecases/build-messages-delivered-chart/build-messages-delivered-chart.usecase.ts (1)
Injectable(13-77)apps/api/src/app/activity/usecases/build-provider-by-volume-chart/build-provider-by-volume-chart.usecase.ts (1)
Injectable(13-69)apps/api/src/app/activity/usecases/build-total-interactions-chart/build-total-interactions-chart.usecase.ts (1)
Injectable(13-77)libs/application-generic/src/services/analytic-logs/trace-log/trace-log.repository.ts (1)
Injectable(15-228)
apps/api/src/app/activity/usecases/build-total-interactions-chart/build-total-interactions-chart.usecase.ts (3)
apps/api/src/app/activity/usecases/build-active-subscribers-chart/build-active-subscribers-chart.usecase.ts (1)
Injectable(13-77)apps/api/src/app/activity/usecases/build-messages-delivered-chart/build-messages-delivered-chart.usecase.ts (1)
Injectable(13-77)apps/api/src/app/activity/usecases/build-provider-by-volume-chart/build-provider-by-volume-chart.usecase.ts (1)
Injectable(13-69)
apps/api/src/app/activity/usecases/build-active-subscribers-chart/build-active-subscribers-chart.usecase.ts (1)
libs/application-generic/src/services/analytic-logs/index.ts (1)
WorkflowRunRepository(10-10)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: Unit test backend services (@novu/worker)
- GitHub Check: E2E test API / Test E2E
- GitHub Check: Unit test backend services (@novu/api-service)
- GitHub Check: Unit test backend services (@novu/webhook)
- GitHub Check: Unit test @novu public NPM packages (except providers)
- GitHub Check: Unit test @novu internal packages
- GitHub Check: Unit test backend services (@novu/inbound-mail)
- GitHub Check: Unit test backend services (@novu/ws)
- GitHub Check: Validate OpenAPI
- GitHub Check: Analyze (javascript)
- GitHub Check: Analyze (typescript)
- GitHub Check: Redirect rules - dashboard-v2-novu-staging
- GitHub Check: Header rules - dashboard-v2-novu-staging
- GitHub Check: Pages changed - dashboard-v2-novu-staging
- GitHub Check: publish_preview_packages
🔇 Additional comments (11)
apps/api/src/app/activity/usecases/build-active-subscribers-trend-chart/build-active-subscribers-trend-chart.usecase.ts (1)
1-62: LGTM! Clean implementation of feature-flag driven data source selection.The pattern follows the established convention across other chart use cases: parallel flag fetching, combined boolean check, and conditional repository call. The existing chart data transformation logic is preserved.
apps/api/src/app/activity/usecases/build-active-subscribers-chart/build-active-subscribers-chart.usecase.ts (1)
1-76: LGTM! Implementation follows the established feature-flag pattern.The conditional data source selection between
TraceRollupRepositoryandWorkflowRunRepositoryis correctly implemented with parallel flag fetching.apps/api/src/app/activity/usecases/build-messages-delivered-chart/build-messages-delivered-chart.usecase.ts (1)
1-76: LGTM! Consistent implementation of feature-flag driven data source selection.The pattern aligns with other chart use cases, correctly using
TraceRollupRepository.getMessageSendCountwhen enabled and falling back toStepRunRepository.getMessagesDeliveredData.apps/api/src/app/activity/usecases/build-provider-by-volume-chart/build-provider-by-volume-chart.usecase.ts (1)
1-68: LGTM! Clean feature-flag driven data source selection.The implementation correctly follows the established pattern. The data mapping from
provider_idtoproviderIdis preserved regardless of which repository is used.apps/api/src/app/activity/usecases/build-avg-messages-per-subscriber-chart/build-avg-messages-per-subscriber-chart.usecase.ts (1)
1-76: LGTM! Follows the consistent feature-flag pattern.The implementation correctly selects between
TraceRollupRepositoryandStepRunRepositorybased on feature flags, maintaining the same return structure.apps/api/src/app/activity/usecases/build-delivery-trend-chart/build-delivery-trend-chart.usecase.ts (1)
1-62: LGTM! Consistent feature-flag driven data source selection.The implementation correctly uses
DeliveryTrendCountsRepository(appropriate for this specific use case) instead ofTraceRollupRepository, following the same flag-checking pattern. The existing chart data transformation logic is preserved.apps/api/src/app/activity/usecases/build-total-interactions-chart/build-total-interactions-chart.usecase.ts (2)
2-9: LGTM!The imports and constructor injections correctly add
TraceRollupRepositoryandFeatureFlagsServicedependencies, following the established pattern used across other analytics usecases in this PR.Also applies to: 15-22
32-70: Feature-flag routing is consistent with other usecases.The implementation correctly:
- Constructs the feature flag context with org/env IDs
- Fetches both global and dedicated flags in parallel
- Uses OR logic to enable the new query path
- Passes identical parameters to both the new and legacy repository methods
This matches the established pattern in
BuildProviderByVolumeChart,BuildMessagesDeliveredChart, and other analytics usecases.apps/api/src/app/activity/usecases/build-interaction-trend-chart/build-interaction-trend-chart.usecase.ts (3)
2-9: LGTM!The imports and dependency injection follow the same pattern as
BuildTotalInteractionsChartand other usecases, correctly addingTraceRollupRepositoryandFeatureFlagsService.Also applies to: 15-22
28-62: Feature-flag based routing implemented correctly.The feature flag logic follows the established pattern, using
IS_ANALYTIC_V2_INTERACTION_TREND_READ_ENABLEDas the dedicated flag for this usecase. Both repository paths receive identical parameters.
64-102: Data transformation logic is sound.The chart data aggregation correctly:
- Pre-populates all dates in the range to ensure continuous time series
- Safely handles the
event_typelookup with optional chaining- Parses string counts from the repository response
No changes needed here.
…lated scripts - Introduced provider_id column in the traces table to store the provider ID for messages. - Updated SQL migration to ensure provider_id is added before creating dependent materialized views. - Modified seed script to reflect changes in trace rollup and delivery trend counts. - Adjusted generator interface and trace record creation to include provider_id.
…djust previous end date calculation
djabarovgeorge
temporarily deployed
to
Linting
GitHub Actions
Inactive
djabarovgeorge
temporarily deployed
to
Linting
GitHub Actions
Inactive
djabarovgeorge
temporarily deployed
to
Linting
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Fix all issues with AI agents
In @apps/api/migrations/clickhouse-migrations/3_analytics_tables.sql:
- Around line 1-27: The initial CREATE TABLE/CREATE MATERIALIZED VIEW for
trace_rollup and trace_rollup_mv are redundant because later statements drop and
recreate them with extended schema; remove lines creating trace_rollup and
trace_rollup_mv (the CREATE TABLE IF NOT EXISTS trace_rollup and CREATE
MATERIALIZED VIEW IF NOT EXISTS trace_rollup_mv blocks) from this migration, or
instead split into two migrations: keep an initial migration that creates
trace_rollup and delivery_trend_counts, and move the schema-evolution changes
that add event_type and provider_id into a subsequent migration that
alters/recreates trace_rollup and trace_rollup_mv.
- Around line 1-2: The top-of-file comment above the trace rollup materialized
view is stale: it claims the table "Handles both message counts and subscriber
activity from traces table (message_sent events)" but the final materialized
view aggregates multiple event types (message_sent, message_seen, message_read,
message_snoozed, message_archived). Update the comment to accurately describe
the materialized view's purpose and the full set of event types it captures (or
remove the misleading parenthetical), referencing the trace rollup materialized
view and the listed event types so readers know what is being aggregated.
- Around line 64-70: The DROP TABLEs for trace_rollup and interaction_counts
will permanently delete historical aggregates; confirm with the team whether
data loss is acceptable, and if not implement a backfill before dropping:
create/ensure the new schema (trace_rollup with event_type and merged
structure), run an INSERT ... SELECT from the source traces (or from
interaction_counts if it contains unique rows) to populate the new trace_rollup
(and/or merged interaction_counts data) preserving timestamps and deriving
event_type as needed, verify counts match pre-drop totals, then drop the old
tables and create the new materialized views (trace_rollup_mv,
interaction_counts_mv) only after successful verification.
In @apps/api/scripts/clickhouse-seeder/generators.ts:
- Around line 268-276: The loop creating subscribers uses a single subscriberId
variable initialized from singleEnvConfig.subscriberId so every subscriber gets
the same ID when a custom id is provided; change the logic in the for loop
(where subscriberId is set and env.subscribers.push is called) to use the
provided singleEnvConfig.subscriberId only for the first subscriber (s === 0)
and call generateId() for all other iterations so each subscriber gets a unique
id.
- Around line 253-266: The loop assigns the same provided
singleEnvConfig.workflowId to every workflow, causing duplicate IDs; change the
workflowId assignment inside the for-loop so it uses singleEnvConfig.workflowId
only for the first iteration (w === 0) and calls generateId() for all subsequent
workflows (use a conditional when setting workflowId), keeping the rest of the
push logic (selectWorkflowTemplate(), env.workflows.push(...), template,
channels) unchanged.
In
@apps/worker/src/app/workflow/usecases/subscriber-job-bound/subscriber-job-bound.usecase.ts:
- Line 414: The traceData object sets provider_id to an empty string which
contradicts the schema (CHNullable(CHString())) and other fields that use null;
change the assignment to use null instead of '' (update provider_id to null),
following the existing pattern used elsewhere (e.g.,
create-execution-details.usecase.ts uses command.providerId || null) so
traceData.provider_id is consistently null when absent.
In
@libs/application-generic/src/services/analytic-logs/trace-rollup/trace-rollup.repository.ts:
- Around line 106-183: getActiveSubscribersCount is ignoring the previousEndDate
parameter and instead uses adjustedPreviousEndDate computed from startDate;
update the method to use the provided previousEndDate (formatted to date-only)
for previousParams (replace the getDateOnlyPreviousEndDate(startDate) usage),
ensure adjustedPreviousEndDate (or directly previousParams.previousEndDate) is
set from previousEndDate.toISOString().split('T')[0], and keep workflowIds
injection and query params logic unchanged so the previous period query uses the
passed previousEndDate.
- Around line 234-320: The method getAvgMessagesPerSubscriberData defines a
previousEndDate parameter but then ignores it by using adjustedPreviousEndDate;
update previousParams to use previousEndDate (converted to date-only string)
instead of adjustedPreviousEndDate, or if adjustedPreviousEndDate is required,
remove the unused previousEndDate parameter and its references; ensure
previousParams uses the matching key previousEndDate and workflowIds logic
remains the same so the ClickHouse query receives the correct date value.
- Around line 322-397: The previousEndDate parameter passed into
getTotalInteractionsCount is never used; replace the computed
adjustedPreviousEndDate (currently from getDateOnlyPreviousEndDate(startDate))
with a string derived from the previousEndDate parameter and assign that to
previousParams.previousEndDate (e.g.,
previousEndDate.toISOString().split('T')[0]), removing the unused
adjustedPreviousEndDate computation; update references in
getTotalInteractionsCount so previousParams uses the provided previousEndDate
value.
🧹 Nitpick comments (6)
apps/api/scripts/clickhouse-seeder/generators.ts (4)
137-139: Consider adding empty array guard.If
itemsis empty, this returnsundefined(typed asT), which could cause subtle issues. Since this is a seeding script, empty arrays are unlikely, but a defensive check would be safer.♻️ Optional defensive fix
function randomChoice<T>(items: T[]): T { + if (items.length === 0) { + throw new Error('Cannot select from empty array'); + } return items[Math.floor(Math.random() * items.length)]; }
161-166: Hardcoded divisor couples to distribution sum.Line 166 divides by 10, which assumes
ENTERPRISE_HEAVY_DISTRIBUTIONvalues sum to 10. If the distribution changes, this calculation breaks silently.♻️ Derive divisor from distribution
const distribution = ENTERPRISE_HEAVY_DISTRIBUTION; + const distributionTotal = Object.values(distribution).reduce((sum, count) => sum + count, 0); let orgCount = 0; for (const [profileType, count] of Object.entries(distribution)) { - const scaledCount = Math.ceil(count * (config.organizations / 10)); + const scaledCount = Math.ceil(count * (config.organizations / distributionTotal));
520-523: Unused variable:orgMapis created but never used.The
orgMapis populated but never referenced in this function.♻️ Remove unused code
export function generateStepRuns(workflowRuns: WorkflowRunRecord[], organizations: Organization[]): StepRunRecord[] { const allStepRuns: StepRunRecord[] = []; - const orgMap = new Map<string, Organization>(); - for (const org of organizations) { - orgMap.set(org.id, org); - } - const workflowMap = new Map<string, Workflow>();
639-649: Dead code: 'canceled' status case is unreachable.
STEP_RUN_STATUS_DISTRIBUTIONdefines statuses ascompleted,failed,skipped, anddelayed. ThestepStatus === 'canceled'branch (Line 644) will never execute.♻️ Remove unreachable case or add 'canceled' to distribution
if (index === total - 1) { if (stepStatus === 'completed') { return 'step_completed'; } else if (stepStatus === 'failed') { return 'step_canceled'; - } else if (stepStatus === 'canceled') { - return 'step_canceled'; } return 'step_completed'; }libs/application-generic/src/services/analytic-logs/trace-rollup/trace-rollup.repository.ts (2)
8-13: Consider renaming for clarity.The function name
getDateOnlyPreviousEndDateand parameter namestartDatecreate confusion. It calculates the day beforestartDate, which is conceptually the end of the previous period. Consider renaming togetDayBeforeAsDateString(date: Date)or similar to better express intent.
29-397: Consider extracting shared patterns to reduce duplication.The methods for current/previous period queries (lines 29-397) share significant boilerplate:
- Workflow filter construction
- Date formatting (
toISOString().split('T')[0])- Params building for current and previous periods
Promise.allexecution pattern- Result parsing
Extracting helpers (e.g.,
formatDateParam,buildWorkflowFilter,executePeriodComparison) would improve maintainability and reduce repetition.♻️ Example helper extractions
private formatDateParam(date: Date): string { return date.toISOString().split('T')[0]; } private buildWorkflowFilter(workflowIds?: string[]): string { return workflowIds?.length ? 'AND workflow_id IN {workflowIds:Array(String)}' : ''; } private buildDateParams( environmentId: string, organizationId: string, startDate: Date, endDate: Date, workflowIds?: string[] ): Record<string, unknown> { const params: Record<string, unknown> = { environmentId, organizationId, startDate: this.formatDateParam(startDate), endDate: this.formatDateParam(endDate), }; if (workflowIds?.length) { params.workflowIds = workflowIds; } return params; }
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/api/migrations/clickhouse-migrations/3_analytics_tables.sqlapps/api/scripts/clickhouse-seeder/generators.tsapps/api/scripts/seed-clickhouse.tsapps/worker/src/app/workflow/usecases/subscriber-job-bound/subscriber-job-bound.usecase.tslibs/application-generic/src/services/analytic-logs/trace-rollup/trace-rollup.repository.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Write concise, technical TypeScript code with accurate examples
Use descriptive variable names with auxiliary verbs (isLoading, hasError)
Add blank lines before return statements
Import motion components from 'motion/react' instead of 'motion-react'
**/*.{ts,tsx}: Write concise, technical TypeScript code with accurate examples
Use functional and declarative programming patterns; avoid classes
Prefer iteration and modularization over code duplication, minimize code duplication as possible
Use descriptive variable names with auxiliary verbs (e.g., isLoading, hasError)
Structure files: exported component, subcomponents, helpers, static content, types
Don't leave comments in code, unless they explain something complex and not trivial
Don't use nested ternaries
Favor named exports for components
Use TypeScript for all code; prefer interfaces over types
In front end code, use types over interfaces
Use functional components with TypeScript types
Use the "function" keyword for pure functions
Avoid unnecessary curly braces in conditionals; use concise syntax for simple statements
Add blank lines before return statements
When importing "motion-react" package, import it from "motion/react"
Files:
apps/worker/src/app/workflow/usecases/subscriber-job-bound/subscriber-job-bound.usecase.tsapps/api/scripts/seed-clickhouse.tslibs/application-generic/src/services/analytic-logs/trace-rollup/trace-rollup.repository.tsapps/api/scripts/clickhouse-seeder/generators.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use functional and declarative programming patterns; avoid classes
Files:
apps/worker/src/app/workflow/usecases/subscriber-job-bound/subscriber-job-bound.usecase.tsapps/api/scripts/seed-clickhouse.tslibs/application-generic/src/services/analytic-logs/trace-rollup/trace-rollup.repository.tsapps/api/scripts/clickhouse-seeder/generators.ts
**/*.{tsx,ts}
📄 CodeRabbit inference engine (CLAUDE.md)
Favor named exports for components
Files:
apps/worker/src/app/workflow/usecases/subscriber-job-bound/subscriber-job-bound.usecase.tsapps/api/scripts/seed-clickhouse.tslibs/application-generic/src/services/analytic-logs/trace-rollup/trace-rollup.repository.tsapps/api/scripts/clickhouse-seeder/generators.ts
**
📄 CodeRabbit inference engine (.cursor/rules/novu.mdc)
Use lowercase with dashes for directories and files (e.g., components/auth-wizard)
Files:
apps/worker/src/app/workflow/usecases/subscriber-job-bound/subscriber-job-bound.usecase.tsapps/api/scripts/seed-clickhouse.tsapps/api/migrations/clickhouse-migrations/3_analytics_tables.sqllibs/application-generic/src/services/analytic-logs/trace-rollup/trace-rollup.repository.tsapps/api/scripts/clickhouse-seeder/generators.ts
apps/api/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Prefer interfaces over types in backend code
Files:
apps/api/scripts/seed-clickhouse.tsapps/api/scripts/clickhouse-seeder/generators.ts
🧬 Code graph analysis (2)
apps/api/scripts/seed-clickhouse.ts (3)
apps/api/scripts/clickhouse-seeder/generators.ts (5)
GenerationProgress(330-335)generateOrganizations(155-227)estimateTotalWorkflowRuns(345-355)generateDataInBatches(357-439)Organization(17-22)apps/api/scripts/clickhouse-seeder/config.ts (1)
parseCliArgs(131-224)apps/api/scripts/clickhouse-seeder/inserter.ts (2)
ClickHouseInserter(33-152)estimateDataSize(164-173)
apps/api/scripts/clickhouse-seeder/generators.ts (2)
apps/api/scripts/clickhouse-seeder/config.ts (11)
OrganizationProfile(21-31)WorkflowTemplate(75-80)SeederConfig(12-19)ENTERPRISE_HEAVY_DISTRIBUTION(69-73)ORGANIZATION_PROFILES(33-67)SingleEnvironmentConfig(1-10)WORKFLOW_TEMPLATES(82-87)WORKFLOW_RUN_STATUS_DISTRIBUTION(89-93)DELIVERY_LIFECYCLE_STATUS_DISTRIBUTION(102-109)STEP_RUN_STATUS_DISTRIBUTION(95-100)TRACE_EVENT_TYPES(111-115)apps/api/scripts/clickhouse-seeder/time-distribution.ts (2)
generateRandomTimestampsForDay(89-125)addRandomJitter(155-158)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: Redirect rules - dashboard-v2-novu-staging
- GitHub Check: Header rules - dashboard-v2-novu-staging
- GitHub Check: Pages changed - dashboard-v2-novu-staging
- GitHub Check: E2E test API / Test E2E
- GitHub Check: Unit test backend services (@novu/ws)
- GitHub Check: Unit test backend services (@novu/worker)
- GitHub Check: Unit test backend services (@novu/inbound-mail)
- GitHub Check: Unit test backend services (@novu/api-service)
- GitHub Check: Unit test @novu internal packages
- GitHub Check: Unit test backend services (@novu/webhook)
- GitHub Check: Unit test @novu public NPM packages (except providers)
- GitHub Check: Validate OpenAPI
- GitHub Check: Analyze (javascript)
- GitHub Check: Analyze (typescript)
- GitHub Check: publish_preview_packages
🔇 Additional comments (18)
apps/api/scripts/clickhouse-seeder/generators.ts (7)
1-15: LGTM!Imports are well-organized, using named imports from local modules. The use of
randomBytesfrom Node'scryptomodule for ID generation is appropriate.
17-127: LGTM!The interfaces are well-structured and follow the coding guideline to prefer interfaces over types in backend code. The record interfaces properly model the ClickHouse table schemas with appropriate nullable fields.
302-328: LGTM!The function correctly generates workflow runs across organizations, environments, and days. The date handling properly creates new Date instances per iteration to avoid mutation issues.
357-438: LGTM!Excellent use of a generator function for memory-efficient batch streaming. The progress tracking and final batch handling are correctly implemented.
471-515: LGTM!Record creation is thorough with appropriate use of weighted random distributions for status fields and proper date handling.
552-602: LGTM!The step run record creation correctly maps channels to providers and populates the
provider_idfield, which aligns with the PR objective of adding provider tracking.
656-703: LGTM!The trace record creation correctly propagates
provider_idfrom step runs to traces, supporting the PR's goal of adding provider tracking. TheformatEventTitlehelper is a clean utility.apps/api/scripts/seed-clickhouse.ts (5)
1-6: LGTM!Environment configuration is loaded early before other imports that might depend on it. Good practice using
path.joinfor cross-platform compatibility.
62-73: Consider validating optional credentials consistently.
CLICK_HOUSE_USERandCLICK_HOUSE_PASSWORDare used (Lines 70-71) but not validated. If they're required for your ClickHouse setup, add them to the validation. If optional, this is fine as-is.
75-176: LGTM!The main execution flow is well-structured with proper error handling, resource cleanup in the
finallyblock, and efficient parallel batch inserts. Progress reporting provides good visibility during long-running seeds.
178-193: LGTM!The breakdown helper correctly aggregates organization counts by profile type. TypeScript's type system ensures only valid keys are accessed.
195-206: LGTM!Standard Node.js script pattern with proper module detection and clean process exit codes. The named export follows coding guidelines.
libs/application-generic/src/services/analytic-logs/trace-rollup/trace-rollup.repository.ts (4)
15-27: LGTM!The class setup follows NestJS conventions. The constructor correctly initializes the base
LogRepositoryand sets the logger context.
185-232: LGTM!The trend data method is correctly implemented. It queries a single period and returns the daily breakdown as expected.
399-447: LGTM!The interaction trend data method is correctly implemented with proper grouping by date and event type.
449-496: LGTM!The provider volume query is correctly implemented. The
LIMIT 5is hardcoded—consider making it configurable if future requirements need flexibility, but it's acceptable as-is.apps/api/migrations/clickhouse-migrations/3_analytics_tables.sql (2)
29-58: LGTM!The
delivery_trend_countstable and materialized view are well-structured. Good use ofLowCardinality(String)forstep_typeandSummingMergeTreefor efficient count aggregation. The filter onstatus = 'completed'and specific messaging step types is appropriate.
78-107: LGTM on the extended schema design.The final
trace_rolluptable and materialized view are well-designed:
LowCardinality(String)forevent_typeis optimal for the limited set of event typesSummingMergeTreewith comprehensive ORDER BY ensures proper aggregationifNull()handling for nullable source columns prevents NULL propagation- The event type filter captures the full range of message lifecycle events
apps/api/migrations/clickhouse-migrations/3_analytics_tables.sql
Outdated
Show resolved
Hide resolved
apps/api/migrations/clickhouse-migrations/3_analytics_tables.sql
Outdated
Show resolved
Hide resolved
apps/api/migrations/clickhouse-migrations/3_analytics_tables.sql
Outdated
Show resolved
Hide resolved
| for (let w = 0; w < singleEnvConfig.workflows; w++) { | ||
| const template = selectWorkflowTemplate(); | ||
| const workflowId = singleEnvConfig.workflowId || generateId(); | ||
|
|
||
| env.workflows.push({ | ||
| id: workflowId, | ||
| name: `${template.name} ${w + 1}`, | ||
| triggerIdentifier: `${template.type}_${w + 1}`.toLowerCase().replace(/\s+/g, '_'), | ||
| environmentId: envId, | ||
| organizationId: orgId, | ||
| channels: template.channels, | ||
| template, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Bug: Same workflowId assigned to all workflows when provided.
When singleEnvConfig.workflowId is provided, every workflow in the loop receives the same ID. This creates duplicate workflow IDs which would cause data integrity issues.
🐛 Proposed fix: Only use provided ID for first workflow
for (let w = 0; w < singleEnvConfig.workflows; w++) {
const template = selectWorkflowTemplate();
- const workflowId = singleEnvConfig.workflowId || generateId();
+ const workflowId = (w === 0 && singleEnvConfig.workflowId) ? singleEnvConfig.workflowId : generateId();
env.workflows.push({
id: workflowId,📝 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.
| for (let w = 0; w < singleEnvConfig.workflows; w++) { | |
| const template = selectWorkflowTemplate(); | |
| const workflowId = singleEnvConfig.workflowId || generateId(); | |
| env.workflows.push({ | |
| id: workflowId, | |
| name: `${template.name} ${w + 1}`, | |
| triggerIdentifier: `${template.type}_${w + 1}`.toLowerCase().replace(/\s+/g, '_'), | |
| environmentId: envId, | |
| organizationId: orgId, | |
| channels: template.channels, | |
| template, | |
| }); | |
| } | |
| for (let w = 0; w < singleEnvConfig.workflows; w++) { | |
| const template = selectWorkflowTemplate(); | |
| const workflowId = (w === 0 && singleEnvConfig.workflowId) ? singleEnvConfig.workflowId : generateId(); | |
| env.workflows.push({ | |
| id: workflowId, | |
| name: `${template.name} ${w + 1}`, | |
| triggerIdentifier: `${template.type}_${w + 1}`.toLowerCase().replace(/\s+/g, '_'), | |
| environmentId: envId, | |
| organizationId: orgId, | |
| channels: template.channels, | |
| template, | |
| }); | |
| } |
🤖 Prompt for AI Agents
In @apps/api/scripts/clickhouse-seeder/generators.ts around lines 253 - 266, The
loop assigns the same provided singleEnvConfig.workflowId to every workflow,
causing duplicate IDs; change the workflowId assignment inside the for-loop so
it uses singleEnvConfig.workflowId only for the first iteration (w === 0) and
calls generateId() for all subsequent workflows (use a conditional when setting
workflowId), keeping the rest of the push logic (selectWorkflowTemplate(),
env.workflows.push(...), template, channels) unchanged.
| for (let s = 0; s < singleEnvConfig.subscribers; s++) { | ||
| const subscriberId = singleEnvConfig.subscriberId || generateId(); | ||
| env.subscribers.push({ | ||
| id: subscriberId, | ||
| externalId: `user_${s + 1}`, | ||
| environmentId: envId, | ||
| organizationId: orgId, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Bug: Same subscriberId assigned to all subscribers when provided.
Similar to the workflow issue, when singleEnvConfig.subscriberId is provided, every subscriber gets the same ID.
🐛 Proposed fix: Only use provided ID for first subscriber
for (let s = 0; s < singleEnvConfig.subscribers; s++) {
- const subscriberId = singleEnvConfig.subscriberId || generateId();
+ const subscriberId = (s === 0 && singleEnvConfig.subscriberId) ? singleEnvConfig.subscriberId : generateId();
env.subscribers.push({
id: subscriberId,🤖 Prompt for AI Agents
In @apps/api/scripts/clickhouse-seeder/generators.ts around lines 268 - 276, The
loop creating subscribers uses a single subscriberId variable initialized from
singleEnvConfig.subscriberId so every subscriber gets the same ID when a custom
id is provided; change the logic in the for loop (where subscriberId is set and
env.subscribers.push is called) to use the provided singleEnvConfig.subscriberId
only for the first subscriber (s === 0) and call generateId() for all other
iterations so each subscriber gets a unique id.
| entity_id: command.requestId, | ||
| workflow_run_identifier: command.identifier, | ||
| workflow_id: command.templateId, | ||
| provider_id: '', |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, read the file around line 414 to see the full context
sed -n '400,420p' apps/worker/src/app/workflow/usecases/subscriber-job-bound/subscriber-job-bound.usecase.ts | cat -nRepository: novuhq/novu
Length of output: 941
🏁 Script executed:
# Search for other trace creation sites and provider_id patterns
rg -n "provider_id:" --type=ts -C 3 apps/worker/src/app/workflow/usecases/subscriber-job-bound/Repository: novuhq/novu
Length of output: 950
🏁 Script executed:
# Search for traceData definitions and trace creation patterns more broadly
rg -n "traceData\s*=" --type=ts -C 2 | head -100Repository: novuhq/novu
Length of output: 769
🏁 Script executed:
# Read the create-execution-details usecase to see how it structures traceData
sed -n '175,210p' libs/application-generic/src/usecases/create-execution-details/create-execution-details.usecase.ts | cat -nRepository: novuhq/novu
Length of output: 1643
🏁 Script executed:
# Search for all provider_id assignments to see the pattern
rg -n "provider_id\s*:" --type=ts -A 1 -B 1 | head -120Repository: novuhq/novu
Length of output: 8721
🏁 Script executed:
# Search for TraceData type definition or interface
rg -n "interface TraceData|type TraceData|createRequest" --type=ts | head -30Repository: novuhq/novu
Length of output: 3342
Use null instead of empty string for consistency.
The schema explicitly defines provider_id as CHNullable(CHString()), which expects null for missing values. Other optional fields in this same traceData object use null (subscriber_id, external_subscriber_id, message, raw_data), and other trace creation sites (e.g., create-execution-details.usecase.ts) use command.providerId || null. Using '' here creates data inconsistency that complicates queries and violates the schema design.
🤖 Prompt for AI Agents
In
@apps/worker/src/app/workflow/usecases/subscriber-job-bound/subscriber-job-bound.usecase.ts
at line 414, The traceData object sets provider_id to an empty string which
contradicts the schema (CHNullable(CHString())) and other fields that use null;
change the assignment to use null instead of '' (update provider_id to null),
following the existing pattern used elsewhere (e.g.,
create-execution-details.usecase.ts uses command.providerId || null) so
traceData.provider_id is consistently null when absent.
| async getMessageSendCount( | ||
| environmentId: string, | ||
| organizationId: string, | ||
| startDate: Date, | ||
| endDate: Date, | ||
| previousStartDate: Date, | ||
| previousEndDate: Date, | ||
| workflowIds?: string[] | ||
| ): Promise<{ currentPeriod: number; previousPeriod: number }> { | ||
| const workflowFilter = | ||
| workflowIds && workflowIds.length > 0 ? `AND workflow_id IN {workflowIds:Array(String)}` : ''; | ||
|
|
||
| const currentQuery = ` | ||
| SELECT sum(count) as count | ||
| FROM ${TRACE_ROLLUP_TABLE_NAME} | ||
| WHERE | ||
| organization_id = {organizationId:String} | ||
| AND environment_id = {environmentId:String} | ||
| AND event_type = 'message_sent' | ||
| AND date >= {startDate:Date} | ||
| AND date <= {endDate:Date} | ||
| ${workflowFilter} | ||
| `; | ||
|
|
||
| const previousQuery = ` | ||
| SELECT sum(count) as count | ||
| FROM ${TRACE_ROLLUP_TABLE_NAME} | ||
| WHERE | ||
| organization_id = {organizationId:String} | ||
| AND environment_id = {environmentId:String} | ||
| AND event_type = 'message_sent' | ||
| AND date >= {previousStartDate:Date} | ||
| AND date <= {previousEndDate:Date} | ||
| ${workflowFilter} | ||
| `; | ||
|
|
||
| const adjustedPreviousEndDate = getDateOnlyPreviousEndDate(startDate); | ||
|
|
||
| const currentParams: Record<string, unknown> = { | ||
| environmentId, | ||
| organizationId, | ||
| startDate: startDate.toISOString().split('T')[0], | ||
| endDate: endDate.toISOString().split('T')[0], | ||
| }; | ||
|
|
||
| const previousParams: Record<string, unknown> = { | ||
| environmentId, | ||
| organizationId, | ||
| previousStartDate: previousStartDate.toISOString().split('T')[0], | ||
| previousEndDate: adjustedPreviousEndDate, | ||
| }; | ||
|
|
||
| if (workflowIds && workflowIds.length > 0) { | ||
| currentParams.workflowIds = workflowIds; | ||
| previousParams.workflowIds = workflowIds; | ||
| } | ||
|
|
||
| const [currentResult, previousResult] = await Promise.all([ | ||
| this.clickhouseService.query<{ count: string }>({ | ||
| query: currentQuery, | ||
| params: currentParams, | ||
| }), | ||
| this.clickhouseService.query<{ count: string }>({ | ||
| query: previousQuery, | ||
| params: previousParams, | ||
| }), | ||
| ]); | ||
|
|
||
| const currentPeriod = parseInt(currentResult.data[0]?.count || '0', 10); | ||
| const previousPeriod = parseInt(previousResult.data[0]?.count || '0', 10); | ||
|
|
||
| return { | ||
| currentPeriod, | ||
| previousPeriod, | ||
| }; | ||
| } |
There was a problem hiding this comment.
The previousEndDate parameter is accepted but never used.
The method signature accepts previousEndDate (line 36), but the actual query uses adjustedPreviousEndDate calculated from startDate (line 65). This discrepancy could cause incorrect date ranges and confuse callers who expect their previousEndDate value to be respected.
Either use the previousEndDate parameter:
🔧 Option A: Use the passed parameter
- const adjustedPreviousEndDate = getDateOnlyPreviousEndDate(startDate);
-
const currentParams: Record<string, unknown> = {
...
};
const previousParams: Record<string, unknown> = {
environmentId,
organizationId,
previousStartDate: previousStartDate.toISOString().split('T')[0],
- previousEndDate: adjustedPreviousEndDate,
+ previousEndDate: previousEndDate.toISOString().split('T')[0],
};🔧 Option B: Remove the unused parameter from the signature
async getMessageSendCount(
environmentId: string,
organizationId: string,
startDate: Date,
endDate: Date,
previousStartDate: Date,
- previousEndDate: Date,
workflowIds?: string[]
): Promise<{ currentPeriod: number; previousPeriod: number }> {This would require updating all callers accordingly.
📝 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.
| async getMessageSendCount( | |
| environmentId: string, | |
| organizationId: string, | |
| startDate: Date, | |
| endDate: Date, | |
| previousStartDate: Date, | |
| previousEndDate: Date, | |
| workflowIds?: string[] | |
| ): Promise<{ currentPeriod: number; previousPeriod: number }> { | |
| const workflowFilter = | |
| workflowIds && workflowIds.length > 0 ? `AND workflow_id IN {workflowIds:Array(String)}` : ''; | |
| const currentQuery = ` | |
| SELECT sum(count) as count | |
| FROM ${TRACE_ROLLUP_TABLE_NAME} | |
| WHERE | |
| organization_id = {organizationId:String} | |
| AND environment_id = {environmentId:String} | |
| AND event_type = 'message_sent' | |
| AND date >= {startDate:Date} | |
| AND date <= {endDate:Date} | |
| ${workflowFilter} | |
| `; | |
| const previousQuery = ` | |
| SELECT sum(count) as count | |
| FROM ${TRACE_ROLLUP_TABLE_NAME} | |
| WHERE | |
| organization_id = {organizationId:String} | |
| AND environment_id = {environmentId:String} | |
| AND event_type = 'message_sent' | |
| AND date >= {previousStartDate:Date} | |
| AND date <= {previousEndDate:Date} | |
| ${workflowFilter} | |
| `; | |
| const adjustedPreviousEndDate = getDateOnlyPreviousEndDate(startDate); | |
| const currentParams: Record<string, unknown> = { | |
| environmentId, | |
| organizationId, | |
| startDate: startDate.toISOString().split('T')[0], | |
| endDate: endDate.toISOString().split('T')[0], | |
| }; | |
| const previousParams: Record<string, unknown> = { | |
| environmentId, | |
| organizationId, | |
| previousStartDate: previousStartDate.toISOString().split('T')[0], | |
| previousEndDate: adjustedPreviousEndDate, | |
| }; | |
| if (workflowIds && workflowIds.length > 0) { | |
| currentParams.workflowIds = workflowIds; | |
| previousParams.workflowIds = workflowIds; | |
| } | |
| const [currentResult, previousResult] = await Promise.all([ | |
| this.clickhouseService.query<{ count: string }>({ | |
| query: currentQuery, | |
| params: currentParams, | |
| }), | |
| this.clickhouseService.query<{ count: string }>({ | |
| query: previousQuery, | |
| params: previousParams, | |
| }), | |
| ]); | |
| const currentPeriod = parseInt(currentResult.data[0]?.count || '0', 10); | |
| const previousPeriod = parseInt(previousResult.data[0]?.count || '0', 10); | |
| return { | |
| currentPeriod, | |
| previousPeriod, | |
| }; | |
| } | |
| async getMessageSendCount( | |
| environmentId: string, | |
| organizationId: string, | |
| startDate: Date, | |
| endDate: Date, | |
| previousStartDate: Date, | |
| previousEndDate: Date, | |
| workflowIds?: string[] | |
| ): Promise<{ currentPeriod: number; previousPeriod: number }> { | |
| const workflowFilter = | |
| workflowIds && workflowIds.length > 0 ? `AND workflow_id IN {workflowIds:Array(String)}` : ''; | |
| const currentQuery = ` | |
| SELECT sum(count) as count | |
| FROM ${TRACE_ROLLUP_TABLE_NAME} | |
| WHERE | |
| organization_id = {organizationId:String} | |
| AND environment_id = {environmentId:String} | |
| AND event_type = 'message_sent' | |
| AND date >= {startDate:Date} | |
| AND date <= {endDate:Date} | |
| ${workflowFilter} | |
| `; | |
| const previousQuery = ` | |
| SELECT sum(count) as count | |
| FROM ${TRACE_ROLLUP_TABLE_NAME} | |
| WHERE | |
| organization_id = {organizationId:String} | |
| AND environment_id = {environmentId:String} | |
| AND event_type = 'message_sent' | |
| AND date >= {previousStartDate:Date} | |
| AND date <= {previousEndDate:Date} | |
| ${workflowFilter} | |
| `; | |
| const currentParams: Record<string, unknown> = { | |
| environmentId, | |
| organizationId, | |
| startDate: startDate.toISOString().split('T')[0], | |
| endDate: endDate.toISOString().split('T')[0], | |
| }; | |
| const previousParams: Record<string, unknown> = { | |
| environmentId, | |
| organizationId, | |
| previousStartDate: previousStartDate.toISOString().split('T')[0], | |
| previousEndDate: previousEndDate.toISOString().split('T')[0], | |
| }; | |
| if (workflowIds && workflowIds.length > 0) { | |
| currentParams.workflowIds = workflowIds; | |
| previousParams.workflowIds = workflowIds; | |
| } | |
| const [currentResult, previousResult] = await Promise.all([ | |
| this.clickhouseService.query<{ count: string }>({ | |
| query: currentQuery, | |
| params: currentParams, | |
| }), | |
| this.clickhouseService.query<{ count: string }>({ | |
| query: previousQuery, | |
| params: previousParams, | |
| }), | |
| ]); | |
| const currentPeriod = parseInt(currentResult.data[0]?.count || '0', 10); | |
| const previousPeriod = parseInt(previousResult.data[0]?.count || '0', 10); | |
| return { | |
| currentPeriod, | |
| previousPeriod, | |
| }; | |
| } |
| async getMessageSendCount( | |
| environmentId: string, | |
| organizationId: string, | |
| startDate: Date, | |
| endDate: Date, | |
| previousStartDate: Date, | |
| previousEndDate: Date, | |
| workflowIds?: string[] | |
| ): Promise<{ currentPeriod: number; previousPeriod: number }> { | |
| const workflowFilter = | |
| workflowIds && workflowIds.length > 0 ? `AND workflow_id IN {workflowIds:Array(String)}` : ''; | |
| const currentQuery = ` | |
| SELECT sum(count) as count | |
| FROM ${TRACE_ROLLUP_TABLE_NAME} | |
| WHERE | |
| organization_id = {organizationId:String} | |
| AND environment_id = {environmentId:String} | |
| AND event_type = 'message_sent' | |
| AND date >= {startDate:Date} | |
| AND date <= {endDate:Date} | |
| ${workflowFilter} | |
| `; | |
| const previousQuery = ` | |
| SELECT sum(count) as count | |
| FROM ${TRACE_ROLLUP_TABLE_NAME} | |
| WHERE | |
| organization_id = {organizationId:String} | |
| AND environment_id = {environmentId:String} | |
| AND event_type = 'message_sent' | |
| AND date >= {previousStartDate:Date} | |
| AND date <= {previousEndDate:Date} | |
| ${workflowFilter} | |
| `; | |
| const adjustedPreviousEndDate = getDateOnlyPreviousEndDate(startDate); | |
| const currentParams: Record<string, unknown> = { | |
| environmentId, | |
| organizationId, | |
| startDate: startDate.toISOString().split('T')[0], | |
| endDate: endDate.toISOString().split('T')[0], | |
| }; | |
| const previousParams: Record<string, unknown> = { | |
| environmentId, | |
| organizationId, | |
| previousStartDate: previousStartDate.toISOString().split('T')[0], | |
| previousEndDate: adjustedPreviousEndDate, | |
| }; | |
| if (workflowIds && workflowIds.length > 0) { | |
| currentParams.workflowIds = workflowIds; | |
| previousParams.workflowIds = workflowIds; | |
| } | |
| const [currentResult, previousResult] = await Promise.all([ | |
| this.clickhouseService.query<{ count: string }>({ | |
| query: currentQuery, | |
| params: currentParams, | |
| }), | |
| this.clickhouseService.query<{ count: string }>({ | |
| query: previousQuery, | |
| params: previousParams, | |
| }), | |
| ]); | |
| const currentPeriod = parseInt(currentResult.data[0]?.count || '0', 10); | |
| const previousPeriod = parseInt(previousResult.data[0]?.count || '0', 10); | |
| return { | |
| currentPeriod, | |
| previousPeriod, | |
| }; | |
| } | |
| async getMessageSendCount( | |
| environmentId: string, | |
| organizationId: string, | |
| startDate: Date, | |
| endDate: Date, | |
| previousStartDate: Date, | |
| workflowIds?: string[] | |
| ): Promise<{ currentPeriod: number; previousPeriod: number }> { | |
| const workflowFilter = | |
| workflowIds && workflowIds.length > 0 ? `AND workflow_id IN {workflowIds:Array(String)}` : ''; | |
| const currentQuery = ` | |
| SELECT sum(count) as count | |
| FROM ${TRACE_ROLLUP_TABLE_NAME} | |
| WHERE | |
| organization_id = {organizationId:String} | |
| AND environment_id = {environmentId:String} | |
| AND event_type = 'message_sent' | |
| AND date >= {startDate:Date} | |
| AND date <= {endDate:Date} | |
| ${workflowFilter} | |
| `; | |
| const previousQuery = ` | |
| SELECT sum(count) as count | |
| FROM ${TRACE_ROLLUP_TABLE_NAME} | |
| WHERE | |
| organization_id = {organizationId:String} | |
| AND environment_id = {environmentId:String} | |
| AND event_type = 'message_sent' | |
| AND date >= {previousStartDate:Date} | |
| AND date <= {previousEndDate:Date} | |
| ${workflowFilter} | |
| `; | |
| const adjustedPreviousEndDate = getDateOnlyPreviousEndDate(startDate); | |
| const currentParams: Record<string, unknown> = { | |
| environmentId, | |
| organizationId, | |
| startDate: startDate.toISOString().split('T')[0], | |
| endDate: endDate.toISOString().split('T')[0], | |
| }; | |
| const previousParams: Record<string, unknown> = { | |
| environmentId, | |
| organizationId, | |
| previousStartDate: previousStartDate.toISOString().split('T')[0], | |
| previousEndDate: adjustedPreviousEndDate, | |
| }; | |
| if (workflowIds && workflowIds.length > 0) { | |
| currentParams.workflowIds = workflowIds; | |
| previousParams.workflowIds = workflowIds; | |
| } | |
| const [currentResult, previousResult] = await Promise.all([ | |
| this.clickhouseService.query<{ count: string }>({ | |
| query: currentQuery, | |
| params: currentParams, | |
| }), | |
| this.clickhouseService.query<{ count: string }>({ | |
| query: previousQuery, | |
| params: previousParams, | |
| }), | |
| ]); | |
| const currentPeriod = parseInt(currentResult.data[0]?.count || '0', 10); | |
| const previousPeriod = parseInt(previousResult.data[0]?.count || '0', 10); | |
| return { | |
| currentPeriod, | |
| previousPeriod, | |
| }; | |
| } |
| async getActiveSubscribersCount( | ||
| environmentId: string, | ||
| organizationId: string, | ||
| startDate: Date, | ||
| endDate: Date, | ||
| previousStartDate: Date, | ||
| previousEndDate: Date, | ||
| workflowIds?: string[] | ||
| ): Promise<{ currentPeriod: number; previousPeriod: number }> { | ||
| const workflowFilter = | ||
| workflowIds && workflowIds.length > 0 ? `AND workflow_id IN {workflowIds:Array(String)}` : ''; | ||
|
|
||
| const currentQuery = ` | ||
| SELECT count(DISTINCT external_subscriber_id) as count | ||
| FROM ${TRACE_ROLLUP_TABLE_NAME} | ||
| WHERE | ||
| organization_id = {organizationId:String} | ||
| AND environment_id = {environmentId:String} | ||
| AND event_type = 'message_sent' | ||
| AND external_subscriber_id != '' | ||
| AND date >= {startDate:Date} | ||
| AND date <= {endDate:Date} | ||
| ${workflowFilter} | ||
| `; | ||
|
|
||
| const previousQuery = ` | ||
| SELECT count(DISTINCT external_subscriber_id) as count | ||
| FROM ${TRACE_ROLLUP_TABLE_NAME} | ||
| WHERE | ||
| organization_id = {organizationId:String} | ||
| AND environment_id = {environmentId:String} | ||
| AND event_type = 'message_sent' | ||
| AND external_subscriber_id != '' | ||
| AND date >= {previousStartDate:Date} | ||
| AND date <= {previousEndDate:Date} | ||
| ${workflowFilter} | ||
| `; | ||
|
|
||
| const adjustedPreviousEndDate = getDateOnlyPreviousEndDate(startDate); | ||
|
|
||
| const currentParams: Record<string, unknown> = { | ||
| environmentId, | ||
| organizationId, | ||
| startDate: startDate.toISOString().split('T')[0], | ||
| endDate: endDate.toISOString().split('T')[0], | ||
| }; | ||
|
|
||
| const previousParams: Record<string, unknown> = { | ||
| environmentId, | ||
| organizationId, | ||
| previousStartDate: previousStartDate.toISOString().split('T')[0], | ||
| previousEndDate: adjustedPreviousEndDate, | ||
| }; | ||
|
|
||
| if (workflowIds && workflowIds.length > 0) { | ||
| currentParams.workflowIds = workflowIds; | ||
| previousParams.workflowIds = workflowIds; | ||
| } | ||
|
|
||
| const [currentResult, previousResult] = await Promise.all([ | ||
| this.clickhouseService.query<{ count: string }>({ | ||
| query: currentQuery, | ||
| params: currentParams, | ||
| }), | ||
| this.clickhouseService.query<{ count: string }>({ | ||
| query: previousQuery, | ||
| params: previousParams, | ||
| }), | ||
| ]); | ||
|
|
||
| const currentPeriod = parseInt(currentResult.data[0]?.count || '0', 10); | ||
| const previousPeriod = parseInt(previousResult.data[0]?.count || '0', 10); | ||
|
|
||
| return { | ||
| currentPeriod, | ||
| previousPeriod, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Same issue: previousEndDate parameter is unused.
This method has the identical problem—previousEndDate (line 112) is passed but ignored in favor of adjustedPreviousEndDate calculated from startDate (line 144). Apply the same fix as suggested for getMessageSendCount.
🤖 Prompt for AI Agents
In
@libs/application-generic/src/services/analytic-logs/trace-rollup/trace-rollup.repository.ts
around lines 106 - 183, getActiveSubscribersCount is ignoring the
previousEndDate parameter and instead uses adjustedPreviousEndDate computed from
startDate; update the method to use the provided previousEndDate (formatted to
date-only) for previousParams (replace the getDateOnlyPreviousEndDate(startDate)
usage), ensure adjustedPreviousEndDate (or directly
previousParams.previousEndDate) is set from
previousEndDate.toISOString().split('T')[0], and keep workflowIds injection and
query params logic unchanged so the previous period query uses the passed
previousEndDate.
| async getAvgMessagesPerSubscriberData( | ||
| environmentId: string, | ||
| organizationId: string, | ||
| startDate: Date, | ||
| endDate: Date, | ||
| previousStartDate: Date, | ||
| previousEndDate: Date, | ||
| workflowIds?: string[] | ||
| ): Promise<{ currentPeriod: number; previousPeriod: number }> { | ||
| const workflowFilter = | ||
| workflowIds && workflowIds.length > 0 ? `AND workflow_id IN {workflowIds:Array(String)}` : ''; | ||
|
|
||
| const currentQuery = ` | ||
| SELECT | ||
| sum(count) as total_messages, | ||
| count(DISTINCT external_subscriber_id) as unique_subscribers | ||
| FROM ${TRACE_ROLLUP_TABLE_NAME} | ||
| WHERE | ||
| organization_id = {organizationId:String} | ||
| AND environment_id = {environmentId:String} | ||
| AND event_type = 'message_sent' | ||
| AND external_subscriber_id != '' | ||
| AND date >= {startDate:Date} | ||
| AND date <= {endDate:Date} | ||
| ${workflowFilter} | ||
| `; | ||
|
|
||
| const previousQuery = ` | ||
| SELECT | ||
| sum(count) as total_messages, | ||
| count(DISTINCT external_subscriber_id) as unique_subscribers | ||
| FROM ${TRACE_ROLLUP_TABLE_NAME} | ||
| WHERE | ||
| organization_id = {organizationId:String} | ||
| AND environment_id = {environmentId:String} | ||
| AND event_type = 'message_sent' | ||
| AND external_subscriber_id != '' | ||
| AND date >= {previousStartDate:Date} | ||
| AND date <= {previousEndDate:Date} | ||
| ${workflowFilter} | ||
| `; | ||
|
|
||
| const adjustedPreviousEndDate = getDateOnlyPreviousEndDate(startDate); | ||
|
|
||
| const currentParams: Record<string, unknown> = { | ||
| environmentId, | ||
| organizationId, | ||
| startDate: startDate.toISOString().split('T')[0], | ||
| endDate: endDate.toISOString().split('T')[0], | ||
| }; | ||
|
|
||
| const previousParams: Record<string, unknown> = { | ||
| environmentId, | ||
| organizationId, | ||
| previousStartDate: previousStartDate.toISOString().split('T')[0], | ||
| previousEndDate: adjustedPreviousEndDate, | ||
| }; | ||
|
|
||
| if (workflowIds && workflowIds.length > 0) { | ||
| currentParams.workflowIds = workflowIds; | ||
| previousParams.workflowIds = workflowIds; | ||
| } | ||
|
|
||
| const [currentResult, previousResult] = await Promise.all([ | ||
| this.clickhouseService.query<{ total_messages: string; unique_subscribers: string }>({ | ||
| query: currentQuery, | ||
| params: currentParams, | ||
| }), | ||
| this.clickhouseService.query<{ total_messages: string; unique_subscribers: string }>({ | ||
| query: previousQuery, | ||
| params: previousParams, | ||
| }), | ||
| ]); | ||
|
|
||
| const currentTotalMessages = parseInt(currentResult.data[0]?.total_messages || '0', 10); | ||
| const currentUniqueSubscribers = parseInt(currentResult.data[0]?.unique_subscribers || '0', 10); | ||
| const previousTotalMessages = parseInt(previousResult.data[0]?.total_messages || '0', 10); | ||
| const previousUniqueSubscribers = parseInt(previousResult.data[0]?.unique_subscribers || '0', 10); | ||
|
|
||
| const currentPeriod = currentUniqueSubscribers > 0 ? currentTotalMessages / currentUniqueSubscribers : 0; | ||
| const previousPeriod = previousUniqueSubscribers > 0 ? previousTotalMessages / previousUniqueSubscribers : 0; | ||
|
|
||
| return { | ||
| currentPeriod: Math.round(currentPeriod * 100) / 100, | ||
| previousPeriod: Math.round(previousPeriod * 100) / 100, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Same previousEndDate parameter issue; division-by-zero handling is correct.
The previousEndDate parameter (line 240) is unused (line 276 uses calculated value). Apply the same fix pattern.
The division-by-zero check and rounding logic (lines 313-318) are correctly implemented.
🤖 Prompt for AI Agents
In
@libs/application-generic/src/services/analytic-logs/trace-rollup/trace-rollup.repository.ts
around lines 234 - 320, The method getAvgMessagesPerSubscriberData defines a
previousEndDate parameter but then ignores it by using adjustedPreviousEndDate;
update previousParams to use previousEndDate (converted to date-only string)
instead of adjustedPreviousEndDate, or if adjustedPreviousEndDate is required,
remove the unused previousEndDate parameter and its references; ensure
previousParams uses the matching key previousEndDate and workflowIds logic
remains the same so the ClickHouse query receives the correct date value.
| async getTotalInteractionsCount( | ||
| environmentId: string, | ||
| organizationId: string, | ||
| startDate: Date, | ||
| endDate: Date, | ||
| previousStartDate: Date, | ||
| previousEndDate: Date, | ||
| workflowIds?: string[] | ||
| ): Promise<{ currentPeriod: number; previousPeriod: number }> { | ||
| const workflowFilter = | ||
| workflowIds && workflowIds.length > 0 ? `AND workflow_id IN {workflowIds:Array(String)}` : ''; | ||
|
|
||
| const currentQuery = ` | ||
| SELECT sum(count) as count | ||
| FROM ${TRACE_ROLLUP_TABLE_NAME} | ||
| WHERE | ||
| organization_id = {organizationId:String} | ||
| AND environment_id = {environmentId:String} | ||
| AND event_type IN ('message_seen', 'message_read', 'message_snoozed', 'message_archived') | ||
| AND date >= {startDate:Date} | ||
| AND date <= {endDate:Date} | ||
| ${workflowFilter} | ||
| `; | ||
|
|
||
| const previousQuery = ` | ||
| SELECT sum(count) as count | ||
| FROM ${TRACE_ROLLUP_TABLE_NAME} | ||
| WHERE | ||
| organization_id = {organizationId:String} | ||
| AND environment_id = {environmentId:String} | ||
| AND event_type IN ('message_seen', 'message_read', 'message_snoozed', 'message_archived') | ||
| AND date >= {previousStartDate:Date} | ||
| AND date <= {previousEndDate:Date} | ||
| ${workflowFilter} | ||
| `; | ||
|
|
||
| const adjustedPreviousEndDate = getDateOnlyPreviousEndDate(startDate); | ||
|
|
||
| const currentParams: Record<string, unknown> = { | ||
| environmentId, | ||
| organizationId, | ||
| startDate: startDate.toISOString().split('T')[0], | ||
| endDate: endDate.toISOString().split('T')[0], | ||
| }; | ||
|
|
||
| const previousParams: Record<string, unknown> = { | ||
| environmentId, | ||
| organizationId, | ||
| previousStartDate: previousStartDate.toISOString().split('T')[0], | ||
| previousEndDate: adjustedPreviousEndDate, | ||
| }; | ||
|
|
||
| if (workflowIds && workflowIds.length > 0) { | ||
| currentParams.workflowIds = workflowIds; | ||
| previousParams.workflowIds = workflowIds; | ||
| } | ||
|
|
||
| const [currentResult, previousResult] = await Promise.all([ | ||
| this.clickhouseService.query<{ count: string }>({ | ||
| query: currentQuery, | ||
| params: currentParams, | ||
| }), | ||
| this.clickhouseService.query<{ count: string }>({ | ||
| query: previousQuery, | ||
| params: previousParams, | ||
| }), | ||
| ]); | ||
|
|
||
| const currentPeriod = parseInt(currentResult.data[0]?.count || '0', 10); | ||
| const previousPeriod = parseInt(previousResult.data[0]?.count || '0', 10); | ||
|
|
||
| return { | ||
| currentPeriod, | ||
| previousPeriod, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Same previousEndDate parameter issue.
The previousEndDate parameter (line 328) is unused (line 358). Apply the consistent fix across all affected methods.
🤖 Prompt for AI Agents
In
@libs/application-generic/src/services/analytic-logs/trace-rollup/trace-rollup.repository.ts
around lines 322 - 397, The previousEndDate parameter passed into
getTotalInteractionsCount is never used; replace the computed
adjustedPreviousEndDate (currently from getDateOnlyPreviousEndDate(startDate))
with a string derived from the previousEndDate parameter and assign that to
previousParams.previousEndDate (e.g.,
previousEndDate.toISOString().split('T')[0]), removing the unused
adjustedPreviousEndDate computation; update references in
getTotalInteractionsCount so previousParams uses the provided previousEndDate
value.
| ) | ||
| ENGINE = SummingMergeTree(count) | ||
| PARTITION BY toYYYYMM(date) | ||
| ORDER BY (organization_id, environment_id, workflow_id, date, external_subscriber_id); |
There was a problem hiding this comment.
LEt's make sure that this is optimized for environment id + date. We need to make sure that in majority of cases so we should optimize for it
There was a problem hiding this comment.
we can't remove columns because SummingMergeTree is using it in order to summarize. Each defines a dimension that we group by, removing any data that would merge data that should stay separate.
i did changed the order in order to make sure we always use the columns that are always used, like date.
| ) | ||
| ENGINE = SummingMergeTree(count) | ||
| PARTITION BY toYYYYMM(date) | ||
| ORDER BY (organization_id, environment_id, date, workflow_id, step_type); |
There was a problem hiding this comment.
Same here for order by optimization
apps/api/migrations/clickhouse-migrations/3_analytics_tables.sql
Outdated
Show resolved
Hide resolved
libs/application-generic/src/services/analytic-logs/trace-log/trace-log.schema.ts
Outdated
Show resolved
Hide resolved
djabarovgeorge
temporarily deployed
to
Linting
GitHub Actions
Inactive
djabarovgeorge
temporarily deployed
to
Linting
GitHub Actions
Inactive
djabarovgeorge
temporarily deployed
to
Linting
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/api/migrations/clickhouse-migrations/3_analytics_tables.sql (1)
41-53: AddDEFAULT ''toworkflow_idfor consistency and robustness.Unlike
delivery_trend_counts(line 9),workflow_idhere lacks a default. While the MV handles NULLs viaifNull, direct inserts (e.g., backfill queries) could encounter issues. For consistency and defensive schema design:Suggested fix
CREATE TABLE IF NOT EXISTS trace_rollup ( date Date, organization_id String, environment_id String, - workflow_id String, + workflow_id String DEFAULT '', external_subscriber_id String DEFAULT '', event_type LowCardinality(String), provider_id String DEFAULT '', count UInt64 )
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/api/migrations/clickhouse-migrations/3_analytics_tables.sqllibs/application-generic/src/services/analytic-logs/trace-log/trace-log.schema.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/application-generic/src/services/analytic-logs/trace-log/trace-log.schema.ts
🧰 Additional context used
📓 Path-based instructions (1)
**
📄 CodeRabbit inference engine (.cursor/rules/novu.mdc)
Use lowercase with dashes for directories and files (e.g., components/auth-wizard)
Files:
apps/api/migrations/clickhouse-migrations/3_analytics_tables.sql
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: Unit test backend services (@novu/api-service)
- GitHub Check: E2E test API / Test E2E
- GitHub Check: Unit test backend services (@novu/webhook)
- GitHub Check: Unit test backend services (@novu/inbound-mail)
- GitHub Check: Unit test backend services (@novu/ws)
- GitHub Check: Unit test backend services (@novu/worker)
- GitHub Check: Unit test @novu internal packages
- GitHub Check: Validate OpenAPI
- GitHub Check: Unit test @novu public NPM packages (except providers)
- GitHub Check: Redirect rules - dashboard-v2-novu-staging
- GitHub Check: Header rules - dashboard-v2-novu-staging
- GitHub Check: Pages changed - dashboard-v2-novu-staging
- GitHub Check: Analyze (javascript)
- GitHub Check: Analyze (typescript)
- GitHub Check: publish_preview_packages
🔇 Additional comments (4)
apps/api/migrations/clickhouse-migrations/3_analytics_tables.sql (4)
5-15: LGTM!Good use of
SummingMergeTreefor count aggregation,LowCardinalityfor the limitedstep_typevalues, and monthly partitioning. The ORDER BY column order is well-suited for tenant-scoped time-range queries.
17-30: Verify historical data backfill strategy.Materialized views only populate from new inserts after creation. Existing rows in
step_runswon't appear indelivery_trend_counts. If historical data is needed, a separateINSERT INTO delivery_trend_counts SELECT ...backfill statement should be run after this migration.
32-36: LGTM!Correctly ordered before the MV that references it. Using
IF NOT EXISTSensures idempotency for re-runnable migrations.
55-69: Minor:ifNull(provider_id, '')is redundant but harmless.Since
provider_idwas added asString DEFAULT ''(non-nullable in ClickHouse), it can never be NULL. TheifNullwrapper is technically unnecessary but serves as defensive coding if the schema ever changes. No action required.Also, same note as above: historical
tracesdata won't be backfilled automatically.
… for improved query performance
djabarovgeorge
temporarily deployed
to
Linting
GitHub Actions
Inactive
djabarovgeorge
temporarily deployed
to
Linting
GitHub Actions
Inactive
djabarovgeorge
temporarily deployed
to
Linting
GitHub Actions
Inactive
djabarovgeorge
temporarily deployed
to
Linting
GitHub Actions
Inactive
djabarovgeorge
temporarily deployed
to
Linting
GitHub Actions
Inactive
djabarovgeorge
temporarily deployed
to
Linting
GitHub Actions
Inactive
djabarovgeorge
temporarily deployed
to
Linting
GitHub Actions
Inactive
djabarovgeorge
temporarily deployed
to
Linting
GitHub Actions
Inactive
djabarovgeorge
temporarily deployed
to
Linting
GitHub Actions
Inactive
2 participants
What changed? Why was the change needed?
EE-PR
Screenshots
Expand for optional sections
Related enterprise PR
Special notes for your reviewer