feat(api-service): use new workflow run count table in usage page#10074
feat(api-service): use new workflow run count table in usage page#10074djabarovgeorge wants to merge 2 commits intonextfrom
Conversation
… data instead of traces - Replaced TraceLogRepository with WorkflowRunCountRepository in build-workflow-runs-trend-chart.usecase.ts. - Updated feature flag from IS_WORKFLOW_RUN_TREND_FROM_TRACES_ENABLED to IS_WORKFLOW_RUN_TREND_FROM_ROLLUP_ENABLED. - Modified chart building logic to utilize workflow run counts for trend data. - Added new method getWorkflowRunsTrendData in WorkflowRunCountRepository for fetching trend data from the database.
…port - Introduced new method getWorkflowVolumeData in WorkflowRunCountRepository to fetch workflow volume data. - Updated BuildWorkflowByVolumeChart to utilize workflow run counts based on feature flag for rollup data. - Integrated notification template fetching to associate workflow names with their identifiers in the chart data. - Refactored chart building logic to accommodate both rollup and standard workflow run data.
✅ Deploy preview added
To edit notification comments on pull requests, go to your Netlify project configuration. |
djabarovgeorge
temporarily deployed
to
Linting
GitHub Actions
Inactive
djabarovgeorge
temporarily deployed
to
Linting
GitHub Actions
Inactive
djabarovgeorge
temporarily deployed
to
Linting
GitHub Actions
Inactive
|
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 |
github-actions
bot
added
@novu/dal
@novu/shared
@novu/dashboard
@novu/api-service
labels
LaunchDarkly flag references❌ 1 flag removed
|
WalkthroughThis pull request refactors the workflow analytics data pipeline to use rollup-based counts instead of trace logs. A feature flag 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
apps/api/src/app/activity/usecases/build-workflow-by-volume-chart/build-workflow-by-volume-chart.usecase.ts (1)
44-81: The rollup chart-building logic is well-structured. Good use of selective field projection (['name', 'triggers']) and the fallback toworkflow_run_idwhen no template name is found.One consideration:
workflow_run_idin the rollup table stores a trigger identifier, not an actual workflow run ID. This naming mismatch could confuse future maintainers reading this code. A clarifying inline comment at line 61 would help, e.g., noting thatworkflow_run_idis actually a trigger identifier in this context.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/activity/usecases/build-workflow-by-volume-chart/build-workflow-by-volume-chart.usecase.ts` around lines 44 - 81, Add an inline clarifying comment in buildChartFromWorkflowRunCount explaining that the rollup column workflow_run_id actually contains a trigger identifier (not a real workflow run ID) to avoid confusion for future maintainers; place the comment near where workflowVolumes is mapped to triggerIdentifiers / when reading row.workflow_run_id (references: buildChartFromWorkflowRunCount, workflowVolumes, triggerIdentifiers, nameByIdentifier) so it's obvious why we use workflow_run_id as the lookup key for template names.libs/dal/src/repositories/notification-template/notification-template.repository.ts (1)
66-98: Clean overload design for field projection support.The conditional
steps.templatepopulation (line 93) is a good optimization. One subtle note: whenselectis provided without_id, MongoDB still returns_idby default, so consumers will get_ideven if not listed inselect. This is likely desirable but worth being aware of — thePick<NotificationTemplateEntity, K>return type won't include_idin its type signature unless explicitly selected, creating a minor type/runtime mismatch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/dal/src/repositories/notification-template/notification-template.repository.ts` around lines 66 - 98, The runtime projection currently omits explicit _id handling so MongoDB will still include _id by default while the TypeScript return type Pick<NotificationTemplateEntity, K> may not include it; update findByTriggerIdentifierBulk to ensure projection matches the types by adding _id: 1 whenever a select array is provided (i.e., when building projection from select ensure projection._id = 1 if not already present), and keep the rest of the logic (projection variable, baseQuery, query population, and the returned mapEntities) unchanged so runtime and types remain consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/api/src/app/activity/usecases/build-workflow-by-volume-chart/build-workflow-by-volume-chart.usecase.ts`:
- Around line 30-41: The code silently ignores workflowIds when the rollup
feature flag is enabled; update the call-site and implementation so workflowIds
are not dropped: change buildChartFromWorkflowRunCount to accept an optional
workflowIds parameter and apply that filter inside its logic (or if filtering by
workflow is not supported, at minimum add a clear log via processLogger.warn
inside build-workflow-by-volume-chart.usecase.ts before returning to state that
non-empty workflowIds are being ignored when isRollupEnabled is true),
referencing the isRollupEnabled check and the functions
buildChartFromWorkflowRunCount and buildChartFromWorkflowRuns so the behavior is
explicit.
In
`@apps/api/src/app/activity/usecases/build-workflow-runs-trend-chart/build-workflow-runs-trend-chart.usecase.ts`:
- Around line 35-39: When isRollupEnabled is true the code calls
buildChartFromWorkflowRunCount and silently drops the workflowIds filter; update
the branch that currently returns buildChartFromWorkflowRunCount(startDate,
endDate, environmentId, organizationId) to check if workflowIds is non-empty and
emit a warning via the use case's logger (e.g., this.logger.warn or existing
logging utility) stating that workflowIds are ignored for rollup mode, or
alternatively make buildChartFromWorkflowRunCount accept and apply workflowIds
if the rollup data supports it; reference the isRollupEnabled flag and the
buildChartFromWorkflowRunCount / buildChartFromWorkflowRuns call sites when
implementing the change.
---
Nitpick comments:
In
`@apps/api/src/app/activity/usecases/build-workflow-by-volume-chart/build-workflow-by-volume-chart.usecase.ts`:
- Around line 44-81: Add an inline clarifying comment in
buildChartFromWorkflowRunCount explaining that the rollup column workflow_run_id
actually contains a trigger identifier (not a real workflow run ID) to avoid
confusion for future maintainers; place the comment near where workflowVolumes
is mapped to triggerIdentifiers / when reading row.workflow_run_id (references:
buildChartFromWorkflowRunCount, workflowVolumes, triggerIdentifiers,
nameByIdentifier) so it's obvious why we use workflow_run_id as the lookup key
for template names.
In
`@libs/dal/src/repositories/notification-template/notification-template.repository.ts`:
- Around line 66-98: The runtime projection currently omits explicit _id
handling so MongoDB will still include _id by default while the TypeScript
return type Pick<NotificationTemplateEntity, K> may not include it; update
findByTriggerIdentifierBulk to ensure projection matches the types by adding
_id: 1 whenever a select array is provided (i.e., when building projection from
select ensure projection._id = 1 if not already present), and keep the rest of
the logic (projection variable, baseQuery, query population, and the returned
mapEntities) unchanged so runtime and types remain consistent.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/api/src/app/activity/usecases/build-workflow-by-volume-chart/build-workflow-by-volume-chart.usecase.tsapps/api/src/app/activity/usecases/build-workflow-runs-trend-chart/build-workflow-runs-trend-chart.usecase.tsapps/api/src/app/shared/shared.module.tsapps/dashboard/src/components/analytics/charts/workflow-runs-trend-chart.tsxlibs/application-generic/src/services/analytic-logs/workflow-run-count/workflow-run-count.repository.tslibs/dal/src/repositories/notification-template/notification-template.repository.tspackages/shared/src/types/feature-flags.ts
| const isRollupEnabled = await this.featureFlagsService.getFlag({ | ||
| key: FeatureFlagsKeysEnum.IS_WORKFLOW_RUN_TREND_FROM_ROLLUP_ENABLED, | ||
| defaultValue: false, | ||
| organization: { _id: organizationId }, | ||
| environment: { _id: environmentId }, | ||
| }); | ||
|
|
||
| if (isRollupEnabled) { | ||
| return this.buildChartFromWorkflowRunCount(startDate, endDate, environmentId, organizationId); | ||
| } | ||
|
|
||
| return this.buildChartFromWorkflowRuns(startDate, endDate, environmentId, organizationId, workflowIds); |
There was a problem hiding this comment.
Same observation as the trend chart: workflowIds is silently ignored when rollup is enabled.
When the rollup flag is on, buildChartFromWorkflowRunCount ignores the workflowIds parameter. This is the same pattern as in BuildWorkflowRunsTrendChart. If this is expected, consider at minimum adding a log when workflowIds is non-empty but unused.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/api/src/app/activity/usecases/build-workflow-by-volume-chart/build-workflow-by-volume-chart.usecase.ts`
around lines 30 - 41, The code silently ignores workflowIds when the rollup
feature flag is enabled; update the call-site and implementation so workflowIds
are not dropped: change buildChartFromWorkflowRunCount to accept an optional
workflowIds parameter and apply that filter inside its logic (or if filtering by
workflow is not supported, at minimum add a clear log via processLogger.warn
inside build-workflow-by-volume-chart.usecase.ts before returning to state that
non-empty workflowIds are being ignored when isRollupEnabled is true),
referencing the isRollupEnabled check and the functions
buildChartFromWorkflowRunCount and buildChartFromWorkflowRuns so the behavior is
explicit.
| if (isRollupEnabled) { | ||
| return this.buildChartFromWorkflowRunCount(startDate, endDate, environmentId, organizationId); | ||
| } | ||
|
|
||
| return this.buildChartFromWorkflowRuns(startDate, endDate, environmentId, organizationId, workflowIds); |
There was a problem hiding this comment.
Note: workflowIds filter is silently ignored when rollup is enabled.
When isRollupEnabled is true, buildChartFromWorkflowRunCount doesn't accept or use the workflowIds parameter from the command, meaning any workflow-specific filtering the caller requested is silently dropped. If this is intentional (rollup table doesn't support per-workflow filtering), consider logging a warning when workflowIds is provided but ignored, so callers aren't surprised by unfiltered results.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/api/src/app/activity/usecases/build-workflow-runs-trend-chart/build-workflow-runs-trend-chart.usecase.ts`
around lines 35 - 39, When isRollupEnabled is true the code calls
buildChartFromWorkflowRunCount and silently drops the workflowIds filter; update
the branch that currently returns buildChartFromWorkflowRunCount(startDate,
endDate, environmentId, organizationId) to check if workflowIds is non-empty and
emit a warning via the use case's logger (e.g., this.logger.warn or existing
logging utility) stating that workflowIds are ignored for rollup mode, or
alternatively make buildChartFromWorkflowRunCount accept and apply workflowIds
if the rollup data supports it; reference the isRollupEnabled flag and the
buildChartFromWorkflowRunCount / buildChartFromWorkflowRuns call sites when
implementing the change.
1 participant
What changed? Why was the change needed?
Screenshots
Expand for optional sections
Related enterprise PR
Special notes for your reviewer